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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 28, 2010, 06:42 UTC
Message-ID: <20100728064251.GB743@dert.cs.uchicago.edu>
In-Reply-To: <AANLkTik1D45_cHPapbmMMys-V544ssCyoxrs5Fxck7oP@mail.gmail.com>

Greg Brockman wrote:

> That aside, here's an analysis of my patch series:
> Patch 1 just adds

[...]

Agh, it’s getting late. In my last message I completely forgot about the make_cmd() step. Sorry to waste your time on that.

And sorry to waste your time in general --- from your description it sounds like this could be summarized by:

 patch 1 adds
  memory allocation, split_cmdline call (innocuous things)
  execv which will fail if git-shell-commands is not a directory

> This will be an arbitrary directory if a user can 'su' to the
> git-shell user.

That would be an odd setup, but I guess with shared repositories there's a reason to do it.

> (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?)

I don't feel strongly either way. It would be a good way to put the worry about that attack vector to rest (if you use getpwent instead of getenv to fetch $HOME).

Patch 2 adds the new run_shell() feature, but it is guarded with access(COMMAND_DIR), so existing installations should not be affected.

Patch 3 does not even touch git.

> See anything I'm missing?

No, it looks good to me.

Thanks for the patient explanations.

Jonathan