On Thu, Aug 01, 2024 at 09:26:07PM +0000, brian m. carlson wrote:

On 2024-08-01 at 06:14:17, Jeff King wrote:

quoted

...this is doing that loosening for upload-pack by default. I'm not sure
if that is OK or not. My mental model has remained "it is OK to run
upload-pack on an untrusted repository", but it would make sense to get
input from folks who looked at this in the past, like Dscho, and/or to
reassess the threat model from scratch.

In particular I did not follow all of the potential issues with linked
local files. Are we good now after other fixes (in which case this patch
is OK)? Are we good only for non-local clones (so this patch is OK only
combined with a fix for clone to check ownership for --local mode)? Or
are there still problems if an attacker controls the repo paths, in
which case upload-pack should remain conservative?

I think we already have such a patch.  In clone, clone_local either has
option_shared (in which case we simply refer to the other repository via
an alternates file and don't touch it in any way), or it calls
copy_or_link_directory, which in turn calls die_upon_dubious_ownership.

Ah, thanks, I didn't realize that. Looks like it comes from 1204e1a824
(builtin/clone: refuse local clones of unsafe repositories, 2024-04-15).
I'm not sure if this is redundant currently; we still run
git-upload-pack even in the --local case, to get the list of refs. So
presumably it would fail on the same ownership check before even getting
to the local copy code.

But regardless, it would not be redundant if we loosen upload-pack. And
it means that my first two questions have the same answer.

The third one (does upload-pack have race problems with an attacker who
owns the repo?) I'm still not sure of.

One related topic that is potentially interesting as well is whether
`git bundle create` also offers the same security guarantees as `git
upload-pack` in that it can be safely run on an untrusted repository.
Either way, we may want to document that.

I suspect it could be made to give similar guarantees, but I don't think
anybody has ever made a conscious effort. At the very least this isn't
great:

  $ git config pager.bundle 'echo yikes'
  $ git bundle create foo.bundle --all
  yikes

upload-pack doesn't trigger this because the pager setup is tied to
the builtin RUN_SETUP option (arguably it should be made more explicit,
though). I think pack-objects actually exhibits the same problem, but
the pager is only triggered if isatty(1), and that would never be the
case when upload-pack runs it.

-Peff