On Fri, Aug 04, 2000 at 02:52:30PM -0400, Larry Jones wrote:
> > 2) cvs pserver needs to run as root long enough to setuid/setgid, and i
> > wanted to add a line of code ensuring that that really happened. if
> > for some reason the setuid/setgid doesn't happen, and cvs is left
> > running as root, i want it to die before looking at any more user input.
>
> If either setuid or setgid fails, CVS exits immediately. People
> complain that CVS won't let root do commits, you're going to keep root
> from doing anything! (Well, your patch only objects if you've also
> chroot'ed, but the principle's the same.)
The principle is security: I absolutely do not under any circumstances in
any possible universe under any condition want someone accessing the root
user id through CVS pserver.
The behavior is disabled if you don't specify --chroot on the command line
so ordinary CVS operation is completely unaffected by this patch.
Also, are you sure that CVS is going to call setuid/setgid? I really
didn't want to walk through the code auditing line by line looking to
prove that. My patch tests it. Less auditing required.
> > 3) this way the cvs binary does not have to live inside the chrooted area,
> > so you don't have to re-install cvs again if your system wide cvs
> > happens to have this '--chroot' flag.
>
> If you run a shell script, it can copy the binary if it doesn't already
> exist.
Now you have a race condition. The attacker simply has to replace
the copy you just wrote with a copy they have supplied just after
you copy it but just before you execute it.
I'd rather not have the CVS binary inside the chroot area.
> > And, really, the cost is not high. It was about a five line change.
>
> But it's very Unix specific, and it seems to me to be adding the kitchen
> sink to CVS.
The setuid/setgid stuff is already pretty Unix specific too, don't
you think? And if you're really concerned about it, then by all
means autoconf it so that the code is only included if chroot()
exists on your system.
Justin
