Hi,

Jeff King wrote:

On Thu, Feb 22, 2018 at 10:07:15AM -0800, Brandon Williams wrote:

quoted

On 02/22, Jeff King wrote:

quoted

On Wed, Feb 21, 2018 at 01:44:22PM -0800, Jonathan Tan wrote:

quoted

On Tue,  6 Feb 2018 17:12:41 -0800
Brandon Williams [off-list ref] wrote:

quoted

quoted

quoted

quoted

In order to allow for code sharing with the server-side of fetch in
protocol-v2 convert upload-pack to be a builtin.

[...]

quoted

quoted

quoted

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