Re: [PATCH 04/26] upload-pack: convert to a builtin

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 21, 2018, 21:47 UTC
Message-ID: <20180221214755.GA10327@aiede.svl.corp.google.com>
In-Reply-To: <20180103203956.GA44365@google.com>

Brandon Williams wrote:

> On 01/03, Stefan Beller wrote:
> > On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams <bmwill@google.com> wrote:

Show 11 quoted lines

>>> In order to allow for code sharing with the server-side of fetch in
>>> protocol-v2 convert upload-pack to be a builtin.
>>
>> What is the security aspect of this patch?
>>
>> By making upload-pack builtin, it gains additional abilities,
>> such as answers to '-h' or '--help' (which would start a pager).
>> Is there an easy way to sooth my concerns? (best put into the
>> commit message)
>
> receive-pack is already a builtin, so theres that.

*nod*

Since v2.4.12~1^2 (shell: disallow repo names beginning with dash, 2017-04-29), git-shell refuses to pass --help to upload-pack, limiting the security impact in configurations that use git-shell (e.g. gitolite installations).

If you're not using git-shell, then hopefully you have some other form of filtering preventing arbitrary options being passed to git-upload-pack. If you don't, then you're in trouble, for the reasons described in that commit.

Since some installations may be allowing access to git-upload-pack (for read-only access) without allowing access to git-receive-pack, this does increase the chance of attack. On the other hand, I suspect the maintainability benefit is worth it.

For defense in depth, it would be comforting if the git wrapper had some understanding of "don't support --help in handle_builtin when invoked as a dashed command". That is, I don't expect that anyone has been relying on

	git-add --help

acting like

	git help add

instead of printing the usage message from

	git add -h

It's a little fussy because today we rewrite "git add --help" to "git-add --help" before rewriting it to "git help add"; we'd have to skip that middle hop for this to work.

I don't think that has to block this patch or series, though --- it's just a separate thought about hardening.

Cc-ing Sitaram Chamarty since he tends to be wiser about this kind of thing than I am.

What do you think?

Thanks, Jonathan