Re: [PATCHv3] Updated patch series for providing mechanism to list available repositories

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 28, 2010, 23:52 UTC
Message-ID: <20100728235249.GA29156@dert.cs.uchicago.edu>
In-Reply-To: <1280358894.31999.9.camel@balanced-tree>

Anders Kaseorg wrote:

> On Wed, 2010-07-28 at 01:42 -0500, Jonathan Nieder wrote:

Show 6 quoted lines

>> (if you use getpwent instead of getenv to fetch $HOME).
>
> That seems like it could lead to problems with multiple users with the
> same UID, and possibly also on Windows.  If it’s important to be
> paranoid here, what about all the other places Git already uses
> getenv("HOME"), including where it reads ~/.gitconfig?

Thanks for a sanity check. I do not see the multiple-user problem (git-shell is meant to be the login shell, no?) but I think you are right about using getwpwent instead of $HOME being a pointless precaution. My confusion came from a misreading of how 'su' works.

Here was my worry: that a user could do something like this:

 $ mkdir /tmp/git-shell-commands
 $ ln -s /bin/sh /tmp/git-shell-commands/sh
 $ HOME=/tmp su git -m -c sh;		# (1)

and get a shell with the privileges of the user with git-shell as login shell, which is exactly what a restricted shell like this should be preventing.

Now if that is possible, what is to stop me from this?

 $ PAGER=evilscript su git -m -c git-receive-pack --help; # (2)

which became possible (modulo the su bit) as an unintended consequence when receive-pack became builtin.

If I understand the manual correctly, then at least on some systems, luckily su protects correctly against such problems.

	-m
		Preserve the current environment.

		If the target user has a restricted shell,
		this option has no effect (unless su is
		called by root).

Is that behavior portable? It certainly seems like the only sane way to behave. It’s a moot question for the inclusion of this patch series: if we need to worry about (1), then it is still not a regression because (2) was possible already.

The same discussion would seem to apply to ssh with PermitUserEnvironment enabled.