From owner-freebsd-audit Fri Jul 20 11:51:56 2001 Delivered-To: freebsd-audit@freebsd.org Received: from mail.rpi.edu (mail.rpi.edu [128.113.22.40]) by hub.freebsd.org (Postfix) with ESMTP id AB26137B401 for ; Fri, 20 Jul 2001 11:51:46 -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.11.3/8.11.3) with ESMTP id f6KIpj4138148; Fri, 20 Jul 2001 14:51:45 -0400 Mime-Version: 1.0 X-Sender: drosih@mail.rpi.edu Message-Id: In-Reply-To: References: <3B5713AB.79322FDA@vangelderen.org> <20010719234413.A64433@heechee.tobez.org> <20010720001429.A65236@heechee.tobez.org> Date: Fri, 20 Jul 2001 14:51:38 -0400 To: freebsd-audit@freebsd.org From: Garance A Drosihn Subject: Better error-processing in lpd's dofork() rtn Cc: freebsd-print@bostonradio.org, Anton Berezin Content-Type: text/plain; charset="us-ascii" ; format="flowed" 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 From the "Re: initgroups unsolicited warning?" thread... On 7/19/01, Garance A Drosihn wrote to -stable: > >At 12:14 AM +0200, Anton Berezin wrote: >> Here OK means that the caller checks initgroups() return code >> and acts appropriately. NOK means that initgroups() is called >> without return code checking. > [...] >> usr.sbin/lpr/lpd/printjob.c NOK > >Somehow I "just knew" that something in lpr would end up on a >list of things with not-OK code... :-) > >I'll try to look at that (just the call in lpd) if you wish. Here is a patch which adds error-checking for initgroups, and some other system-calls in the area. When testing this error- recovery, I found out that some of the error-checking already in dofork() was dangerously wrong. So, by the time I was done here, I had basically rewritten dofork() -- not that there is all that much there to rewrite... This all works in my testing, but please eyeball it and let me know if I've missed anything. I hope to install this in -current soon, so it can sit there for a week and still get MFC'ed into stable well before any code freeze for 4.4-release. note: you may get errors trying to apply this patch, because my email client sometimes puts an extra blank as the first character of some lines... Index: lpd/printjob.c =================================================================== RCS file: /home/ncvs/src/usr.sbin/lpr/lpd/printjob.c,v retrieving revision 1.40 diff -u -r1.40 printjob.c --- lpd/printjob.c 2001/07/15 00:09:46 1.40 +++ lpd/printjob.c 2001/07/20 18:35:56 @@ -77,8 +77,8 @@ #include "pathnames.h" #include "extern.h" -#define DORETURN 0 /* absorb fork error */ -#define DOABORT 1 /* abort if dofork fails */ +#define DORETURN 0 /* dofork should return "can't fork" error */ +#define DOABORT 1 /* dofork should just die if fork() fails */ /* * Error tokens @@ -106,6 +106,10 @@ static char title[80]; /* ``pr'' title */ static char locale[80]; /* ``pr'' locale */ +/* these two are set from pp->daemon_user, but only if they are needed */ +static char *daemon_uname; /* set from pwd->pw_name */ +static int daemon_defgid; + static char class[32]; /* classification field */ static char origin_host[MAXHOSTNAMELEN]; /* user's host machine */ /* indentation size in static characters */ @@ -1408,11 +1412,24 @@ static int dofork(const struct printer *pp, int action) { - register int i, forkpid; + int i, fail, forkpid; struct passwd *pwd; + forkpid = -1; + if (daemon_uname == NULL) { + pwd = getpwuid(pp->daemon_user); + if (pwd == NULL) { + syslog(LOG_ERR, "%s: Can't lookup default daemon uid (%ld) in password file", + pp->printer, pp->daemon_user); + goto error_ret; + } + daemon_uname = strdup(pwd->pw_name); + daemon_defgid = pwd->pw_gid; + } + for (i = 0; i < 20; i++) { - if ((forkpid = fork()) < 0) { + forkpid = fork(); + if (forkpid < 0) { sleep((unsigned)(i*i)); continue; } @@ -1420,25 +1437,49 @@ * Child should run as daemon instead of root */ if (forkpid == 0) { - if ((pwd = getpwuid(pp->daemon_user)) == NULL) { - syslog(LOG_ERR, "Can't lookup default daemon uid (%ld) in password file", - pp->daemon_user); + errno = 0; + fail = initgroups(daemon_uname, daemon_defgid); + if (fail) { + syslog(LOG_ERR, "%s: initgroups(%s,%u): %m", + pp->printer, daemon_uname, daemon_defgid); break; } - initgroups(pwd->pw_name, pwd->pw_gid); - setgid(pwd->pw_gid); - setuid(pp->daemon_user); + fail = setgid(daemon_defgid); + if (fail) { + syslog(LOG_ERR, "%s: setgid(%u): %m", + pp->printer, daemon_defgid); + break; + } + fail = setuid(pp->daemon_user); + if (fail) { + syslog(LOG_ERR, "%s: setuid(%ld): %m", + pp->printer, pp->daemon_user); + break; + } } - return(forkpid); + return forkpid; + } + + /* + * An error occurred. If the error is in the child process, then + * this routine must ALWAYS exit(). DORETURN only effects how + * errors should be handled in the parent process. + */ +error_ret: + if (forkpid == 0) { + syslog(LOG_ERR, "%s: dofork(): aborting child process...", + pp->printer); + exit(1); } - syslog(LOG_ERR, "can't fork"); + syslog(LOG_ERR, "%s: dofork(): failure in fork", pp->printer); + sleep(1); /* throttle errors, as a safety measure */ switch (action) { case DORETURN: - return (-1); + return -1; default: syslog(LOG_ERR, "bad action (%d) to dofork", action); - /*FALL THRU*/ + /* FALLTHROUGH */ case DOABORT: exit(1); } -- Garance Alistair Drosehn = gad@eclipse.acs.rpi.edu Senior Systems Programmer or gad@freebsd.org Rensselaer Polytechnic Institute or drosih@rpi.edu To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message