Re: Git clone reads safe.directory differently?

From: Jeff King <peff@peff.net>
Date: Jul 31, 2024, 07:28 UTC
Message-ID: <20240731072832.GB595974@coredump.intra.peff.net>
In-Reply-To: <ZqlxtGIyz0G9jlJr@tapette.crustytoothpaste.net>

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

Show 16 quoted lines

> 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