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

From: Greg Brockman <gdb@mit.edu>
Date: Jul 28, 2010, 06:15 UTC
Message-ID: <AANLkTik1D45_cHPapbmMMys-V544ssCyoxrs5Fxck7oP@mail.gmail.com>
In-Reply-To: <20100728003336.GA2248@dert.cs.uchicago.edu>

Show 21 quoted lines

> No, it was only a vague thing.  I do not even use git-shell
> myself, so it was a vague worry for a scenario I am not even
> involved in.  So if you have thought it over and decided it is
> not an issue, that is good enough for me.
>
> What would be most comforting is an explanation like this:
>
>  "Uses not using this feature will not be impacted by patch 1,
>  since all it adds is:
>
>   - some memory allocation
>   - a call to split_cmdline, which I have audited and
>     seems to be safe
>   - an execv that does not permit . or / characters and so
>     can only run commands from the directory the user is
>     in (which would be safe because..."
>
> Actually if I understand correctly I am not comforted at all,
> because a former user at a multi-user installation that only has
> git-shell access now can suddenly run arbitrary commands from
> the home directory once git is upgraded.

So, I think the full story here is that "if one can create a git-shell-commands directory in the home directory of a user with login shell git-shell, then the latter user can then run arbitrary commands." So there's a prerequisite of being able to write to the git-shell user's $HOME, but if I can do that, I can presumably clobber the hooks in the git-shell user's git repositories, which can also allow arbitrary commands to be run. So in some sense, providing this functionality should be no worse than providing hooks.

That being said, perhaps one place where I could imagine this being
different is if:
- a nonbare repository is created in the git-shell user's $HOME directory
- an attacker creates a 'git-shell-commands' directory in a commit to
the repository
- someone checks out a commit with the 'git-shell-commands' directory.

One could avoid this by requiring that git-shell verify that the user's home directory is not a non-bare repository. However, I don't view this as a regression because in this case, the attacker could craft the git-shell user's dotfiles. This would lead to arbitrary command execution by e.g. setting the pager to /tmp/myevilscript in .manpath and running

  ssh git-shell-user@example.com "git-upload-pack '--help'"

That aside, here's an analysis of my patch series:
Patch 1 just adds
* Some memory allocation.
* A call to split_cmdline.  This splits a string on spaces, respecting
quotes and escaping via \.  I have audited it and it seems safe.
* An execv.  The command name is of the form
"git-shell-commands/$CLEAN" where $CLEAN does not contain . or /.
Thus it can only be run from the current working directory.  This will
be the git-shell user's $HOME if git-shell was spawned as a login
shell.  This will be an arbitrary directory if a user can 'su' to the
git-shell user.  (I am however starting to lean towards always
chdir'ing into the git-shell user's $HOME, do people feel strongly
about this in either direction?)
* An error message.

Patch 2 adds
* A call to run_shell, but only if the 'git-shell-commands' directory
is accessible.
* run_shell runs git-shell-commands/help and then runs in a loop
* a call to split_cmdline on user supplied input
* the user can type 'quit', 'exit', etc.. which will terminate the
shell, returning 0.
* an execv on a command of the form "git-shell-commands/$CLEAN", where
again $CLEAN does not contain . or /.
* an invalid command will restart the command loop

Patch 3 adds a list command and a help command to contrib/git-shell-commands, which will only be used if git-shell-commands is enabled. (Note: I'd like to make a small change to list, namely add a 2>/dev/null to the find command.)

See anything I'm missing?

Thanks,

Greg