From owner-freebsd-audit Fri Mar 15 23:58: 7 2002 Delivered-To: freebsd-audit@freebsd.org Received: from rwcrmhc52.attbi.com (rwcrmhc52.attbi.com [216.148.227.88]) by hub.freebsd.org (Postfix) with ESMTP id 70CB337B41B for ; Fri, 15 Mar 2002 23:57:59 -0800 (PST) Received: from blossom.cjclark.org ([12.234.91.48]) by rwcrmhc52.attbi.com (InterMail vM.4.01.03.27 201-229-121-127-20010626) with ESMTP id <20020316075758.BRAG1147.rwcrmhc52.attbi.com@blossom.cjclark.org>; Sat, 16 Mar 2002 07:57:58 +0000 Received: (from cjc@localhost) by blossom.cjclark.org (8.11.6/8.11.6) id g2G7vwj46574; Fri, 15 Mar 2002 23:57:58 -0800 (PST) (envelope-from cjc) Date: Fri, 15 Mar 2002 23:57:58 -0800 From: "Crist J. Clark" To: Dag-Erling Smorgrav Cc: audit@FreeBSD.ORG Subject: Re: Preventing chpass(1) DoS Message-ID: <20020315235758.N29705@blossom.cjclark.org> References: <20020315042007.K29705@blossom.cjclark.org> <20020315190204.L29705@blossom.cjclark.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: <20020315190204.L29705@blossom.cjclark.org>; from crist.clark@attbi.com on Fri, Mar 15, 2002 at 07:02:04PM -0800 X-URL: http://people.freebsd.org/~cjc/ 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 On Fri, Mar 15, 2002 at 07:02:04PM -0800, Crist J. Clark wrote: > On Fri, Mar 15, 2002 at 03:29:52PM +0100, Dag-Erling Smorgrav wrote: > > "Crist J. Clark" writes: > > > I should note that a cursory look at NetBSD and OpenBSD shows they > > > both do not lock while the user is editing. > > > > How about using their code rather than roll our own? > > *shrug* Bigger diff. They also have pw_scan() exposed to the world in > libc. We don't. But instead of using pw_scan() here's an approach that > does the comparison "backwards" from how they do it. But other than > that, it's a pretty straighforward matter to use the same approach. > > Anyone see any issues with this? [snip] Well, since no one responded, I guess no one is really checking the patches. I somehow sent patches that were from halfway through the complete changes. _These_ are the real ones, Index: usr.bin/chpass/chpass.c =================================================================== RCS file: /export/freebsd/ncvs/src/usr.bin/chpass/chpass.c,v retrieving revision 1.18 diff -u -r1.18 chpass.c --- usr.bin/chpass/chpass.c 26 Jul 2001 23:27:10 -0000 1.18 +++ usr.bin/chpass/chpass.c 16 Mar 2002 02:51:11 -0000 @@ -83,7 +83,7 @@ char **argv; { enum { NEWSH, LOADENTRY, EDITENTRY, NEWPW, NEWEXP } op; - struct passwd *pw = NULL, lpw; + struct passwd *pw = NULL, lpw, old_pw; char *username = NULL; int ch, pfd, tfd; char *arg = NULL; @@ -160,7 +160,7 @@ uid = getuid(); - if (op == EDITENTRY || op == NEWSH || op == NEWPW || op == NEWEXP) + if (op == EDITENTRY || op == NEWSH || op == NEWPW || op == NEWEXP) { switch(argc) { #ifdef YP case 0: @@ -186,6 +186,12 @@ default: usage(); } + + /* Make a copy for later verification */ + old_pw = *pw; + old_pw.pw_gecos = strdup(old_pw.pw_gecos); + } + if (op == NEWSH) { /* protect p_shell -- it thinks NULL is /bin/sh */ if (!arg[0]) @@ -246,7 +252,6 @@ * The exit closes the master passwd fp/fd. */ pw_init(); - pfd = pw_lock(); tfd = pw_tmp(); if (op == EDITENTRY) { @@ -262,7 +267,8 @@ (void)unlink(tempname); } else { #endif /* YP */ - pw_copy(pfd, tfd, pw); + pfd = pw_lock(); + pw_copy(pfd, tfd, pw, (op == LOADENTRY) ? NULL : &old_pw); if (!pw_mkdb(username)) pw_error((char *)NULL, 0, 1); Index: usr.bin/chpass/pw_copy.c =================================================================== RCS file: /export/freebsd/ncvs/src/usr.bin/chpass/pw_copy.c,v retrieving revision 1.10 diff -u -r1.10 pw_copy.c --- usr.bin/chpass/pw_copy.c 26 Jul 2001 23:27:10 -0000 1.10 +++ usr.bin/chpass/pw_copy.c 16 Mar 2002 05:47:30 -0000 @@ -51,26 +51,44 @@ #include #include "pw_copy.h" +#define PWLINE_MAX 8192 +#define PWFIELD_MAX 20 + extern char *tempname; -void -pw_copy(ffd, tfd, pw) - int ffd, tfd; - struct passwd *pw; +static char * +pw_fmt(char *buf, size_t size, struct passwd *pw) { - FILE *from, *to; - int done; - char *p, buf[8192]; - char uidstr[20]; - char gidstr[20]; - char chgstr[20]; - char expstr[20]; + char uidstr[PWFIELD_MAX]; + char gidstr[PWFIELD_MAX]; + char chgstr[PWFIELD_MAX]; + char expstr[PWFIELD_MAX]; snprintf(uidstr, sizeof(uidstr), "%lu", (unsigned long)pw->pw_uid); snprintf(gidstr, sizeof(gidstr), "%lu", (unsigned long)pw->pw_gid); snprintf(chgstr, sizeof(chgstr), "%ld", (long)pw->pw_change); snprintf(expstr, sizeof(expstr), "%ld", (long)pw->pw_expire); + (void)snprintf(buf, size, "%s:%s:%s:%s:%s:%s:%s:%s:%s:%s\n", + pw->pw_name, pw->pw_passwd, + pw->pw_fields & _PWF_UID ? uidstr : "", + pw->pw_fields & _PWF_GID ? gidstr : "", + pw->pw_class, + pw->pw_fields & _PWF_CHANGE ? chgstr : "", + pw->pw_fields & _PWF_EXPIRE ? expstr : "", + pw->pw_gecos, pw->pw_dir, pw->pw_shell); + return buf; +} + +void +pw_copy(ffd, tfd, pw, old_pw) + int ffd, tfd; + struct passwd *pw, *old_pw; +{ + FILE *from, *to; + int done; + char *p, buf[PWLINE_MAX], old_buf[PWLINE_MAX]; + if (!(from = fdopen(ffd, "r"))) pw_error(_PATH_MASTERPASSWD, 1, 1); if (!(to = fdopen(tfd, "w"))) @@ -108,14 +126,13 @@ goto err; continue; } - (void)fprintf(to, "%s:%s:%s:%s:%s:%s:%s:%s:%s:%s\n", - pw->pw_name, pw->pw_passwd, - pw->pw_fields & _PWF_UID ? uidstr : "", - pw->pw_fields & _PWF_GID ? gidstr : "", - pw->pw_class, - pw->pw_fields & _PWF_CHANGE ? chgstr : "", - pw->pw_fields & _PWF_EXPIRE ? expstr : "", - pw->pw_gecos, pw->pw_dir, pw->pw_shell); + *p = ':'; + if (old_pw != NULL && + strcmp(buf, pw_fmt(old_buf, sizeof(old_buf), old_pw)) != 0) { + warnx("%s: entry inconsistent", _PATH_MASTERPASSWD); + pw_error(NULL, 0, 1); + } + (void)fprintf(to, "%s", pw_fmt(buf, sizeof(buf), pw)); done = 1; if (ferror(to)) goto err; @@ -129,14 +146,7 @@ pw_error(NULL, 0, 1); } else #endif /* YP */ - (void)fprintf(to, "%s:%s:%s:%s:%s:%s:%s:%s:%s:%s\n", - pw->pw_name, pw->pw_passwd, - pw->pw_fields & _PWF_UID ? uidstr : "", - pw->pw_fields & _PWF_GID ? gidstr : "", - pw->pw_class, - pw->pw_fields & _PWF_CHANGE ? chgstr : "", - pw->pw_fields & _PWF_EXPIRE ? expstr : "", - pw->pw_gecos, pw->pw_dir, pw->pw_shell); + (void)fprintf(to, "%s", pw_fmt(buf, sizeof(buf), pw)); } if (ferror(to)) Index: usr.bin/chpass/pw_copy.h =================================================================== RCS file: /export/freebsd/ncvs/src/usr.bin/chpass/pw_copy.h,v retrieving revision 1.1.1.1 diff -u -r1.1.1.1 pw_copy.h --- usr.bin/chpass/pw_copy.h 27 May 1994 12:30:55 -0000 1.1.1.1 +++ usr.bin/chpass/pw_copy.h 16 Mar 2002 02:00:19 -0000 @@ -33,4 +33,4 @@ * @(#)pw_copy.h 8.1 (Berkeley) 4/2/94 */ -void pw_copy __P((int, int, struct passwd *)); +void pw_copy __P((int, int, struct passwd *, struct passwd *)); -- Crist J. Clark | cjclark@alum.mit.edu | cjclark@jhu.edu http://people.freebsd.org/~cjc/ | cjc@freebsd.org To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message