Anders brings up a good point.
And note that as I alluded to before, there is another attack
$ echo 'DEFINE pager evilscript' > /tmp/.manpath
$ HOME=/tmp su git -m -c "git-receive-pack '--help'" (3)
which only requires being able to control HOME.
(Incidentally, I just noticed a segfault with
$ unset HOME
$ su git -m -c "git-receive-pack '~'"
that's probably worth fixing... if people don't think this is too
pedantic of a case to fix, I'll submit a patch for it in a later
series [I think the segfault comes from path.c:expand_user_path].)
Anyway, i'll revise my first patch to use HOME rather than getpw*.
Greg
On Wed, Jul 28, 2010 at 4:52 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Anders Kaseorg wrote:
>> On Wed, 2010-07-28 at 01:42 -0500, Jonathan Nieder wrote:
>
>>> (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.
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html