Jeff King wrote:
> On Thu, Feb 22, 2018 at 10:07:15AM -0800, Brandon Williams wrote:
>> On 02/22, Jeff King wrote:
>>> On Wed, Feb 21, 2018 at 01:44:22PM -0800, Jonathan Tan wrote:
>>>> On Tue, 6 Feb 2018 17:12:41 -0800
>>>> Brandon Williams <bmw...@google.com> wrote:
>>>>> In order to allow for code sharing with the server-side of fetch in
>>>>> protocol-v2 convert upload-pack to be a builtin.
>>>> As Stefan mentioned in , also mention in the commit message that this
>>>> means that the "git-upload-pack" invocation gains additional
>>>> capabilities (for example, invoking a pager for --help).
>>> And possibly respecting pager.upload-pack, which would violate our rule
>>> that it is safe to run upload-pack in untrusted repositories.
>> And this isn't an issue with receive-pack because this same guarantee
>> doesn't exist?
> Yes, exactly (which is confusing and weird, yes, but that's how it is).
To be clear, which of the following are you (most) worried about?
1. being invoked with --help and spawning a pager
2. receiving and acting on options between 'git' and 'upload-pack'
3. repository discovery
4. pager config
5. alias discovery
6. increased code surface / unknown threats
For (1), "--help" has to be the first argument. "git daemon" passes
--strict so it doesn't happen there. "git http-backend" passes
--stateless-rpc so it doesn't happen there. "git shell" sanitizes
input to avoid it happening there.
A custom setup could provide their own entry point that doesn't do
such sanitization. I think that in some sense it's out of our hands,
but it would be nice to harden as described upthread.
For (2), I am having trouble imagining a setup where it would happen.
upload-pack doesn't have the RUN_SETUP or RUN_SETUP_GENTLY flag set,
so (3) doesn't apply.
Although in most setups the user does not control the config files on
a server, item (4) looks like a real issue worth solving. I think we
should introduce a flag to skip looking for pager config. We could
use it for receive-pack, too.
Builtins are handled before aliases, so (5) doesn't apply.
(6) is a real issue: it is why "git shell" is not a builtin, for
example. But I would rather that we use appropriate sanitization
before upload-pack is invoked than live in fear. git upload-pack is
sufficiently complicated that I don't think the git.c wrapper
increases the complexity by a significant amount.
That leaves me with a personal answer of only being worried about (4)
and not the rest. What do you think? Is one of the other items I
listed above worrisome, or is there another item I missed?