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: > > ...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
