From owner-freebsd-audit Mon Jul 31 4: 7:12 2000 Delivered-To: freebsd-audit@freebsd.org Received: from freefall.freebsd.org (freefall.FreeBSD.ORG [204.216.27.21]) by hub.freebsd.org (Postfix) with ESMTP id DC3E537BC7A for ; Mon, 31 Jul 2000 04:07:11 -0700 (PDT) (envelope-from kris@FreeBSD.org) Received: from localhost (kris@localhost) by freefall.freebsd.org (8.9.3/8.9.2) with ESMTP id EAA02456 for ; Mon, 31 Jul 2000 04:07:11 -0700 (PDT) (envelope-from kris@FreeBSD.org) X-Authentication-Warning: freefall.freebsd.org: kris owned process doing -bs Date: Mon, 31 Jul 2000 04:07:11 -0700 (PDT) From: Kris Kennaway To: audit@freebsd.org Subject: Fuzz testing Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG Well, if anyone is looking for something to do, this came up on bugtraq: ftp://grilled.cs.wisc.edu/fuzz/ basically it's a set of tools which stuff random input into command-line utils to look for bugs. There are probably a few easy targets here (well, I know there are)..if you find one, check with OpenBSD to see if they've already fixed it to save yourself some work. Kris -- In God we Trust -- all others must submit an X.509 certificate. -- Charles Forsythe To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Mon Jul 31 4:10: 2 2000 Delivered-To: freebsd-audit@freebsd.org Received: from freefall.freebsd.org (freefall.FreeBSD.ORG [204.216.27.21]) by hub.freebsd.org (Postfix) with ESMTP id 8B83537BC6A for ; Mon, 31 Jul 2000 04:09:59 -0700 (PDT) (envelope-from kris@FreeBSD.org) Received: from localhost (kris@localhost) by freefall.freebsd.org (8.9.3/8.9.2) with ESMTP id EAA02659 for ; Mon, 31 Jul 2000 04:09:59 -0700 (PDT) (envelope-from kris@FreeBSD.org) X-Authentication-Warning: freefall.freebsd.org: kris owned process doing -bs Date: Mon, 31 Jul 2000 04:09:59 -0700 (PDT) From: Kris Kennaway To: audit@freebsd.org Subject: Re: Fuzz testing In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Mon, 31 Jul 2000, Kris Kennaway wrote: > Well, if anyone is looking for something to do, this came up on bugtraq: > > ftp://grilled.cs.wisc.edu/fuzz/ > > basically it's a set of tools which stuff random input into command-line > utils to look for bugs. There are probably a few easy targets here (well, > I know there are)..if you find one, check with OpenBSD to see if they've > already fixed it to save yourself some work. For example: a2p.core as.core csh.core flex++.core flex.core sh.core :-) Other bugs this can expose are data dependent infinite loops. Kris -- In God we Trust -- all others must submit an X.509 certificate. -- Charles Forsythe To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Mon Jul 31 6:35:24 2000 Delivered-To: freebsd-audit@freebsd.org Received: from pawn.primelocation.net (pawn.primelocation.net [205.161.238.235]) by hub.freebsd.org (Postfix) with ESMTP id 407F437BAB4; Mon, 31 Jul 2000 06:35:21 -0700 (PDT) (envelope-from jedgar@fxp.org) Received: by pawn.primelocation.net (Postfix, from userid 1003) id 7072A9B3E; Mon, 31 Jul 2000 09:35:19 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by pawn.primelocation.net (Postfix) with ESMTP id 656B8BA11; Mon, 31 Jul 2000 09:35:19 -0400 (EDT) Date: Mon, 31 Jul 2000 09:35:19 -0400 (EDT) From: "Chris D. Faulhaber" X-Sender: jedgar@pawn.primelocation.net To: Kris Kennaway Cc: audit@freebsd.org Subject: Re: Fuzz testing In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Mon, 31 Jul 2000, Kris Kennaway wrote: > Well, if anyone is looking for something to do, this came up on bugtraq: > > ftp://grilled.cs.wisc.edu/fuzz/ > > basically it's a set of tools which stuff random input into command-line > utils to look for bugs. There are probably a few easy targets here (well, > I know there are)..if you find one, check with OpenBSD to see if they've > already fixed it to save yourself some work. > A quick port of fuzz is available at: http://people.FreeBSD.org/~jedgar/fuzz.shar ----- Chris D. Faulhaber - jedgar@fxp.org - jedgar@FreeBSD.org -------------------------------------------------------- FreeBSD: The Power To Serve - http://www.FreeBSD.org To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Tue Aug 1 3:10:40 2000 Delivered-To: freebsd-audit@freebsd.org Received: from freefall.freebsd.org (freefall.FreeBSD.ORG [204.216.27.21]) by hub.freebsd.org (Postfix) with ESMTP id 75DDB37BE50; Tue, 1 Aug 2000 03:10:39 -0700 (PDT) (envelope-from kris@FreeBSD.org) Received: from localhost (kris@localhost) by freefall.freebsd.org (8.9.3/8.9.2) with ESMTP id DAA92386; Tue, 1 Aug 2000 03:10:39 -0700 (PDT) (envelope-from kris@FreeBSD.org) X-Authentication-Warning: freefall.freebsd.org: kris owned process doing -bs Date: Tue, 1 Aug 2000 03:10:39 -0700 (PDT) From: Kris Kennaway To: Kelly Yancey Cc: audit@freebsd.org Subject: Re: Update to patch(1) In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Sun, 2 Jul 2000, Kelly Yancey wrote: > > Can someone please review the patches in PR 19642. They merge in many > changes to patch(1) from OpenBSD. Specifically, they remove the standard > mktemp race condition as well as fix some potential buffer overflows. Sorry for the delay. Some comments: * be consistent about sizeof(foo) vs sizeof foo (choose whichever the surrounding file uses) * system() is insecure - there's no point in making all the string operations buffer-safe if you go and pass a user string to system() :-) * mkstemp() + close() isn't a drop-in replacement for mktemp() since it will leave tempfiles around if the program exits through an abnormal channel (error condition, signal, etc). mkstemp() + unlink() is usually okay if the program (or another program) doesn't need to reopen the same file, although it needs more source-code modification. Kris -- In God we Trust -- all others must submit an X.509 certificate. -- Charles Forsythe To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Wed Aug 2 11:55: 6 2000 Delivered-To: freebsd-audit@freebsd.org Received: from pawn.primelocation.net (pawn.primelocation.net [205.161.238.235]) by hub.freebsd.org (Postfix) with ESMTP id 412AE37B926; Wed, 2 Aug 2000 11:55:03 -0700 (PDT) (envelope-from jedgar@fxp.org) Received: from earth (oca-c1s5-31.mfi.net [209.26.94.216]) by pawn.primelocation.net (Postfix) with ESMTP id D96F39B1C; Wed, 2 Aug 2000 14:55:00 -0400 (EDT) Date: Wed, 2 Aug 2000 14:55:00 -0400 (EDT) From: "Chris D. Faulhaber" X-Sender: jedgar@earth.causticlabs.com To: Brian Fundakowski Feldman Cc: Kris Kennaway , freebsd-audit@freebsd.org Subject: fuzz usage (was: Re: cvs commit: ports/security/fuzz Makefile) In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG Moving to FreeBSD-Audit On Tue, 1 Aug 2000, Brian Fundakowski Feldman wrote: > On Tue, 1 Aug 2000, Kris Kennaway wrote: > > > See the preliminary list I posted to -audit the other day for some easy > > and not-so-easy candidates :-) > > Right :) For what it's worth, sed survives a few thousand fuzz runs. I > am using fuzz with kern.chroot_allow_non_suser enabled (don't use more > permissions for anything than necessary...), but I think I'll set up a > jail to run it in. Trusting running programs as root is hard, but even > harder is trusting them with untrusted input ;) > > I'm gonna see what bugs I can find with fuzz in the non-gnu stuff, of > course starting with your suggestions, and I'll post any specifics to > -audit. I encourage anyone else who's looking for some useful things > to do to join -audit, too! > Of course, beware of using fuzz on a machine with multiple users. Fuzz creates temp files in /tmp using the tested program's name and run number (e.g. make.9999, make.9998, etc). While it does clean up after itself, the program does no sanity checking for links, etc, and will gladly overwrite an existing file (or the other end of a sym link). ----- Chris D. Faulhaber - jedgar@fxp.org - jedgar@FreeBSD.org -------------------------------------------------------- FreeBSD: The Power To Serve - http://www.FreeBSD.org To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Wed Aug 2 15: 3:27 2000 Delivered-To: freebsd-audit@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 758) id BC84237B6B3; Wed, 2 Aug 2000 15:03:25 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by hub.freebsd.org (Postfix) with ESMTP id BA7592E819A; Wed, 2 Aug 2000 15:03:25 -0700 (PDT) (envelope-from kris@hub.freebsd.org) Date: Wed, 2 Aug 2000 15:03:25 -0700 (PDT) From: Kris Kennaway To: "Chris D. Faulhaber" Cc: Brian Fundakowski Feldman , freebsd-audit@freebsd.org Subject: Re: fuzz usage (was: Re: cvs commit: ports/security/fuzz Makefile) In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Wed, 2 Aug 2000, Chris D. Faulhaber wrote: > > I'm gonna see what bugs I can find with fuzz in the non-gnu stuff, of > > course starting with your suggestions, and I'll post any specifics to > > -audit. I encourage anyone else who's looking for some useful things > > to do to join -audit, too! > > > > Of course, beware of using fuzz on a machine with multiple users. Fuzz > creates temp files in /tmp using the tested program's name and run number > (e.g. make.9999, make.9998, etc). While it does clean up after itself, > the program does no sanity checking for links, etc, and will gladly > overwrite an existing file (or the other end of a sym link). In fact you should probably only ever run it in a jail in case you trash your machine :-) Kris -- In God we Trust -- all others must submit an X.509 certificate. -- Charles Forsythe To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 1:30:19 2000 Delivered-To: freebsd-audit@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 758) id 5700137BC0F; Fri, 4 Aug 2000 01:29:59 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by hub.freebsd.org (Postfix) with ESMTP id 8D9982E8197 for ; Fri, 4 Aug 2000 01:29:59 -0700 (PDT) (envelope-from kris@hub.freebsd.org) Date: Fri, 4 Aug 2000 01:29:59 -0700 (PDT) From: Kris Kennaway To: audit@freebsd.org Subject: catopen() patch Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG Can someone please review the following patch? Kris Index: msgcat.c =================================================================== RCS file: /home/ncvs/src/lib/libc/nls/msgcat.c,v retrieving revision 1.21 diff -u -r1.21 msgcat.c --- msgcat.c 2000/01/27 23:06:33 1.21 +++ msgcat.c 2000/08/04 08:20:36 @@ -91,8 +91,9 @@ __const char *catpath = NULL; char *nlspath; char *lang; - long len; char *base, *cptr, *pathP; + int spcleft; + long len; struct stat sbuf; if (!name || !*name) { @@ -129,13 +130,20 @@ *cptr = '\0'; for (pathP = path; *nlspath; ++nlspath) { if (*nlspath == '%') { + spcleft = sizeof(path) - (pathP - path); if (*(nlspath + 1) == 'L') { ++nlspath; - strcpy(pathP, lang); + if (strlcpy(pathP, lang, spcleft) >= spcleft) { + errno = ENAMETOOLONG; + return(NLERR); + } pathP += strlen(lang); } else if (*(nlspath + 1) == 'N') { ++nlspath; - strcpy(pathP, name); + if (strlcpy(pathP, name, spcleft) >= spcleft) { + errno = ENAMETOOLONG; + return(NLERR); + } pathP += strlen(name); } else *(pathP++) = *nlspath; } else *(pathP++) = *nlspath; -- In God we Trust -- all others must submit an X.509 certificate. -- Charles Forsythe To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 1:45:10 2000 Delivered-To: freebsd-audit@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 758) id CF5DF37B96E; Fri, 4 Aug 2000 01:45:08 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by hub.freebsd.org (Postfix) with ESMTP id B5D062E8195 for ; Fri, 4 Aug 2000 01:45:08 -0700 (PDT) (envelope-from kris@hub.freebsd.org) Date: Fri, 4 Aug 2000 01:45:08 -0700 (PDT) From: Kris Kennaway To: audit@freebsd.org Subject: libftpio patch Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG Comments? The only thing which seems to use this nowadays is sysinstall, but it still should be fixed. Kris Index: ftpio.c =================================================================== RCS file: /home/ncvs/src/lib/libftpio/ftpio.c,v retrieving revision 1.37 diff -u -r1.37 ftpio.c --- ftpio.c 2000/07/10 10:00:20 1.37 +++ ftpio.c 2000/08/04 08:43:05 @@ -61,7 +61,8 @@ static int ftp_login_session(FTP_t ftp, char *host, int af, char *user, char *passwd, int port, int verbose); static int ftp_file_op(FTP_t ftp, char *operation, char *file, FILE **fp, char *mode, off_t *seekto); static int ftp_close(FTP_t ftp); -static int get_url_info(char *url_in, char *host_ret, int *port_ret, char *name_ret); +static int get_url_info(char *url_in, char *host_ret, int host_size, int *port_ret, char *name_ret, + int name_size); static void ftp_timeout(int sig); static void ftp_set_timeout(void); static void ftp_clear_timeout(void); @@ -382,7 +383,7 @@ if (retcode) *retcode = 0; - if (get_url_info(url, host, &port, name) == SUCCESS) { + if (get_url_info(url, host, sizeof(host), &port, name, sizeof(name)) == SUCCESS) { if (fp && prev_host) { if (!strcmp(prev_host, host)) { /* Try to use cached connection */ @@ -446,7 +447,7 @@ fclose(fp); fp = NULL; } - if (get_url_info(url, host, &port, name) == SUCCESS) { + if (get_url_info(url, host, sizeof(host), &port, name, sizeof(name)) == SUCCESS) { fp = ftpLoginAf(host, af, user, passwd, port, 0, retcode); if (fp) { fp2 = ftpPut(fp, name); @@ -465,7 +466,7 @@ /* Internal workhorse function for dissecting URLs. Takes a URL as the first argument and returns the result of such disection in the host, user, passwd, port and name variables. */ static int -get_url_info(char *url_in, char *host_ret, int *port_ret, char *name_ret) +get_url_info(char *url_in, char *host_ret, int host_size, int *port_ret, char *name_ret, int name_size) { char *name, *host, *cp, url[BUFSIZ]; int port; @@ -489,9 +490,11 @@ if ((name = index(cp ? cp : host, '/')) != NULL) *(name++) = '\0'; if (host_ret) - strcpy(host_ret, host); + if (strlcpy(host_ret, host, host_size) >= host_size) + return FAILURE; if (name && name_ret) - strcpy(name_ret, name); + if (strlcpy(name_ret, name, name_size) >= host_size) + return FAILURE; return SUCCESS; } -- In God we Trust -- all others must submit an X.509 certificate. -- Charles Forsythe To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 2:17:14 2000 Delivered-To: freebsd-audit@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 758) id B93C237B62B; Fri, 4 Aug 2000 02:17:11 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by hub.freebsd.org (Postfix) with ESMTP id B70D92E8194 for ; Fri, 4 Aug 2000 02:17:11 -0700 (PDT) (envelope-from kris@hub.freebsd.org) Date: Fri, 4 Aug 2000 02:17:11 -0700 (PDT) From: Kris Kennaway To: audit@freebsd.org Subject: Re: libftpio patch In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG Here's a better patch: Kris Index: ftpio.c =================================================================== RCS file: /home/ncvs/src/lib/libftpio/ftpio.c,v retrieving revision 1.37 diff -u -r1.37 ftpio.c --- ftpio.c 2000/07/10 10:00:20 1.37 +++ ftpio.c 2000/08/04 09:13:42 @@ -61,7 +61,8 @@ static int ftp_login_session(FTP_t ftp, char *host, int af, char *user, char *passwd, int port, int verbose); static int ftp_file_op(FTP_t ftp, char *operation, char *file, FILE **fp, char *mode, off_t *seekto); static int ftp_close(FTP_t ftp); -static int get_url_info(char *url_in, char *host_ret, int *port_ret, char *name_ret); +static int get_url_info(char *url_in, char *host_ret, int host_size, int *port_ret, char *name_ret, + int name_size); static void ftp_timeout(int sig); static void ftp_set_timeout(void); static void ftp_clear_timeout(void); @@ -203,7 +204,7 @@ off_t size; check_passive(fp); - sprintf(p, "SIZE %s\r\n", name); + snprintf(p, sizeof(p), "SIZE %s\r\n", name); if (ftp->is_verbose) fprintf(stderr, "Sending %s", p); if (writes(ftp->fd_ctrl, p)) @@ -229,7 +230,7 @@ int i; check_passive(fp); - sprintf(p, "MDTM %s\r\n", name); + snprintf(p, sizeof(p), "MDTM %s\r\n", name); if (ftp->is_verbose) fprintf(stderr, "Sending %s", p); if (writes(ftp->fd_ctrl, p)) @@ -382,7 +383,7 @@ if (retcode) *retcode = 0; - if (get_url_info(url, host, &port, name) == SUCCESS) { + if (get_url_info(url, host, sizeof(host), &port, name, sizeof(name)) == SUCCESS) { if (fp && prev_host) { if (!strcmp(prev_host, host)) { /* Try to use cached connection */ @@ -446,7 +447,7 @@ fclose(fp); fp = NULL; } - if (get_url_info(url, host, &port, name) == SUCCESS) { + if (get_url_info(url, host, sizeof(host), &port, name, sizeof(name)) == SUCCESS) { fp = ftpLoginAf(host, af, user, passwd, port, 0, retcode); if (fp) { fp2 = ftpPut(fp, name); @@ -465,7 +466,7 @@ /* Internal workhorse function for dissecting URLs. Takes a URL as the first argument and returns the result of such disection in the host, user, passwd, port and name variables. */ static int -get_url_info(char *url_in, char *host_ret, int *port_ret, char *name_ret) +get_url_info(char *url_in, char *host_ret, int host_size, int *port_ret, char *name_ret, int name_size) { char *name, *host, *cp, url[BUFSIZ]; int port; @@ -475,7 +476,8 @@ if (strncmp("ftp://", url_in, 6) != 0) return FAILURE; /* We like to stomp a lot on the URL string in dissecting it, so copy it first */ - strncpy(url, url_in, BUFSIZ); + if (strlcpy(url, url_in, BUFSIZ) >= BUFSIZ) + return FAILURE; host = url + 6; if ((cp = index(host, ':')) != NULL) { *(cp++) = '\0'; @@ -489,9 +491,11 @@ if ((name = index(cp ? cp : host, '/')) != NULL) *(name++) = '\0'; if (host_ret) - strcpy(host_ret, host); + if (strlcpy(host_ret, host, host_size) >= host_size) + return FAILURE; if (name && name_ret) - strcpy(name_ret, name); + if (strlcpy(name_ret, name, name_size) >= host_size) + return FAILURE; return SUCCESS; } @@ -703,7 +707,7 @@ va_list ap; va_start(ap, fmt); - (void)vsnprintf(p, sizeof p, fmt, ap); + (void)vsnprintf(p, sizeof p - 3, fmt, ap); va_end(ap); if (ftp->con_state == init) -- In God we Trust -- all others must submit an X.509 certificate. -- Charles Forsythe To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 3: 8:54 2000 Delivered-To: freebsd-audit@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 758) id 951D937B9DE; Fri, 4 Aug 2000 03:08:45 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by hub.freebsd.org (Postfix) with ESMTP id 91F6B2E8194 for ; Fri, 4 Aug 2000 03:08:45 -0700 (PDT) (envelope-from kris@hub.freebsd.org) Date: Fri, 4 Aug 2000 03:08:45 -0700 (PDT) From: Kris Kennaway To: audit@freebsd.org Subject: ether_line() patch Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG This patch fixes a minor overflow possibility in ether_line() (not really a problem at all, it's basically academic) which uses sscanf and doesnt specify a string length. I've replaced ether_line() with the version from OpenBSD which doesn't use sscanf() at all. Reviews, please! Kris Index: ether_addr.c =================================================================== RCS file: /home/ncvs/src/lib/libc/net/ether_addr.c,v retrieving revision 1.11 diff -u -r1.11 ether_addr.c --- ether_addr.c 2000/07/18 22:44:09 1.11 +++ ether_addr.c 2000/08/04 10:04:35 @@ -39,14 +39,15 @@ */ -#include -#include #include -#include -#include +#include #include #include -#include +#include +#include +#include +#include +#include #ifdef YP #include #include @@ -57,6 +58,8 @@ #define _PATH_ETHERS "/etc/ethers" #endif +static char *_ether_aton __P((const char *, struct ether_addr *)); + /* * Parse a string of text containing an ethernet address and hostname * and separate it into its component parts. @@ -66,28 +69,39 @@ struct ether_addr *e; char *hostname; { - int i, o[6]; + char *p; + size_t n; - i = sscanf(l, "%x:%x:%x:%x:%x:%x %s", &o[0], &o[1], &o[2], - &o[3], &o[4], &o[5], - hostname); - if (i != 7) - return (i); - - for (i=0; i<6; i++) - e->octet[i] = o[i]; - return (0); + /* Parse "xx:xx:xx:xx:xx:xx" */ + if ((p = _ether_aton(l, e)) == NULL || (*p != ' ' && *p != '\t')) + goto bad; + + /* Now get the hostname */ + while (isspace(*p)) + p++; + if (*p == '\0') + goto bad; + n = strcspn(p, " \t\n"); + if (n >= MAXHOSTNAMELEN) + goto bad; + (void)strncpy(hostname, p, n); + hostname[n] = '\0'; + return (0); + +bad: + errno = EINVAL; + return (-1); } /* * Convert an ASCII representation of an ethernet address to * binary form. */ -struct ether_addr *ether_aton(a) +static char *_ether_aton(a, e) const char *a; + struct ether_addr *e; { int i; - static struct ether_addr o; unsigned int o0, o1, o2, o3, o4, o5; i = sscanf(a, "%x:%x:%x:%x:%x:%x", &o0, &o1, &o2, &o3, &o4, &o5); @@ -95,14 +109,22 @@ if (i != 6) return (NULL); - o.octet[0]=o0; - o.octet[1]=o1; - o.octet[2]=o2; - o.octet[3]=o3; - o.octet[4]=o4; - o.octet[5]=o5; + e->octet[0]=o0; + e->octet[1]=o1; + e->octet[2]=o2; + e->octet[3]=o3; + e->octet[4]=o4; + e->octet[5]=o5; + + return ((char *)e); +} + +struct ether_addr *ether_aton(s) + const char *s; +{ + static struct ether_addr n; - return ((struct ether_addr *)&o); + return (_ether_aton(s, &n) ? &n : NULL); } /* @@ -156,7 +178,7 @@ strlen(ether_a), &result, &resultlen)) { continue; } - strncpy(buf, result, resultlen); + strncpy(buf, result, resultlen - 1); buf[resultlen] = '\0'; free(result); } -- In God we Trust -- all others must submit an X.509 certificate. -- Charles Forsythe To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 12:27:22 2000 Delivered-To: freebsd-audit@freebsd.org Received: from rover.village.org (rover.village.org [204.144.255.49]) by hub.freebsd.org (Postfix) with ESMTP id D8CB637BB49; Fri, 4 Aug 2000 12:27:17 -0700 (PDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (harmony.village.org [10.0.0.6]) by rover.village.org (8.9.3/8.9.3) with ESMTP id NAA75057; Fri, 4 Aug 2000 13:27:15 -0600 (MDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (localhost.village.org [127.0.0.1]) by harmony.village.org (8.9.3/8.8.3) with ESMTP id NAA12389; Fri, 4 Aug 2000 13:27:08 -0600 (MDT) Message-Id: <200008041927.NAA12389@harmony.village.org> To: Kris Kennaway Subject: Re: ether_line() patch Cc: audit@FreeBSD.ORG In-reply-to: Your message of "Fri, 04 Aug 2000 03:08:45 PDT." References: Date: Fri, 04 Aug 2000 13:27:08 -0600 From: Warner Losh Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG In message Kris Kennaway writes: : @@ -156,7 +178,7 @@ : strlen(ether_a), &result, &resultlen)) { : continue; : } : - strncpy(buf, result, resultlen); : + strncpy(buf, result, resultlen - 1); : buf[resultlen] = '\0'; : free(result); : } : This change is wrong. The strcpy puts upto resultlen characters into buf, and then null terminates it at the resultlen + 1st character (counting from 1). The strncpy should therefore not have the -1. Or the line setting the buf[] = 0 should have it as well. Warner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 13:58:18 2000 Delivered-To: freebsd-audit@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 758) id EF6F937BB61; Fri, 4 Aug 2000 13:58:16 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by hub.freebsd.org (Postfix) with ESMTP id ECFA82E8196; Fri, 4 Aug 2000 13:58:16 -0700 (PDT) (envelope-from kris@hub.freebsd.org) Date: Fri, 4 Aug 2000 13:58:16 -0700 (PDT) From: Kris Kennaway To: Warner Losh Cc: audit@FreeBSD.ORG Subject: Re: ether_line() patch In-Reply-To: <200008041927.NAA12389@harmony.village.org> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Fri, 4 Aug 2000, Warner Losh wrote: > : - strncpy(buf, result, resultlen); > : + strncpy(buf, result, resultlen - 1); > : buf[resultlen] = '\0'; > : free(result); > : } > : > > This change is wrong. The strcpy puts upto resultlen characters into > buf, and then null terminates it at the resultlen + 1st character > (counting from 1). The strncpy should therefore not have the -1. Or > the line setting the buf[] = 0 should have it as well. strncpy does not null-terminate if strlen(result) == resultlen. In that case the buf[resultlen] character will be stomped by the NULL - it's a trivial change, but I think it's correct. Kris -- In God we Trust -- all others must submit an X.509 certificate. -- Charles Forsythe To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 14: 4:35 2000 Delivered-To: freebsd-audit@freebsd.org Received: from rover.village.org (rover.village.org [204.144.255.49]) by hub.freebsd.org (Postfix) with ESMTP id E92B737BA3B; Fri, 4 Aug 2000 14:04:27 -0700 (PDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (harmony.village.org [10.0.0.6]) by rover.village.org (8.9.3/8.9.3) with ESMTP id PAA75332; Fri, 4 Aug 2000 15:04:24 -0600 (MDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (localhost.village.org [127.0.0.1]) by harmony.village.org (8.9.3/8.8.3) with ESMTP id PAA12903; Fri, 4 Aug 2000 15:04:15 -0600 (MDT) Message-Id: <200008042104.PAA12903@harmony.village.org> To: Kris Kennaway Subject: Re: ether_line() patch Cc: audit@FreeBSD.ORG In-reply-to: Your message of "Fri, 04 Aug 2000 13:58:16 PDT." References: Date: Fri, 04 Aug 2000 15:04:15 -0600 From: Warner Losh Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG In message Kris Kennaway writes: : On Fri, 4 Aug 2000, Warner Losh wrote: : : > : - strncpy(buf, result, resultlen); : > : + strncpy(buf, result, resultlen - 1); : > : buf[resultlen] = '\0'; : > : free(result); : > : } : > : : > : > This change is wrong. The strcpy puts upto resultlen characters into : > buf, and then null terminates it at the resultlen + 1st character : > (counting from 1). The strncpy should therefore not have the -1. Or : > the line setting the buf[] = 0 should have it as well. : : strncpy does not null-terminate if strlen(result) == resultlen. In that : case the buf[resultlen] character will be stomped by the NULL - it's a : trivial change, but I think it's correct. No. The change isn't right. I know that strncpy doesn't NUL terminate when it copies N characters. That's not the point. The ponit is that you terminate at the resultlen+1ths character, so you should be strncopying resultlen characters, like it was doing before. In other words, the old code is correct. Looking more closely at the code, in the too long case, strncpy copies bytes to buf[0..resultlen-1]. the next line then NUL terminates at buf[resultlen]. Clearly, the last character isn't overwritten, and all the bytes are weel defined in the too long case. In the other case, strncpy is well defined. With your change, the strncpy would copy bytes to buf[0..resultlen-2], leaving buf[resultlen - 1] undefined, and buf[reultlen] = NUL at the end. That's why your change is wrong. Or at least inconsistant. It introduces the potential for a garbage byte. Warner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 14:10: 6 2000 Delivered-To: freebsd-audit@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 758) id D10DE37B69F; Fri, 4 Aug 2000 14:10:03 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by hub.freebsd.org (Postfix) with ESMTP id CEBEC2E8194; Fri, 4 Aug 2000 14:10:03 -0700 (PDT) (envelope-from kris@hub.freebsd.org) Date: Fri, 4 Aug 2000 14:10:03 -0700 (PDT) From: Kris Kennaway To: Warner Losh Cc: audit@FreeBSD.ORG Subject: Re: ether_line() patch In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Fri, 4 Aug 2000, Kris Kennaway wrote: > strncpy does not null-terminate if strlen(result) == resultlen. In that > case the buf[resultlen] character will be stomped by the NULL - it's a > trivial change, but I think it's correct. Actually we were both wrong - this strncpy was just bogus and did no bounds checking. This patch hunk should be better. @@ -156,8 +178,8 @@ strlen(ether_a), &result, &resultlen)) { continue; } - strncpy(buf, result, resultlen); - buf[resultlen] = '\0'; + strncpy(buf, result, sizeof(buf) - 1); + buf[sizeof(buf)] = '\0'; free(result); } #endif Kris -- In God we Trust -- all others must submit an X.509 certificate. -- Charles Forsythe To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 14:22:43 2000 Delivered-To: freebsd-audit@freebsd.org Received: from rover.village.org (rover.village.org [204.144.255.49]) by hub.freebsd.org (Postfix) with ESMTP id EB0B637B75B; Fri, 4 Aug 2000 14:22:37 -0700 (PDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (harmony.village.org [10.0.0.6]) by rover.village.org (8.9.3/8.9.3) with ESMTP id PAA75387; Fri, 4 Aug 2000 15:22:36 -0600 (MDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (localhost.village.org [127.0.0.1]) by harmony.village.org (8.9.3/8.8.3) with ESMTP id PAA13032; Fri, 4 Aug 2000 15:22:28 -0600 (MDT) Message-Id: <200008042122.PAA13032@harmony.village.org> To: Kris Kennaway Subject: Re: ether_line() patch Cc: audit@FreeBSD.ORG In-reply-to: Your message of "Fri, 04 Aug 2000 14:10:03 PDT." References: Date: Fri, 04 Aug 2000 15:22:28 -0600 From: Warner Losh Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG In message Kris Kennaway writes: : On Fri, 4 Aug 2000, Kris Kennaway wrote: : : > strncpy does not null-terminate if strlen(result) == resultlen. In that : > case the buf[resultlen] character will be stomped by the NULL - it's a : > trivial change, but I think it's correct. : : Actually we were both wrong - this strncpy was just bogus and did no : bounds checking. This patch hunk should be better. : : @@ -156,8 +178,8 @@ : strlen(ether_a), &result, &resultlen)) { : continue; : } : - strncpy(buf, result, resultlen); : - buf[resultlen] = '\0'; : + strncpy(buf, result, sizeof(buf) - 1); : + buf[sizeof(buf)] = '\0'; : free(result); : } : #endif Well, you weren't listening to me when I was wrong :-) At least about the parts I was right about :-). This is incorrect too. It should be buf[sizeof(buf) - 1] = '\0'; because the valid range of buf is [0..sizeof(buf) - 1]. You don't need the -1 on strncpy, but that's a style issue. The post conditions are identical with it or without it: strncpy(buf, result, sizeof(buf)); buf[sizeof(buf) - 1] = '\0'; The -1 might optimize the copying of one byte, but often it can cause more code to execute if the expression to the left of it isn't a compile time constant. Warner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 14:28: 1 2000 Delivered-To: freebsd-audit@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 758) id CCACD37B8C5; Fri, 4 Aug 2000 14:27:59 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by hub.freebsd.org (Postfix) with ESMTP id CA97E2E8196; Fri, 4 Aug 2000 14:27:59 -0700 (PDT) (envelope-from kris@hub.freebsd.org) Date: Fri, 4 Aug 2000 14:27:59 -0700 (PDT) From: Kris Kennaway To: Warner Losh Cc: audit@FreeBSD.ORG Subject: Re: ether_line() patch In-Reply-To: <200008042122.PAA13032@harmony.village.org> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Fri, 4 Aug 2000, Warner Losh wrote: > This is incorrect too. It should be buf[sizeof(buf) - 1] = '\0'; > because the valid range of buf is [0..sizeof(buf) - 1]. You don't > need the -1 on strncpy, but that's a style issue. The post conditions > are identical with it or without it: Oops again :) This change was just intended to be the above style issue/micro-optimization (until I noticed the real bug in the old code). In this case sizeof(buf) - 1 should still be correctly optimized by the compiler since it's a compile-time constant, right? Kris -- In God we Trust -- all others must submit an X.509 certificate. -- Charles Forsythe To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 14:33:28 2000 Delivered-To: freebsd-audit@freebsd.org Received: from rover.village.org (rover.village.org [204.144.255.49]) by hub.freebsd.org (Postfix) with ESMTP id EA3E137BA3B; Fri, 4 Aug 2000 14:33:16 -0700 (PDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (harmony.village.org [10.0.0.6]) by rover.village.org (8.9.3/8.9.3) with ESMTP id PAA75422; Fri, 4 Aug 2000 15:33:15 -0600 (MDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (localhost.village.org [127.0.0.1]) by harmony.village.org (8.9.3/8.8.3) with ESMTP id PAA13109; Fri, 4 Aug 2000 15:33:07 -0600 (MDT) Message-Id: <200008042133.PAA13109@harmony.village.org> To: Kris Kennaway Subject: Re: ether_line() patch Cc: audit@FreeBSD.ORG In-reply-to: Your message of "Fri, 04 Aug 2000 14:27:59 PDT." References: Date: Fri, 04 Aug 2000 15:33:07 -0600 From: Warner Losh Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG In message Kris Kennaway writes: : On Fri, 4 Aug 2000, Warner Losh wrote: : : > This is incorrect too. It should be buf[sizeof(buf) - 1] = '\0'; : > because the valid range of buf is [0..sizeof(buf) - 1]. You don't : > need the -1 on strncpy, but that's a style issue. The post conditions : > are identical with it or without it: : : Oops again :) : : This change was just intended to be the above style : issue/micro-optimization (until I noticed the real bug in the old code). : In this case sizeof(buf) - 1 should still be correctly optimized by the : compiler since it's a compile-time constant, right? Yes. Just pointing out that each optimization might have other, unintended effects. Usually, I do the following stylaistically: strncpy(dst, src, X); dst[X] = '\0'; where X is the size of the buffer minus 1. This goes to show how right Theo de Raadt was with his strlcpy API change :-). Getting this right is too tedious to be left to the user. Warner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 14:33:44 2000 Delivered-To: freebsd-audit@freebsd.org Received: from smtp1a.ispchannel.com (smtp.ispchannel.com [24.142.63.7]) by hub.freebsd.org (Postfix) with ESMTP id 2319537BDD8 for ; Fri, 4 Aug 2000 14:33:37 -0700 (PDT) (envelope-from mheffner@mailandnews.com) Received: from muriel.penguinpowered.com ([208.138.198.109]) by smtp1a.ispchannel.com (InterMail vK.4.02.00.00 201-232-116 license 7d3764cdaca754bf8ae20adf0db2aa60) with ESMTP id <20000804213521.THFZ8223.smtp1a@muriel.penguinpowered.com>; Fri, 4 Aug 2000 14:35:21 -0700 Content-Length: 2306 Message-ID: X-Mailer: XFMail 1.4.4 on FreeBSD X-Priority: 3 (Normal) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 8bit MIME-Version: 1.0 In-Reply-To: Date: Fri, 04 Aug 2000 17:31:54 -0400 (EDT) Reply-To: Mike Heffner From: Mike Heffner To: Kris Kennaway Subject: RE: catopen() patch Cc: audit@freebsd.org Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG My PR: misc/16954 is also another method, but I like the idea of setting errno. Also remember manpage would also need patching. On 04-Aug-2000 Kris Kennaway wrote: | Can someone please review the following patch? | | Kris | | Index: msgcat.c | =================================================================== | RCS file: /home/ncvs/src/lib/libc/nls/msgcat.c,v | retrieving revision 1.21 | diff -u -r1.21 msgcat.c | --- msgcat.c 2000/01/27 23:06:33 1.21 | +++ msgcat.c 2000/08/04 08:20:36 | @@ -91,8 +91,9 @@ | __const char *catpath = NULL; | char *nlspath; | char *lang; | - long len; | char *base, *cptr, *pathP; | + int spcleft; | + long len; | struct stat sbuf; | | if (!name || !*name) { | @@ -129,13 +130,20 @@ | *cptr = '\0'; | for (pathP = path; *nlspath; ++nlspath) { | if (*nlspath == '%') { | + spcleft = sizeof(path) - (pathP - path); | if (*(nlspath + 1) == 'L') { | ++nlspath; | - strcpy(pathP, lang); | + if (strlcpy(pathP, lang, spcleft) >= spcleft) { | + errno = ENAMETOOLONG; | + return(NLERR); | + } | pathP += strlen(lang); | } else if (*(nlspath + 1) == 'N') { | ++nlspath; | - strcpy(pathP, name); | + if (strlcpy(pathP, name, spcleft) >= spcleft) { | + errno = ENAMETOOLONG; | + return(NLERR); | + } | pathP += strlen(name); | } else *(pathP++) = *nlspath; | } else *(pathP++) = *nlspath; | | -- | In God we Trust -- all others must submit an X.509 certificate. | -- Charles Forsythe | | | | To Unsubscribe: send mail to majordomo@FreeBSD.org | with "unsubscribe freebsd-audit" in the body of the message -- Mike Heffner Fredericksburg, VA ICQ# 882073 http://my.ispchannel.com/~mheffner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 14:34:14 2000 Delivered-To: freebsd-audit@freebsd.org Received: from rover.village.org (rover.village.org [204.144.255.49]) by hub.freebsd.org (Postfix) with ESMTP id 0BBF537B96E for ; Fri, 4 Aug 2000 14:34:12 -0700 (PDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (harmony.village.org [10.0.0.6]) by rover.village.org (8.9.3/8.9.3) with ESMTP id PAA75426 for ; Fri, 4 Aug 2000 15:34:10 -0600 (MDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (localhost.village.org [127.0.0.1]) by harmony.village.org (8.9.3/8.8.3) with ESMTP id PAA13123 for ; Fri, 4 Aug 2000 15:34:03 -0600 (MDT) Message-Id: <200008042134.PAA13123@harmony.village.org> To: audit@freebsd.org Subject: strncpy bugs Date: Fri, 04 Aug 2000 15:34:03 -0600 From: Warner Losh Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG I wonder how many other strncpy bugs like this are in the tree. Warner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 18:47: 5 2000 Delivered-To: freebsd-audit@freebsd.org Received: from freefall.freebsd.org (freefall.FreeBSD.ORG [204.216.27.21]) by hub.freebsd.org (Postfix) with ESMTP id 2825E37B81C; Fri, 4 Aug 2000 18:47:04 -0700 (PDT) (envelope-from kris@FreeBSD.org) Received: from localhost (kris@localhost) by freefall.freebsd.org (8.9.3/8.9.2) with ESMTP id SAA54322; Fri, 4 Aug 2000 18:47:04 -0700 (PDT) (envelope-from kris@FreeBSD.org) X-Authentication-Warning: freefall.freebsd.org: kris owned process doing -bs Date: Fri, 4 Aug 2000 18:47:04 -0700 (PDT) From: Kris Kennaway To: Warner Losh Cc: audit@freebsd.org Subject: Re: strncpy bugs In-Reply-To: <200008042134.PAA13123@harmony.village.org> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Fri, 4 Aug 2000, Warner Losh wrote: > I wonder how many other strncpy bugs like this are in the tree. At least a few, I'd guess. I also noticed fencepost errors in some of the OpenBSD fixes a while back, although I dont have a record of these and promised to submit them when I came across them again. Kris -- In God we Trust -- all others must submit an X.509 certificate. -- Charles Forsythe To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 19:48:43 2000 Delivered-To: freebsd-audit@freebsd.org Received: from mail.rpi.edu (mail.rpi.edu [128.113.100.7]) by hub.freebsd.org (Postfix) with ESMTP id 0EA1B37B6BB; Fri, 4 Aug 2000 19:48:39 -0700 (PDT) (envelope-from drosih@rpi.edu) Received: from [128.113.24.47] (gilead.acs.rpi.edu [128.113.24.47]) by mail.rpi.edu (8.9.3/8.9.3) with ESMTP id WAA169824; Fri, 4 Aug 2000 22:48:32 -0400 Mime-Version: 1.0 X-Sender: drosih@mail.rpi.edu Message-Id: In-Reply-To: <200008042133.PAA13109@harmony.village.org> References: <200008042133.PAA13109@harmony.village.org> Date: Fri, 4 Aug 2000 22:48:49 -0400 To: Warner Losh , Kris Kennaway From: Garance A Drosihn Subject: Re: strncpy bugs Cc: audit@FreeBSD.ORG Content-Type: text/plain; charset="us-ascii" ; format="flowed" Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG At 3:33 PM -0600 8/4/00, Warner Losh wrote: > >Usually, I do the following stylaistically: > strncpy(dst, src, X); > dst[X] = '\0'; >where X is the size of the buffer minus 1. > >This goes to show how right Theo de Raadt was with his strlcpy >API change :-). Getting this right is too tedious to be left >to the user. So, should we change our habits, and use strlcpy fixes instead of repeatedly rewriting strncpy fixes? --- Garance Alistair Drosehn = gad@eclipse.acs.rpi.edu Senior Systems Programmer or drosih@rpi.edu Rensselaer Polytechnic Institute To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 20:28:17 2000 Delivered-To: freebsd-audit@freebsd.org Received: from freefall.freebsd.org (freefall.FreeBSD.ORG [204.216.27.21]) by hub.freebsd.org (Postfix) with ESMTP id EFE0037B8CD; Fri, 4 Aug 2000 20:28:14 -0700 (PDT) (envelope-from kris@FreeBSD.org) Received: from localhost (kris@localhost) by freefall.freebsd.org (8.9.3/8.9.2) with ESMTP id UAA63098; Fri, 4 Aug 2000 20:28:14 -0700 (PDT) (envelope-from kris@FreeBSD.org) X-Authentication-Warning: freefall.freebsd.org: kris owned process doing -bs Date: Fri, 4 Aug 2000 20:28:14 -0700 (PDT) From: Kris Kennaway To: Garance A Drosihn Cc: Warner Losh , Kris Kennaway , audit@FreeBSD.ORG Subject: Re: strncpy bugs In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Fri, 4 Aug 2000, Garance A Drosihn wrote: > At 3:33 PM -0600 8/4/00, Warner Losh wrote: > > > >Usually, I do the following stylaistically: > > strncpy(dst, src, X); > > dst[X] = '\0'; > >where X is the size of the buffer minus 1. > > > >This goes to show how right Theo de Raadt was with his strlcpy > >API change :-). Getting this right is too tedious to be left > >to the user. > > So, should we change our habits, and use strlcpy fixes instead > of repeatedly rewriting strncpy fixes? strlcpy is definitely easier to use correctly. However, it's probably not worthwhile replacing correct strncpy usage in most instances. Kris -- In God we Trust -- all others must submit an X.509 certificate. -- Charles Forsythe To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 21: 3:56 2000 Delivered-To: freebsd-audit@freebsd.org Received: from freefall.freebsd.org (freefall.FreeBSD.ORG [204.216.27.21]) by hub.freebsd.org (Postfix) with ESMTP id 33C1637B8E7 for ; Fri, 4 Aug 2000 21:03:51 -0700 (PDT) (envelope-from kris@FreeBSD.org) Received: from localhost (kris@localhost) by freefall.freebsd.org (8.9.3/8.9.2) with ESMTP id VAA65322 for ; Fri, 4 Aug 2000 21:03:50 -0700 (PDT) (envelope-from kris@FreeBSD.org) X-Authentication-Warning: freefall.freebsd.org: kris owned process doing -bs Date: Fri, 4 Aug 2000 21:03:50 -0700 (PDT) From: Kris Kennaway To: audit@freebsd.org Subject: ftp(1) patch Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG Bunch of overflows here..I'm slightly unsure of the macro expansion one - can everyone please check it? Incidentally, there was a case of strncpy() abuse here (using strlen as the third argument) Kris Index: cmds.c =================================================================== RCS file: /home/ncvs/src/usr.bin/ftp/cmds.c,v retrieving revision 1.18 diff -u -r1.18 cmds.c --- cmds.c 2000/06/24 15:34:30 1.18 +++ cmds.c 2000/08/05 03:52:38 @@ -125,7 +125,7 @@ else comret = command("TYPE %s", p->t_mode); if (comret == COMPLETE) { - (void)strcpy(typename, p->t_name); + (void)strlcpy(typename, p->t_name, sizeof(typename)); curtype = type = p->t_type; } } Index: complete.c =================================================================== RCS file: /home/ncvs/src/usr.bin/ftp/complete.c,v retrieving revision 1.5 diff -u -r1.5 complete.c --- complete.c 1999/08/28 01:01:30 1.5 +++ complete.c 2000/08/05 03:34:46 @@ -92,7 +92,7 @@ return (CC_ERROR); /* no choices available */ if (words->sl_cur == 1) { /* only once choice available */ - (void)strcpy(insertstr, words->sl_str[0]); + (void)strlcpy(insertstr, words->sl_str[0], sizeof(insertstr)); if (el_insertstr(el, insertstr + wordlen) == -1) return (CC_ERROR); else @@ -276,7 +276,7 @@ printf("\n%s\n", emesg); return (CC_REDISPLAY); } - (void)strcpy(lastdir, dir); + (void)strlcpy(lastdir, dir, sizeof(lastdir)); dirchange = 0; } Index: domacro.c =================================================================== RCS file: /home/ncvs/src/usr.bin/ftp/domacro.c,v retrieving revision 1.6 diff -u -r1.6 domacro.c --- domacro.c 1999/08/28 01:01:31 1.6 +++ domacro.c 2000/08/05 03:49:13 @@ -56,7 +56,7 @@ int argc; char *argv[]; { - int i, j, count = 2, loopflg = 0; + int i, j, count = 2, linefull = 0, loopflg = 0; char *cp1, *cp2, line2[200]; struct cmd *c; @@ -75,7 +75,7 @@ code = -1; return; } - (void)strcpy(line2, line); + (void)strlcpy(line2, line, sizeof(line2)); TOP: cp1 = macros[i].mac_start; while (cp1 != macros[i].mac_end) { @@ -83,7 +83,7 @@ cp1++; } cp2 = line; - while (*cp1 != '\0') { + while (*cp1 != '\0' && !linefull) { switch(*cp1) { case '\\': *cp2++ = *++cp1; @@ -96,7 +96,12 @@ } cp1--; if (argc - 2 >= j) { - (void)strcpy(cp2, argv[j+1]); + if (strlcpy(cp2, argv[j+1], + sizeof(line) - (cp2 - line)) >= + sizeof(line) - (cp2 - line)) { + linefull = 1; + break; + } cp2 += strlen(argv[j+1]); } break; @@ -105,8 +110,13 @@ loopflg = 1; cp1++; if (count < argc) { - (void)strcpy(cp2, argv[count]); - cp2 += strlen(argv[count]); + if (strlcpy(cp2, argv[count], + sizeof(line) - (cp2 - line)) >= + sizeof(line) - (cp2 - line)) { + linefull = 1; + break; + } + cp2 += strlen(argv[count]); } break; } @@ -141,7 +151,7 @@ if (bell && c->c_bell) { (void)putchar('\007'); } - (void)strcpy(line, line2); + (void)strlcpy(line, line2, sizeof(line)); makeargv(); argc = margc; argv = margv; Index: fetch.c =================================================================== RCS file: /home/ncvs/src/usr.bin/ftp/fetch.c,v retrieving revision 1.14 diff -u -r1.14 fetch.c --- fetch.c 2000/06/24 15:34:31 1.14 +++ fetch.c 2000/08/05 03:50:21 @@ -598,7 +598,7 @@ if (strcmp(host, lasthost) != 0) { int oautologin; - (void)strcpy(lasthost, host); + (void)strlcpy(lasthost, host, sizeof(lasthost)); if (connected) disconnect(0, NULL); xargv[0] = __progname; Index: ftp.c =================================================================== RCS file: /home/ncvs/src/usr.bin/ftp/ftp.c,v retrieving revision 1.31 diff -u -r1.31 ftp.c --- ftp.c 2000/06/24 15:34:31 1.31 +++ ftp.c 2000/08/05 03:30:36 @@ -112,11 +112,9 @@ char *host; if (*host0 == '[' && strrchr(host0, ']') != NULL) { /*IPv6 addr in []*/ - strncpy(hostnamebuf, host0 + 1, strlen(host0) - 2); - hostnamebuf[strlen(host0) - 2] = '\0'; + strlcpy(hostnamebuf, host0 + 1, MIN(strlen(host0) - 1, sizeof(hostnamebuf))); } else { - strncpy(hostnamebuf, host0, strlen(host0)); - hostnamebuf[strlen(host0)] = '\0'; + strlcpy(hostnamebuf, host0, MIN(strlen(host0) + 1, sizeof(hostnamebuf))); } host = hostnamebuf; memset(&hints, 0, sizeof(hints)); @@ -135,7 +133,7 @@ res = res0; if (res->ai_canonname) - (void) strncpy(hostnamebuf, res->ai_canonname, + (void) strlcpy(hostnamebuf, res->ai_canonname, sizeof(hostnamebuf)); hostname = hostnamebuf; while (1) { @@ -440,8 +438,7 @@ if (len > sizeof(reply_string)) len = sizeof(reply_string); - (void)strncpy(reply_string, current_line, len); - reply_string[len] = '\0'; + (void)strlcpy(reply_string, current_line, len); } if (continuation && code != originalcode) { if (originalcode == 0) @@ -1658,20 +1655,16 @@ mcase = op->mcse; ip->ntflg = ntflag; ntflag = op->ntflg; - (void)strncpy(ip->nti, ntin, sizeof(ip->nti) - 1); - (ip->nti)[sizeof(ip->nti) - 1] = '\0'; - (void)strcpy(ntin, op->nti); - (void)strncpy(ip->nto, ntout, sizeof(ip->nto) - 1); - (ip->nto)[sizeof(ip->nto) - 1] = '\0'; - (void)strcpy(ntout, op->nto); + (void)strlcpy(ip->nti, ntin, sizeof(ip->nti)); + (void)strlcpy(ntin, op->nti, sizeof(ntin)); + (void)strlcpy(ip->nto, ntout, sizeof(ip->nto)); + (void)strlcpy(ntout, op->nto, sizeof(ntout)); ip->mapflg = mapflag; mapflag = op->mapflg; - (void)strncpy(ip->mi, mapin, sizeof(ip->mi) - 1); - (ip->mi)[sizeof(ip->mi) - 1] = '\0'; - (void)strcpy(mapin, op->mi); - (void)strncpy(ip->mo, mapout, sizeof(ip->mo) - 1); - (ip->mo)[sizeof(ip->mo) - 1] = '\0'; - (void)strcpy(mapout, op->mo); + (void)strlcpy(ip->mi, mapin, sizeof(ip->mi)); + (void)strlcpy(mapin, op->mi, sizeof(mapin)); + (void)strlcpy(ip->mo, mapout, sizeof(ip->mo)); + (void)strlcpy(mapout, op->mo, sizeof(mapout)); (void)signal(SIGINT, oldintr); if (abrtflag) { abrtflag = 0; @@ -1862,14 +1855,11 @@ warn("local: %s", local); return ((char *) 0); } - (void)strcpy(new, local); + (void)strlcpy(new, local, sizeof(new)); cp = new + strlen(new); - *cp++ = '.'; - while (!d) { - if (++count == 100) { - puts("runique: can't find unique file name."); - return ((char *) 0); - } + if (cp - new < sizeof(new)) + *cp++ = '.'; + while (!d && ++count < 100 && cp - new < sizeof(new)) { *cp++ = ext; *cp = '\0'; if (ext == '9') @@ -1886,6 +1876,10 @@ *(cp - 2) = *(cp - 2) + 1; cp--; } + } + if (count == 100 || cp - new >= sizeof(new)) { + puts("runique: can't find unique file name."); + return ((char *) 0); } return (new); } Index: main.c =================================================================== RCS file: /home/ncvs/src/usr.bin/ftp/main.c,v retrieving revision 1.28 diff -u -r1.28 main.c --- main.c 2000/06/24 15:34:31 1.28 +++ main.c 2000/08/05 03:57:59 @@ -258,7 +258,7 @@ pw = getpwuid(getuid()); if (pw != NULL) { home = homedir; - (void)strcpy(home, pw->pw_dir); + (void)strlcpy(home, pw->pw_dir, sizeof(homedir)); } setttywidth(0); -- In God we Trust -- all others must submit an X.509 certificate. -- Charles Forsythe To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 21:40:45 2000 Delivered-To: freebsd-audit@freebsd.org Received: from rover.village.org (rover.village.org [204.144.255.49]) by hub.freebsd.org (Postfix) with ESMTP id C653D37B939; Fri, 4 Aug 2000 21:40:41 -0700 (PDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (harmony.village.org [10.0.0.6]) by rover.village.org (8.9.3/8.9.3) with ESMTP id WAA76304; Fri, 4 Aug 2000 22:40:39 -0600 (MDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (localhost.village.org [127.0.0.1]) by harmony.village.org (8.9.3/8.8.3) with ESMTP id WAA14655; Fri, 4 Aug 2000 22:40:31 -0600 (MDT) Message-Id: <200008050440.WAA14655@harmony.village.org> To: Kris Kennaway Subject: Re: strncpy bugs Cc: audit@FreeBSD.org In-reply-to: Your message of "Fri, 04 Aug 2000 18:47:04 PDT." References: Date: Fri, 04 Aug 2000 22:40:31 -0600 From: Warner Losh Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG In message Kris Kennaway writes: : At least a few, I'd guess. I also noticed fencepost errors in some of the : OpenBSD fixes a while back, although I dont have a record of these and : promised to submit them when I came across them again. Most of the ones that I saw didn't have fencepost errors, although people thought they might. They were of the form strncpy(foo, bar, sizeof(foo)); foo[sizeof(foo) - 1] = '\0'; which some people will whine about saying that the last parameter should be sizeof(foo) - 1, but the above and the following have exactly the same post conditions: strncpy(foo, bar, sizeof(foo) - 1); foo[sizeof(foo) - 1] = '\0'; Warner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 21:40:59 2000 Delivered-To: freebsd-audit@freebsd.org Received: from rover.village.org (rover.village.org [204.144.255.49]) by hub.freebsd.org (Postfix) with ESMTP id D12FA37B939; Fri, 4 Aug 2000 21:40:56 -0700 (PDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (harmony.village.org [10.0.0.6]) by rover.village.org (8.9.3/8.9.3) with ESMTP id WAA76308; Fri, 4 Aug 2000 22:40:55 -0600 (MDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (localhost.village.org [127.0.0.1]) by harmony.village.org (8.9.3/8.8.3) with ESMTP id WAA14668; Fri, 4 Aug 2000 22:40:46 -0600 (MDT) Message-Id: <200008050440.WAA14668@harmony.village.org> To: Garance A Drosihn Subject: Re: strncpy bugs Cc: Kris Kennaway , audit@FreeBSD.ORG In-reply-to: Your message of "Fri, 04 Aug 2000 22:48:49 EDT." References: <200008042133.PAA13109@harmony.village.org> Date: Fri, 04 Aug 2000 22:40:46 -0600 From: Warner Losh Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG In message Garance A Drosihn writes: : So, should we change our habits, and use strlcpy fixes instead : of repeatedly rewriting strncpy fixes? Actaully, yes. Warner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Fri Aug 4 21:41:51 2000 Delivered-To: freebsd-audit@freebsd.org Received: from rover.village.org (rover.village.org [204.144.255.49]) by hub.freebsd.org (Postfix) with ESMTP id C22C537B793; Fri, 4 Aug 2000 21:41:47 -0700 (PDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (harmony.village.org [10.0.0.6]) by rover.village.org (8.9.3/8.9.3) with ESMTP id WAA76318; Fri, 4 Aug 2000 22:41:46 -0600 (MDT) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (localhost.village.org [127.0.0.1]) by harmony.village.org (8.9.3/8.8.3) with ESMTP id WAA14686; Fri, 4 Aug 2000 22:41:37 -0600 (MDT) Message-Id: <200008050441.WAA14686@harmony.village.org> To: Kris Kennaway Subject: Re: strncpy bugs Cc: Garance A Drosihn , Kris Kennaway , audit@FreeBSD.ORG In-reply-to: Your message of "Fri, 04 Aug 2000 20:28:14 PDT." References: Date: Fri, 04 Aug 2000 22:41:37 -0600 From: Warner Losh Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG In message Kris Kennaway writes: : strlcpy is definitely easier to use correctly. However, it's probably not : worthwhile replacing correct strncpy usage in most instances. Long term it likely is. Short term, or doing it in a rush will lead to either more errors than before, or to propigation of errors that are in the existing code. Warner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Sat Aug 5 15:29:37 2000 Delivered-To: freebsd-audit@freebsd.org Received: from smtp1a.ispchannel.com (smtp.ispchannel.com [24.142.63.7]) by hub.freebsd.org (Postfix) with ESMTP id 4B2E037BB70 for ; Sat, 5 Aug 2000 15:29:35 -0700 (PDT) (envelope-from mheffner@mailandnews.com) Received: from muriel.penguinpowered.com ([208.138.198.109]) by smtp1a.ispchannel.com (InterMail vK.4.02.00.00 201-232-116 license 7d3764cdaca754bf8ae20adf0db2aa60) with ESMTP id <20000805223120.WGCQ8223.smtp1a@muriel.penguinpowered.com>; Sat, 5 Aug 2000 15:31:20 -0700 Content-Length: 752 Message-ID: X-Mailer: XFMail 1.4.4 on FreeBSD X-Priority: 3 (Normal) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 8bit MIME-Version: 1.0 In-Reply-To: Date: Sat, 05 Aug 2000 18:28:05 -0400 (EDT) Reply-To: Mike Heffner From: Mike Heffner To: Kris Kennaway Subject: RE: catopen() patch Cc: audit@freebsd.org Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On 04-Aug-2000 Kris Kennaway wrote: | Can someone please review the following patch? | ... | ++nlspath; | - strcpy(pathP, name); | + if (strlcpy(pathP, name, spcleft) >= spcleft) { | + errno = ENAMETOOLONG; | + return(NLERR); | + } | pathP += strlen(name); | } else *(pathP++) = *nlspath; | } else *(pathP++) = *nlspath; ^^^^^^^^^^^^^^^^^^^^^ We can still walk right off the end. -- Mike Heffner Fredericksburg, VA ICQ# 882073 http://my.ispchannel.com/~mheffner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message From owner-freebsd-audit Sat Aug 5 19: 4:21 2000 Delivered-To: freebsd-audit@freebsd.org Received: from smtp1a.ispchannel.com (smtp.ispchannel.com [24.142.63.7]) by hub.freebsd.org (Postfix) with ESMTP id D888437B7C4 for ; Sat, 5 Aug 2000 19:04:15 -0700 (PDT) (envelope-from mheffner@mailandnews.com) Received: from muriel.penguinpowered.com ([208.138.198.109]) by smtp1a.ispchannel.com (InterMail vK.4.02.00.00 201-232-116 license 7d3764cdaca754bf8ae20adf0db2aa60) with ESMTP id <20000806020601.WOYW8223.smtp1a@muriel.penguinpowered.com>; Sat, 5 Aug 2000 19:06:01 -0700 Content-Length: 800 Message-ID: X-Mailer: XFMail 1.4.4 on FreeBSD X-Priority: 3 (Normal) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 8bit MIME-Version: 1.0 In-Reply-To: Date: Sat, 05 Aug 2000 22:02:48 -0400 (EDT) Reply-To: Mike Heffner From: Mike Heffner To: Kris Kennaway Subject: Re: libftpio patch Cc: audit@freebsd.org Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On 04-Aug-2000 Kris Kennaway wrote: | if (strncmp("ftp://", url_in, 6) != 0) | return FAILURE; | /* We like to stomp a lot on the URL string in dissecting it, so copy | it first */ | - strncpy(url, url_in, BUFSIZ); | + if (strlcpy(url, url_in, BUFSIZ) >= BUFSIZ) | + return FAILURE; Just to be nit-picky, should this be sizeof(url) to remain consistent with your other changes? | @@ -703,7 +707,7 @@ | | va_list ap; | va_start(ap, fmt); | - (void)vsnprintf(p, sizeof p, fmt, ap); | + (void)vsnprintf(p, sizeof p - 3, fmt, ap); Assuming the "\r\n" is all you strcat() on to p, this only needs to be - 2 -- Mike Heffner Fredericksburg, VA ICQ# 882073 http://my.ispchannel.com/~mheffner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message