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
