Date: Fri, 20 Jul 2001 14:51:38 -0400 From: Garance A Drosihn <drosih@rpi.edu> To: freebsd-audit@freebsd.org Cc: freebsd-print@bostonradio.org, Anton Berezin <tobez@tobez.org> Subject: Better error-processing in lpd's dofork() rtn Message-ID: <p05101015b77e2b80f910@[128.113.24.47]> In-Reply-To: <p0510100db77d1591d36e@[128.113.24.47]> References: <3B5713AB.79322FDA@vangelderen.org> <20010719234413.A64433@heechee.tobez.org> <20010720001429.A65236@heechee.tobez.org> <p0510100db77d1591d36e@[128.113.24.47]>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?p05101015b77e2b80f910>