Hi, 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 <bmwill@xxxxxxxxxx> 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 [1], 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? Thanks, Jonathan
