On Tue, Jul 30, 2024 at 11:05:24PM +0000, brian m. carlson wrote:

The git(1) manual page also says this:

  If you have an untrusted `.git` directory, you should first clone it
  with `git clone --no-local` to obtain a clean copy. Git does restrict
  the set of options and hooks that will be run by `upload-pack`, which
  handles the server side of a clone or fetch, but beware that the
  surface area for attack against `upload-pack` is large, so this does
  carry some risk. The safest thing is to serve the repository as an
  unprivileged user (either via git-daemon(1), ssh, or using
  other tools to change user ids). See the discussion in the `SECURITY`
  section of git-upload-pack(1).

I think that has been traditionally the policy that we've had here, and
given that we document that it is safe to do so, I think it should be
fine.  This documentation apparently came from Peff, who I think should
be reasonably well informed on such matters.

Right, that was added in v2.45, but I think was just explaining the
long-standing approach.

My understanding of the recent expansion of the ownership checks was
that they were a defense-in-depth. It _should_ be safe to run
upload-pack against an untrusted repository, but it would be easy for us
to accidentally violate that.

But I admit I did not carefully follow all of the discussion around
crossing filesystem boundaries (e.g., symbolic and hard links pointing
outside docker containers that are bind-mounted, or something like
that). I did not find those cases all that compelling from a security
perspective, but again, I didn't look into them that closely.

It could be that "clone" should try to avoid a "--local" clone from a
repo with different ownership, if the local hardlink path is more
dangerous. But that distinction is not something upload-pack even knows
about, so the code would have to go into clone. And then upload-pack
could be free to drop the ownership check. Certainly a lot of people
have complained about it (I had actually thought we reverted it in
v2.45.2, but that was just the extra hooks defense-in-depth; so again, I
may be getting confused about the extra value of the enter_repo()
ownership check that came at the same time).

-Peff