a couple of ping(8) patches for review
Maxim Konovalov <[email protected]>
| Newsgroups | gmane.os.freebsd.devel.audit |
|---|---|
| Message-ID | <[email protected]> |
Hello -audit,
I am going to commit two patches below. Bruce Evans has already
reviewed them and now I would like to ask -audit. The first patch is
based on Kris's code.
#1
o correct a maximum payload size,
o replace serveral magic numbers by constants,
o allow zero payload,
o increase an input buffer size.
Index: ping.c
===================================================================
RCS file: /home/ncvs/src/sbin/ping/ping.c,v
retrieving revision 1.64
diff -u -r1.64 ping.c
--- ping.c 23 Mar 2002 18:10:59 -0000 1.64
+++ ping.c 2 Apr 2002 08:23:42 -0000
@@ -100,12 +100,12 @@
#define DEFDATALEN (64 - PHDR_LEN) /* default data length */
#define FLOOD_BACKOFF 20000 /* usecs to back off if F_FLOOD mode */
/* runs out of buffer space */
-#define MAXIPLEN 60
-#define MAXICMPLEN 76
-#define MAXPACKET (65536 - 60 - 8)/* max packet size */
+#define MAXIPLEN (sizeof(struct ip) + MAX_IPOPTLEN)
+#define MAXICMPLEN (ICMP_ADVLENMIN + MAX_IPOPTLEN)
+#define MINICMPLEN ICMP_MINLEN
+#define MAXPAYLOAD (IP_MAXPACKET - MAXIPLEN - MINICMPLEN)
#define MAXWAIT 10 /* max seconds to wait for response */
#define MAXALARM (60 * 60) /* max seconds for alarm timeout */
-#define NROUTES 9 /* number of record route slots */
#define A(bit) rcvd_tbl[(bit)>>3] /* identify byte in array */
#define B(bit) (1 << ((bit) & 0x07)) /* identify bit in byte */
@@ -149,7 +149,7 @@
struct sockaddr_in whereto; /* who to ping */
int datalen = DEFDATALEN;
int s; /* socket file descriptor */
-u_char outpack[MAXPACKET];
+u_char outpack[MINICMPLEN + MAXPAYLOAD];
char BSPACE = '\b'; /* characters written for flood */
char BBELL = '\a'; /* characters written for MISSED and AUDIBLE */
char DOT = '.';
@@ -206,7 +206,7 @@
struct timeval last, intvl;
struct hostent *hp;
struct sockaddr_in *to;
- u_char *datap, *packet;
+ u_char *datap, packet[IP_MAXPACKET];
char *ep, *source, *target;
#ifdef IPSEC_POLICY_IPSEC
char *policy_in, *policy_out;
@@ -216,7 +216,7 @@
char ctrl[CMSG_SPACE(sizeof(struct timeval))];
char hnamebuf[MAXHOSTNAMELEN], snamebuf[MAXHOSTNAMELEN];
#ifdef IP_OPTIONS
- char rspace[3 + 4 * NROUTES + 1]; /* record route space */
+ char rspace[MAX_IPOPTLEN]; /* record route space */
#endif
unsigned char mttl, loop;
@@ -238,7 +238,7 @@
alarmtimeout = preload = 0;
- datap = &outpack[8 + PHDR_LEN];
+ datap = &outpack[MINICMPLEN + PHDR_LEN];
while ((ch = getopt(argc, argv,
"AI:LQRS:T:c:adfi:l:m:np:qrs:t:v"
#ifdef IPSEC
@@ -348,10 +348,11 @@
err(EX_NOPERM, "-s flag");
}
ultmp = strtoul(optarg, &ep, 0);
- if (ultmp > MAXPACKET)
- errx(EX_USAGE, "packet size too large: %lu",
- ultmp);
- if (*ep || ep == optarg || !ultmp)
+ if (ultmp > MAXPAYLOAD)
+ errx(EX_USAGE,
+ "packet size too large: %lu > %u",
+ ultmp, MAXPAYLOAD);
+ if (*ep || ep == optarg)
errx(EX_USAGE, "invalid packet size: `%s'",
optarg);
datalen = ultmp;
@@ -414,7 +415,8 @@
source, hstrerror(h_errno));
sin.sin_len = sizeof sin;
- if (hp->h_length > sizeof(sin.sin_addr))
+ if (hp->h_length > sizeof(sin.sin_addr) ||
+ hp->h_length < 0)
errx(1,"gethostbyname2: illegal address");
memcpy(&sin.sin_addr, hp->h_addr_list[0],
sizeof (sin.sin_addr));
@@ -460,9 +462,8 @@
if (datalen >= PHDR_LEN) /* can we time transfer */
timing = 1;
- packlen = datalen + MAXIPLEN + MAXICMPLEN;
- if (!(packet = (u_char *)malloc((size_t)packlen)))
- err(EX_UNAVAILABLE, "malloc");
+ packlen = MAXIPLEN + MAXICMPLEN + datalen;
+ packlen = packlen > IP_MAXPACKET ? IP_MAXPACKET : packlen;
if (!(options & F_PINGFILLED))
for (i = PHDR_LEN; i < datalen; ++i)
@@ -563,7 +564,12 @@
* /etc/ethers. But beware: RFC 1122 allows hosts to ignore broadcast
* or multicast pings if they wish.
*/
- hold = 48 * 1024;
+
+ /*
+ * XXX receive buffer needs undetermined space for mbuf overhead
+ * as well.
+ */
+ hold = IP_MAXPACKET + 128;
(void)setsockopt(s, SOL_SOCKET, SO_RCVBUF, (char *)&hold,
sizeof(hold));
@@ -742,9 +748,9 @@
* pinger --
* Compose and transmit an ICMP ECHO REQUEST packet. The IP packet
* will be added on by the kernel. The ID field is our UNIX process ID,
- * and the sequence number is an ascending integer. The first 8 bytes
- * of the data portion are used to hold a UNIX "timeval" struct in host
- * byte-order, to compute the round-trip time.
+ * and the sequence number is an ascending integer. The first PHDR_LEN
+ * bytes of the data portion are used to hold a UNIX "timeval" struct in
+ * host byte-order, to compute the round-trip time.
*/
static void
pinger(void)
@@ -762,10 +768,10 @@
CLR(ntransmitted % mx_dup_ck);
if (timing)
- (void)gettimeofday((struct timeval *)&outpack[8],
+ (void)gettimeofday((struct timeval *)&outpack[MINICMPLEN],
(struct timezone *)NULL);
- cc = datalen + PHDR_LEN; /* skips ICMP portion */
+ cc = MINICMPLEN + datalen;
/* compute ICMP checksum here */
icp->icmp_cksum = in_cksum((u_short *)icp, cc);
@@ -881,7 +887,7 @@
(void)write(STDOUT_FILENO, &BBELL, 1);
/* check the data */
cp = (u_char*)&icp->icmp_data[PHDR_LEN];
- dp = &outpack[8 + PHDR_LEN];
+ dp = &outpack[MINICMPLEN + PHDR_LEN];
for (i = PHDR_LEN; i < datalen; ++i, ++cp, ++dp) {
if (*cp != *dp) {
(void)printf("\nwrong data byte #%d should be 0x%x but was 0x%x",
@@ -894,7 +900,7 @@
(void)printf("%x ", *cp);
}
printf("\ndp:");
- cp = &outpack[8];
+ cp = &outpack[MINICMPLEN];
for (i = 0; i < datalen; ++i, ++cp) {
if ((i % 32) == 8)
(void)printf("\n\t");
@@ -1416,7 +1422,7 @@
{
char *cp;
int pat[16];
- int ii, jj, kk;
+ u_int ii, jj, kk;
for (cp = patp; *cp; cp++) {
if (!isxdigit(*cp))
@@ -1431,9 +1437,7 @@
&pat[13], &pat[14], &pat[15]);
if (ii > 0)
- for (kk = 0;
- kk <= MAXPACKET - (8 + PHDR_LEN + ii);
- kk += ii)
+ for (kk = 0; kk <= MAXPAYLOAD - (PHDR_LEN + ii); kk += ii)
for (jj = 0; jj < ii; ++jj)
bp[jj + kk] = pat[jj];
if (!(options & F_QUIET)) {
%%%
#2
Rewrote IP options parsing:
o more strict boundary checks,
o replace magic numbers by constants.
The diff is practically unreadable, sorry.
Index: ping.c
===================================================================
RCS file: /home/ncvs/src/sbin/ping/ping.c,v
retrieving revision 1.64
diff -u -r1.64 ping.c
--- ping.c 23 Mar 2002 18:10:59 -0000 1.64
+++ ping.c 2 Apr 2002 08:20:16 -0000
@@ -96,6 +96,7 @@
#include <netinet6/ipsec.h>
#endif /*IPSEC*/
+#define INADDR_LEN ((int)sizeof(in_addr_t))
#define PHDR_LEN sizeof(struct timeval)
#define DEFDATALEN (64 - PHDR_LEN) /* default data length */
#define FLOOD_BACKOFF 20000 /* usecs to back off if F_FLOOD mode */
@@ -806,10 +807,10 @@
{
struct icmp *icp;
struct ip *ip;
+ struct in_addr ina;
struct timeval *tp;
u_char *cp, *dp;
double triptime;
- u_long l;
int dupflag, hlen, i, j, seq;
static int old_rrlen;
static char old_rr[MAX_IPOPTLEN];
@@ -945,79 +946,68 @@
break;
case IPOPT_LSRR:
(void)printf("\nLSRR: ");
+ j = cp[IPOPT_OLEN] - IPOPT_MINOFF + 1;
hlen -= 2;
- j = *++cp;
- ++cp;
- if (j > IPOPT_MINOFF)
+ cp += 2;
+ if (j >= INADDR_LEN && j <= hlen - INADDR_LEN) {
for (;;) {
- l = *++cp;
- l = (l<<8) + *++cp;
- l = (l<<8) + *++cp;
- l = (l<<8) + *++cp;
- if (l == 0) {
+ bcopy(++cp, &ina.s_addr, INADDR_LEN);
+ if (ina.s_addr == 0)
printf("\t0.0.0.0");
- } else {
- struct in_addr ina;
- ina.s_addr = ntohl(l);
+ else
printf("\t%s", pr_addr(ina));
- }
- hlen -= 4;
- j -= 4;
- if (j <= IPOPT_MINOFF)
- break;
- (void)putchar('\n');
- }
+ hlen -= INADDR_LEN;
+ cp += INADDR_LEN - 1;
+ j -= INADDR_LEN;
+ if (j < INADDR_LEN)
+ break;
+ (void)putchar('\n');
+ }
+ } else
+ (void)printf("\t(truncated route)\n");
break;
case IPOPT_RR:
- j = *++cp; /* get length */
- i = *++cp; /* and pointer */
+ j = cp[IPOPT_OLEN]; /* get length */
+ i = cp[IPOPT_OFFSET]; /* and pointer */
hlen -= 2;
+ cp += 2;
if (i > j)
i = j;
- i -= IPOPT_MINOFF;
- if (i <= 0)
+ i = i - IPOPT_MINOFF + 1;
+ if (i < 0 || i > (hlen - (int)sizeof(struct ip))) {
+ old_rrlen = 0;
continue;
+ }
if (i == old_rrlen
- && cp == (u_char *)buf + sizeof(struct ip) + 2
&& !bcmp((char *)cp, old_rr, i)
&& !(options & F_FLOOD)) {
(void)printf("\t(same route)");
- i = ((i + 3) / 4) * 4;
hlen -= i;
cp += i;
break;
}
- if (i < MAX_IPOPTLEN) {
- old_rrlen = i;
- bcopy((char *)cp, old_rr, i);
- } else
- old_rrlen = 0;
+ old_rrlen = i;
+ bcopy((char *)cp, old_rr, i);
(void)printf("\nRR: ");
- j = 0;
- for (;;) {
- l = *++cp;
- l = (l<<8) + *++cp;
- l = (l<<8) + *++cp;
- l = (l<<8) + *++cp;
- if (l == 0) {
- printf("\t0.0.0.0");
- } else {
- struct in_addr ina;
- ina.s_addr = ntohl(l);
- printf("\t%s", pr_addr(ina));
- }
- hlen -= 4;
- i -= 4;
- j += 4;
- if (i <= 0)
- break;
- if (j >= MAX_IPOPTLEN) {
- (void) printf("\t(truncated route)");
- break;
+
+ if (i >= INADDR_LEN &&
+ i <= hlen - (int)sizeof(struct ip)) {
+ for (;;) {
+ bcopy(++cp, &ina.s_addr, INADDR_LEN);
+ if (ina.s_addr == 0)
+ printf("\t0.0.0.0");
+ else
+ printf("\t%s", pr_addr(ina));
+ hlen -= INADDR_LEN;
+ cp += INADDR_LEN - 1;
+ i -= INADDR_LEN;
+ if (i < INADDR_LEN)
+ break;
+ (void)putchar('\n');
}
- (void)putchar('\n');
- }
+ } else
+ (void)printf("\t(truncated route)");
break;
case IPOPT_NOP:
(void)printf("\nNOP");
%%%
Thanks,
Maxim.
To Unsubscribe: send mail to [email protected]
with "unsubscribe freebsd-audit" in the body of the message