From owner-freebsd-audit Tue Apr 2 1:11:48 2002 Delivered-To: freebsd-audit@freebsd.org Received: from relay1.macomnet.ru (relay1.macomnet.ru [195.128.64.10]) by hub.freebsd.org (Postfix) with ESMTP id 25BDF37B41A for ; Tue, 2 Apr 2002 01:11:33 -0800 (PST) Received: from news1.macomnet.ru (maxim@news1.macomnet.ru [195.128.64.14]) by relay1.macomnet.ru (8.11.6/8.11.6) with ESMTP id g329BVT9778948 for ; Tue, 2 Apr 2002 13:11:31 +0400 (MSD) Date: Tue, 2 Apr 2002 13:11:31 +0400 (MSD) From: Maxim Konovalov To: freebsd-audit@freebsd.org Subject: a couple of ping(8) patches for review Message-ID: <20020402114845.U76561-100000@news1.macomnet.ru> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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 #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 majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message