Date: Sat, 25 Nov 2000 09:15:21 -0500 From: "Brian F. Feldman" <green@FreeBSD.org> To: Alfred Perlstein <bright@wintelcom.net> Cc: Brian Feldman <green@FreeBSD.org>, cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/usr.sbin/inetd builtins.c Message-ID: <200011251415.eAPEFL566372@green.dyndns.org> In-Reply-To: Message from Alfred Perlstein <bright@wintelcom.net> of "Sat, 25 Nov 2000 02:12:11 PST." <20001125021211.H8051@fw.wintelcom.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Alfred Perlstein <bright@wintelcom.net> wrote: > * Brian Feldman <green@FreeBSD.org> [001124 20:13] wrote: > > green 2000/11/24 20:13:05 PST > > > > Modified files: > > usr.sbin/inetd builtins.c > > Log: > > Security fix: correctly set groups according to the user. Previously, > > root's groups' permissions were being used, so a user could read up to > > 16 (excluding initial whitespace) bytes of e.g. a wheel-accessible file. > > Was this reviewed by anyone? Security fixes need review. I've tested it, and it doesn't make things worse -- it prevents the blocking on FIFO case, and fixes the wrong use of groups. Robert Watson has stated he will review it soon. As for now, if it doesn't make things worse, and fixes the obvious cases, pending review I feel it's still appropriate to commit it. If there are problems, it can be changed. > > Also, don't allow blocking on the opening of ~/.fakeid, so replace a fopen() > > with open() and fdopen(). I knew I'd be going to hell for using C file > > streams instead of POSIX syscalls... > > What happens if ~/.fakeid is a FIFO without a writer/reader? Is > this the blocking you're trying to avoid? Without a writer, yes. This is a very elementary thing which I just didn't know to think about that long ago. > I think you may have a problem here because you don't use fclose > later, you may be leaking stdio streams with no backing fd. The fclose()s are are there just for fun, though. I should remove them. The daemon is about to exit. It would be a leak in the !S_ISREG() case, but I really didn't care to bother fixing something that didn't matter. > Don't you need to change: > > + if (fakeid_fd != -1) > + close(fakeid_fd); > > to something like: > > if (fakeid != NULL) > fclose(fakeid); > else if fakeid_fd != -1) > close(fakeid_fd); That would be strictly right. The fclose() closes the fd, too, so it's just paranoia to call it again (it being close()). > There's also a problem because this close isn't part of the 'else' > clause above, what happens is that you may call close on fakeid_fd > (after the 'else' clause) after calling fclose on it already in > the 'if' clause. Right, reason above. I'll do a cleanup where I either make the closes do the right thing or remove them, your choice :) > Doesn't the fstat need to happen before the fdopen? (to avoid > blocking on FIFOs or other wierdness). That's the race described in the comment. O_NONBLOCK handles that, though. > What's going on here? And why was it MFC'd already? It can expose up to 16 bytes of wheel-readable data. That's bad! > Maybe I'm wrong but these are some _possible_ bugs that I discovered > after a cursory examination of the change, had it been dicussed > I'd have either realized I was wrong or we could have had a proper > fix the first time. Yes, thank you for taking time out on this. Do you want to make the style changes? I think it would make sense to be consistent. If so, this is all I can think of to make the code more generally useful to the "persistent" program case: --- builtins.c.orig Sat Nov 25 09:09:34 2000 +++ builtins.c Sat Nov 25 09:13:50 2000 @@ -603,7 +603,6 @@ fclose(fakeid); goto printit; } - fclose(fakeid); /* * Usually, the file will have the desired identity * in the form "identity\n", so we use strcspn() to @@ -633,7 +632,9 @@ } } else cp = pw->pw_name; - if (fakeid_fd != -1) + if (fakeid != NULL) + fclose(fakeid); + else if (fakeid_fd != -1) close(fakeid_fd); } else if (!usedfallback) cp = pw->pw_name; -- Brian Fundakowski Feldman \ FreeBSD: The Power to Serve! / green@FreeBSD.org `------------------------------' To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200011251415.eAPEFL566372>