{"thread":{"id":"61854","subject":"Git clone reads safe.directory differently?","startedAt":"2024-07-27T16:24:22Z","lastAt":"2024-08-05T15:49:24Z","messageCount":21,"participants":["W. Michael Petullo","Jeff King","brian m. carlson","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"499488","messageId":"ZqUc8DJ1uKcHYlcy@imp.flyn.org","threadId":"61854","inReplyTo":null,"subject":"Git clone reads safe.directory differently?","fromName":"W. Michael Petullo","fromEmail":"mike@flyn.org","sentAt":"2024-07-27T16:14:40Z","receivedAt":"2024-07-27T16:24:22Z","isPatch":false,"sender":{"key":"mike@flyn.org","avatar":null},"body":"Glen Choo's commit 6061601d modified Git so that safe.directory could\nbe set on the command line using \"-c\". It seems most sub-commands work\nthis way, but not \"clone\". Here is an example:\n\n====================\n$ whoami\nmike\n$ ls -ld /tmp/y | cut -d \" \" -f 3\nroot\n$ /home/mike/Scratch/git/git -c safe.directory=\"*\" -C /tmp/y log\ncommit 4be57d4489010047fd3ef39a633264f634a432b5 (HEAD -> master)\nAuthor: [...]\nDate:   Sat Jul 27 11:06:19 2024 -0500\n\n    Initial commit\n$ /home/mike/Scratch/git/git -c safe.directory=\"*\" clone /tmp/y /tmp/z\nCloning into '/tmp/z'...\nwarning: templates not found in /home/mike/share/git-core/templates\nfatal: detected dubious ownership in repository at '/tmp/y/.git'\nTo add an exception for this directory, call:\n\n\tgit config --global --add safe.directory /tmp/y/.git\nfatal: Could not read from remote repository.\n\nPlease make sure you have the correct access rights\nand the repository exists.\n====================\n\nGlen's commit 6061601d seems to indicate there is no security reason\nfor this:\n\n\t\"As a result, `safe.directory` now respects '-c', so update the\n\ttests and docs accordingly. It used to ignore '-c' due to how it\n\twas implemented, not because of security or correctness concerns.\"\n\nI added some printf statements to ensure_valid_ownership() in setup.c,\nand I found that they executed in the case of running \"git log\" but not\n\"git clone\".\n\nA little more research found that \"git clone\" bailed after the call to\ntransport_get_remote_refs() many lines before the call to clone_local()\n-> copy_or_link_directory() -> die_upon_dubious_ownership().\nI suspect something along that earlier call path is still using\nsetup_git_directory_gently(), rather than git_protected_config(), to\ncheck for dubious ownership.\n\n[System Info]\ngit version:\ngit version 2.46.0.rc2.dirty\ncpu: x86_64\nbuilt from commit: ad57f148c6b5f8735b62238dda8f571c582e0e54\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nlibcurl: 8.6.0\nOpenSSL: OpenSSL 3.2.1 30 Jan 2024\nzlib: 1.3.1.zlib-ng\nuname: Linux 6.9.8-200.fc40.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Jul  5 16:20:11 UTC 2024 x86_64\ncompiler info: gnuc: 14.1\nlibc info: glibc: 2.39\n$SHELL (typically, interactive shell): /bin/bash\n\n[Enabled Hooks]\n\n-- \nMike\n\n:wq\n"},{"id":"499491","messageId":"20240727215845.GA1263246@coredump.intra.peff.net","threadId":"61854","inReplyTo":"ZqUc8DJ1uKcHYlcy@imp.flyn.org","subject":"Re: Git clone reads safe.directory differently?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-27T21:58:45Z","receivedAt":"2024-07-27T21:58:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 27, 2024 at 11:14:40AM -0500, W. Michael Petullo wrote:\n\n> Glen Choo's commit 6061601d modified Git so that safe.directory could\n> be set on the command line using \"-c\". It seems most sub-commands work\n> this way, but not \"clone\". Here is an example:\n\nThis is because upload-pack, the server half of a fetch/clone, is run as\na separate process which does not accept the client-side \"-c\" options.\nSee this email for more details and a workaround:\n\n  https://lore.kernel.org/git/20240529102307.GF1098944@coredump.intra.peff.net/\n\n-Peff\n"},{"id":"499500","messageId":"ZqZjRMqpEV_3WIkD@imp.flyn.org","threadId":"61854","inReplyTo":"20240727215845.GA1263246@coredump.intra.peff.net","subject":"Re: Git clone reads safe.directory differently?","fromName":"W. Michael Petullo","fromEmail":"mike@flyn.org","sentAt":"2024-07-28T15:27:00Z","receivedAt":"2024-07-28T15:27:01Z","isPatch":false,"sender":{"key":"mike@flyn.org","avatar":null},"body":">> Glen Choo's commit 6061601d modified Git so that safe.directory could\n>> be set on the command line using \"-c\". It seems most sub-commands work\n>> this way, but not \"clone\". Here is an example:\n> \n> This is because upload-pack, the server half of a fetch/clone, is run as\n> a separate process which does not accept the client-side \"-c\" options.\n> See this email for more details and a workaround:\n> \n>   https://lore.kernel.org/git/20240529102307.GF1098944@coredump.intra.peff.net/\n\nThank you, Jeff! This works and makes sense, although I can see how users\nwould be confused by the edge case.\n\nFor completeness, I investigated how to do the same over SSH. Imagine a\nrepository user-owned by Bob, but group-owned with r/w/x permissions by\na group containing Alice. It seems the same trick fails because git-shell\nrejects the custom upload-pack command:\n\n\tgit -c safe.directory=\"*\" clone -u 'git -c safe.directory=\"*\" upload-pack' alice@git.example.com:/shared/repository\n\tCloning into 'git'...\n\tfatal: unrecognized command 'git -c safe.directory=\"*\" upload-pack '/shared/repository''\n\tfatal: Could not read from remote repository.\n\n\tPlease make sure you have the correct access rights\n\tand the repository exists.\n\nI was able to overcome this by creating /home/alice/git-shell-commands/upload-pack-safe,\nplacing the following there\n\n\t#!/bin/sh\n\n\tgit -c safe.directory=\"$1\" upload-pack $1\n\nand running:\n\n\tgit -c safe.directory=\"*\" clone -u upload-pack-safe alice@git.example.com:/shared/repository\n\nThis seems to be another interface edge case. Is my solution reasonable,\nor is there something else that would be more consistent?\n\nRelated: Would anyone be interested in working on an academic paper\nabout safe.directory? It seems like people who write Git forge-type\nsoftware or other collaborative systems based on Git would benefit from\na rigorous description of the conditions and operations under which the\nuse of safe.directory is indeed safe. I am not sure I have this worked\nout in my own mind yet. Something like \"Setuid Demystified\" from USENIX\n2002 might help.\n\n-- \nMike\n\n:wq\n"},{"id":"499502","messageId":"20240728224807.GA1299337@coredump.intra.peff.net","threadId":"61854","inReplyTo":"ZqZjRMqpEV_3WIkD@imp.flyn.org","subject":"Re: Git clone reads safe.directory differently?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-28T22:48:07Z","receivedAt":"2024-07-28T22:48:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 28, 2024 at 10:27:00AM -0500, W. Michael Petullo wrote:\n\n> For completeness, I investigated how to do the same over SSH. Imagine a\n> repository user-owned by Bob, but group-owned with r/w/x permissions by\n> a group containing Alice. It seems the same trick fails because git-shell\n> rejects the custom upload-pack command:\n> \n> \tgit -c safe.directory=\"*\" clone -u 'git -c safe.directory=\"*\" upload-pack' alice@git.example.com:/shared/repository\n> \tCloning into 'git'...\n> \tfatal: unrecognized command 'git -c safe.directory=\"*\" upload-pack '/shared/repository''\n> \tfatal: Could not read from remote repository.\n> \n> \tPlease make sure you have the correct access rights\n> \tand the repository exists.\n\nIt should work over ssh in the general case, but yes, if you're using\ngit-shell, the whole idea there is to restrict what the client can\nspecify. And so there won't be any workaround from the client side (if\nthere were, it would be a bug!).\n\n> I was able to overcome this by creating /home/alice/git-shell-commands/upload-pack-safe,\n> placing the following there\n> \n> \t#!/bin/sh\n> \n> \tgit -c safe.directory=\"$1\" upload-pack $1\n> \n> and running:\n> \n> \tgit -c safe.directory=\"*\" clone -u upload-pack-safe alice@git.example.com:/shared/repository\n> \n> This seems to be another interface edge case. Is my solution reasonable,\n> or is there something else that would be more consistent?\n\nSo yeah, that would work. But it might be simpler still to just put\n\"[safe]directory = *\" into /home/alice/.gitconfig. Then the client side\ndoes not have to do anything differently (or you could even put in a\nmore limited relaxation of the rules, like \"/shared/repository/*\" or\nwhatever).\n\n-Peff\n"},{"id":"499597","messageId":"ZqjQi6i2kiY4gcc1@imp.flyn.org","threadId":"61854","inReplyTo":"20240728224807.GA1299337@coredump.intra.peff.net","subject":"Re: Git clone reads safe.directory differently?","fromName":"W. Michael Petullo","fromEmail":"mike@flyn.org","sentAt":"2024-07-30T11:37:47Z","receivedAt":"2024-07-30T11:37:59Z","isPatch":false,"sender":{"key":"mike@flyn.org","avatar":null},"body":">> I was able to overcome this by creating /home/alice/git-shell-commands/upload-pack-safe,\n>> placing the following there\n>> \n>> \t#!/bin/sh\n>> \n>> \tgit -c safe.directory=\"$1\" upload-pack $1\n>> \n>> and running:\n>> \n>> \tgit -c safe.directory=\"*\" clone -u upload-pack-safe alice@git.example.com:/shared/repository\n>> \n>> This seems to be another interface edge case. Is my solution reasonable,\n>> or is there something else that would be more consistent?\n \n> So yeah, that would work. But it might be simpler still to just put\n> \"[safe]directory = *\" into /home/alice/.gitconfig. Then the client side\n> does not have to do anything differently (or you could even put in a\n> more limited relaxation of the rules, like \"/shared/repository/*\" or\n> whatever).\n\nA little feedback to regarding why I chose to avoid .gitconfig along\nwith some consequences:\n\nI have a system built around Git in which I use the permissions approaches\nI have described in this email thread. The downside of .gitconfig is\nthat its effect is global. I fear I might lose track of the fact that I\nhave turned off a security protection in a global way, possibly years\ndown the road as I maintain my system.  Instead, I went about this by\npatching particular uses of Git to use \"-c\" and so on. I only needed to\ndo this at two or three places my code executes Git out of dozens.\n\nThis makes my intention clear in the code (that executes Git) and lessens\nthe likelihood of me (a year or so later?) forgetting I am responsible\nfor ensuring the circumstances leave my use of safe.directory safe.\n\nI suppose the inconsistency surrounding \"-c\" that we have discussed sort\nof works against my approach. Indeed, I wonder if in an ideal world we\nshould remove the dangerous features that have good intentions\n(hardlinks and so on) and instead require users to opt in to them.\nI say ideal world because this ignores backward/forward compatibility\nissues.\n\nPerhaps a compromise would be to tie safe.directory to the type\nof source directory given to clone. Would a remote URL be enough to\nturn off the safe.directory checks on a clone, similar to the effect of\na remote URL on --local/--no-local?\n\nThe current behavior seems to mean:\n\n\t(1) -c safe.directory works for some sub-commands, but not clone,\n\n\t(2) clone requires your additional workaround, and \n\n\t(3) your workaround does not work with SSH/git-shell for the\n\tobvious reason that git-shell needs to limit the \"commands\" it\n\twill run; this requires another level of intervention.\n\nAnother thing I came across was in the git-clone man page:\n\n\t\"When given, and the repository to clone from is accessed via ssh,\n\tthis specifies a non-default path for the command run on the other end.\"\n\nYour \"-u 'git -c safe.directory=X upload-pack'\" workaround uses\nupload-pack in the absence of SSH, so I wonder if \"and the repository\nto clone from is accessed via ssh\" is inaccurate.\n\n-- \nMike\n\n:wq\n"},{"id":"499648","messageId":"Zqlo-i8uCb1Yr4Jm@tapette.crustytoothpaste.net","threadId":"61854","inReplyTo":"ZqjQi6i2kiY4gcc1@imp.flyn.org","subject":"Re: Git clone reads safe.directory differently?","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-07-30T22:28:10Z","receivedAt":"2024-07-30T22:28:20Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-07-30 at 11:37:47, W. Michael Petullo wrote:\n> Perhaps a compromise would be to tie safe.directory to the type\n> of source directory given to clone. Would a remote URL be enough to\n> turn off the safe.directory checks on a clone, similar to the effect of\n> a remote URL on --local/--no-local?\n\nI think if we're using --no-local (that is, if we're using upload-pack\ninstead of creating symlinks), then we should not complain about the\nrepository ownership.  It's supposed to always be safe to clone or fetch\nfrom an untrusted repository, and we shouldn't complain about that.\n\nBoth of these commands should work correctly and do not, and that's a\nbug (assuming tr1 is owned by a different user):\n\n  git clone --no-local --no-hardlinks $PWD/tr1 tr2\n  git clone --no-local --no-hardlinks ssh://localhost$PWD/tr1 tr2\n\ngit-upload-pack should not complain about safe.directory at all. If it's\nnot secure to clone or fetch from an untrusted repository with\ngit-upload-pack, then we have a bigger security problem that needs to be\naddressed.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"499653","messageId":"xmqqv80m8pha.fsf@gitster.g","threadId":"61854","inReplyTo":"Zqlo-i8uCb1Yr4Jm@tapette.crustytoothpaste.net","subject":"Re: Git clone reads safe.directory differently?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T22:49:37Z","receivedAt":"2024-07-30T22:49:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> I think if we're using --no-local (that is, if we're using upload-pack\n> instead of creating symlinks), then we should not complain about the\n> repository ownership.  It's supposed to always be safe to clone or fetch\n> from an untrusted repository, and we shouldn't complain about that.\n\nThe safety is promised by \"git fetch\" when you fetch from some other\nmachine because the only thing you will be seeing from that\nuntrusted source is a bytestream that is the packfile, plus the tips\nof their histories---nothing runs as yourself in this exchange other\nthan what you control, i.e. \"git fetch\", locally defined hooks and\nfilters defined by your configuration..  They cannot affect your\nconfiguration file and hooks that may name extra programs that may\nrun as you while fetching or cloning.\n\nWhen you are using \"--no-local\" on the same machine, I do not think\nthere is any guarantee that \"upload-pack\" side is safe.  And that is\nwhere the safe.directory thing needs to kick in.\n\nStepping into an untrusted repository and running git operations\nopens up the user the Git process runs as to attacks by the\nuntrusted repository, i.e. you may invoke hooks on the upload-pack\nside, defined in the source repository that is controlled by others,\nand that is where the safe.directory thing kicks in.  You need to\ndeclare that you trust that source repository.\n\n"},{"id":"499655","messageId":"xmqqr0ba8p7u.fsf@gitster.g","threadId":"61854","inReplyTo":"xmqqv80m8pha.fsf@gitster.g","subject":"Re: Git clone reads safe.directory differently?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T22:55:17Z","receivedAt":"2024-07-30T22:55:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> When you are using \"--no-local\" on the same machine, I do not think\n> there is any guarantee that \"upload-pack\" side is safe.  And that is\n> where the safe.directory thing needs to kick in.\n\nAh, I take it back.  packObjectsHook won't run based on what the\nuntrusted source repository configures, so at least we have been\naware of the fact that upload-pack must be more careful than other\nstuff.\n\n"},{"id":"499657","messageId":"ZqlxtGIyz0G9jlJr@tapette.crustytoothpaste.net","threadId":"61854","inReplyTo":"xmqqv80m8pha.fsf@gitster.g","subject":"Re: Git clone reads safe.directory differently?","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-07-30T23:05:24Z","receivedAt":"2024-07-30T23:05:27Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-07-30 at 22:49:37, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> > I think if we're using --no-local (that is, if we're using upload-pack\n> > instead of creating symlinks), then we should not complain about the\n> > repository ownership.  It's supposed to always be safe to clone or fetch\n> > from an untrusted repository, and we shouldn't complain about that.\n> \n> The safety is promised by \"git fetch\" when you fetch from some other\n> machine because the only thing you will be seeing from that\n> untrusted source is a bytestream that is the packfile, plus the tips\n> of their histories---nothing runs as yourself in this exchange other\n> than what you control, i.e. \"git fetch\", locally defined hooks and\n> filters defined by your configuration..  They cannot affect your\n> configuration file and hooks that may name extra programs that may\n> run as you while fetching or cloning.\n> \n> When you are using \"--no-local\" on the same machine, I do not think\n> there is any guarantee that \"upload-pack\" side is safe.  And that is\n> where the safe.directory thing needs to kick in.\n> \n> Stepping into an untrusted repository and running git operations\n> opens up the user the Git process runs as to attacks by the\n> untrusted repository, i.e. you may invoke hooks on the upload-pack\n> side, defined in the source repository that is controlled by others,\n> and that is where the safe.directory thing kicks in.  You need to\n> declare that you trust that source repository.\n\nI don't believe git-upload-pack invokes hooks.  At least, I don't see\nany documented to do so, and I'm not aware of any from my experience\nhosting repositories.  Certainly git-receive-pack does so, which I can\nunderstand as a problem, but upload-pack should not.\n\nAnd the manual page does say this:\n\n  Most Git commands should not be run in an untrusted `.git` directory\n  (see the section `SECURITY` in git(1)). `upload-pack` tries to\n  avoid any dangerous configuration options or hooks from the repository\n  it's serving, making it safe to clone an untrusted directory and run\n  commands on the resulting clone.\n\nThe git(1) manual page also says this:\n\n  If you have an untrusted `.git` directory, you should first clone it\n  with `git clone --no-local` to obtain a clean copy. Git does restrict\n  the set of options and hooks that will be run by `upload-pack`, which\n  handles the server side of a clone or fetch, but beware that the\n  surface area for attack against `upload-pack` is large, so this does\n  carry some risk. The safest thing is to serve the repository as an\n  unprivileged user (either via git-daemon(1), ssh, or using\n  other tools to change user ids). See the discussion in the `SECURITY`\n  section of git-upload-pack(1).\n\nI think that has been traditionally the policy that we've had here, and\ngiven that we document that it is safe to do so, I think it should be\nfine.  This documentation apparently came from Peff, who I think should\nbe reasonably well informed on such matters.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"499667","messageId":"20240731071909.GA595974@coredump.intra.peff.net","threadId":"61854","inReplyTo":"ZqjQi6i2kiY4gcc1@imp.flyn.org","subject":"Re: Git clone reads safe.directory differently?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-31T07:19:09Z","receivedAt":"2024-07-31T07:19:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 30, 2024 at 06:37:47AM -0500, W. Michael Petullo wrote:\n\n> A little feedback to regarding why I chose to avoid .gitconfig along\n> with some consequences:\n> \n> I have a system built around Git in which I use the permissions approaches\n> I have described in this email thread. The downside of .gitconfig is\n> that its effect is global. I fear I might lose track of the fact that I\n> have turned off a security protection in a global way, possibly years\n> down the road as I maintain my system.  Instead, I went about this by\n> patching particular uses of Git to use \"-c\" and so on. I only needed to\n> do this at two or three places my code executes Git out of dozens.\n> \n> This makes my intention clear in the code (that executes Git) and lessens\n> the likelihood of me (a year or so later?) forgetting I am responsible\n> for ensuring the circumstances leave my use of safe.directory safe.\n\nMakes sense, though I think it's somewhat specific to your setup / use\ncase. If you had arbitrary clients connecting, telling them to use \"-u\"\nprobably wouldn't scale.\n\n> I suppose the inconsistency surrounding \"-c\" that we have discussed sort\n> of works against my approach. Indeed, I wonder if in an ideal world we\n> should remove the dangerous features that have good intentions\n> (hardlinks and so on) and instead require users to opt in to them.\n> I say ideal world because this ignores backward/forward compatibility\n> issues.\n\nSo there's an open question on the degree to which running upload-pack\nis actually dangerous. It's not _supposed_ to be, but the ownership\ncheck is a defense-in-depth approach to safety.\n\n> Perhaps a compromise would be to tie safe.directory to the type\n> of source directory given to clone. Would a remote URL be enough to\n> turn off the safe.directory checks on a clone, similar to the effect of\n> a remote URL on --local/--no-local?\n\nIt already is tied to the URL, in the sense that \"clone\" is not doing\nanything at all. It is entirely the server upload-pack that has started\ndoing the ownership check (as of v2.45). For a local clone, the \"server\"\nis just another process owned by your user on the same machine, and\nthat's where most people are running into it.\n\nBut for all other remotes, the ownership issue is one for the server to\ndeal with. So server operators serving over http or via git-daemon would\nneed to make sure their daemon processes match the on-disk repository\nownership (or use safe.directory to get around it). And there really is\n_potential_ danger there. The server-side daemon process is running\nupload-pack against a repository owned by some other user. If you're\nserving untrusted repositories that way (say, a system git-daemon with\nrepositories owned by users), there could be a mismatch there (but\nagain, this is defense-in-depth and upload-pack is supposed to be safe\nhere).\n\nCloning over ssh is sort of a middle ground. It still is the server's\nproblem, and the \"user\" in this case is the user on the server side, not\nthe user on the local machine running \"git clone\"). But often that user\nis conceptually the same entity (but doesn't have to be; as you can\nimagine, GitHub's ssh endpoint does not have one Unix user per\nconceptual user).\n\n> The current behavior seems to mean:\n> \n> \t(1) -c safe.directory works for some sub-commands, but not clone,\n\nAlso fetch, push, ls-remote, etc. Anything dealing with a \"remote\"\nrepository which spawn extra processes that clear the environment of\n\"-c\" config.\n\n> \t(2) clone requires your additional workaround, and \n> \n> \t(3) your workaround does not work with SSH/git-shell for the\n> \tobvious reason that git-shell needs to limit the \"commands\" it\n> \twill run; this requires another level of intervention.\n\nRight. The workaround does work, though, if you're not using git-shell.\n\n> Another thing I came across was in the git-clone man page:\n> \n> \t\"When given, and the repository to clone from is accessed via ssh,\n> \tthis specifies a non-default path for the command run on the other end.\"\n> \n> Your \"-u 'git -c safe.directory=X upload-pack'\" workaround uses\n> upload-pack in the absence of SSH, so I wonder if \"and the repository\n> to clone from is accessed via ssh\" is inaccurate.\n\nYes, I think that documentation is overly specific. The \"-u\" command is\nused whenever we can ask the other side to run an arbitrary command. So\nlocal clones, as well as ssh. Not git-daemon or http, where we just tell\nthe other side the type of operation to perform.\n\n-Peff\n"},{"id":"499668","messageId":"20240731072832.GB595974@coredump.intra.peff.net","threadId":"61854","inReplyTo":"ZqlxtGIyz0G9jlJr@tapette.crustytoothpaste.net","subject":"Re: Git clone reads safe.directory differently?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-31T07:28:32Z","receivedAt":"2024-07-31T07:28:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 30, 2024 at 11:05:24PM +0000, brian m. carlson wrote:\n\n> The git(1) manual page also says this:\n> \n>   If you have an untrusted `.git` directory, you should first clone it\n>   with `git clone --no-local` to obtain a clean copy. Git does restrict\n>   the set of options and hooks that will be run by `upload-pack`, which\n>   handles the server side of a clone or fetch, but beware that the\n>   surface area for attack against `upload-pack` is large, so this does\n>   carry some risk. The safest thing is to serve the repository as an\n>   unprivileged user (either via git-daemon(1), ssh, or using\n>   other tools to change user ids). See the discussion in the `SECURITY`\n>   section of git-upload-pack(1).\n> \n> I think that has been traditionally the policy that we've had here, and\n> given that we document that it is safe to do so, I think it should be\n> fine.  This documentation apparently came from Peff, who I think should\n> be reasonably well informed on such matters.\n\nRight, that was added in v2.45, but I think was just explaining the\nlong-standing approach.\n\nMy understanding of the recent expansion of the ownership checks was\nthat they were a defense-in-depth. It _should_ be safe to run\nupload-pack against an untrusted repository, but it would be easy for us\nto accidentally violate that.\n\nBut I admit I did not carefully follow all of the discussion around\ncrossing filesystem boundaries (e.g., symbolic and hard links pointing\noutside docker containers that are bind-mounted, or something like\nthat). I did not find those cases all that compelling from a security\nperspective, but again, I didn't look into them that closely.\n\nIt could be that \"clone\" should try to avoid a \"--local\" clone from a\nrepo with different ownership, if the local hardlink path is more\ndangerous. But that distinction is not something upload-pack even knows\nabout, so the code would have to go into clone. And then upload-pack\ncould be free to drop the ownership check. Certainly a lot of people\nhave complained about it (I had actually thought we reverted it in\nv2.45.2, but that was just the extra hooks defense-in-depth; so again, I\nmay be getting confused about the extra value of the enter_repo()\nownership check that came at the same time).\n\n-Peff\n"},{"id":"499717","messageId":"xmqqo76d7coa.fsf@gitster.g","threadId":"61854","inReplyTo":"20240731072832.GB595974@coredump.intra.peff.net","subject":"Re: Git clone reads safe.directory differently?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-31T16:23:49Z","receivedAt":"2024-07-31T16:23:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> It could be that \"clone\" should try to avoid a \"--local\" clone from a\n> repo with different ownership, if the local hardlink path is more\n> dangerous. But that distinction is not something upload-pack even knows\n> about, so the code would have to go into clone.\n\nSounds good.\n\n> And then upload-pack\n> could be free to drop the ownership check. Certainly a lot of people\n> have complained about it (I had actually thought we reverted it in\n> v2.45.2, but that was just the extra hooks defense-in-depth; so again, I\n> may be getting confused about the extra value of the enter_repo()\n> ownership check that came at the same time).\n\nAs enter_repo() is about the protocol driver thing and not about\nnormal users working inside a repository, calls to it appear only in\nreceive-pack, upload-pack, upload-archive, http-backend, and daemon.\n\nAmong them, upload-pack is the only thing we promise that is safe to\nwork even in a hostile repository?  If we push into a repository\nover the local transport, we would trigger post-receive hook as\nourselves, which we would probably not want.  The same story goes\nfor daemon, http-backend, and upload-archive.\n\nSo we probably need to add another axis to the \"strict\" parameter\nenter_repo() takes to selectively disable the ownership checks only\nfor upload-pack, or something like that.\n\nWe may want to restrict \"tar.<format>.command\" only to protected\nconfiguration and then we may be able to loosen the ownership check\nfor the upload-archive command.\n\nThanks.\n"},{"id":"499751","messageId":"xmqq1q391afc.fsf@gitster.g","threadId":"61854","inReplyTo":"xmqqo76d7coa.fsf@gitster.g","subject":"Re: Git clone reads safe.directory differently?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-31T22:08:55Z","receivedAt":"2024-07-31T22:09:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> So we probably need to add another axis to the \"strict\" parameter\n> enter_repo() takes to selectively disable the ownership checks only\n> for upload-pack, or something like that.\n\nSo, here is a rough sketch for the above.  Interested parties may\nbuild on top of it, perhaps by adding separate knobs to loosen or\ntighten the second parameter given to enter_repo() at different\ncallsites, by writing tests to make sure they work as intended, and\nby documenting the security story around it none of which I do here\n;-).\n\nThe two bits are\n\n - ENTER_REPO_STRICT: callers that require exact paths (as opposed\n   to allowing known suffixes like \".git\", \".git/.git\" to be\n   omitted) can set this bit.  Corresponds to the \"strict\" parameter\n   that the flags wordreplaces.\n\n - ENTER_REPO_ANY_OWNER_OK: callers that are willing to run without\n   ownership check can set this bit.\n\nThe former is --strict-paths option of \"git daemon\".  The latter is\nset only by upload-pack, but you may want to add configuration knobs\nin protected configuration files to loosen it per callsites.\n\n builtin/upload-pack.c |  5 ++++-\n daemon.c              |  6 ++++--\n path.c                | 10 ++++++----\n path.h                |  7 ++++++-\n 4 files changed, 20 insertions(+), 8 deletions(-)\n\ndiff --git c/builtin/upload-pack.c w/builtin/upload-pack.c\nindex 46d93278d9..fe50ce3eed 100644\n--- c/builtin/upload-pack.c\n+++ w/builtin/upload-pack.c\n@@ -36,6 +36,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n \t\t\t    N_(\"interrupt transfer after <n> seconds of inactivity\")),\n \t\tOPT_END()\n \t};\n+\tunsigned enter_repo_flags = ENTER_REPO_ANY_OWNER_OK;\n \n \tpacket_trace_identity(\"upload-pack\");\n \tdisable_replace_refs();\n@@ -51,7 +52,9 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n \n \tdir = argv[0];\n \n-\tif (!enter_repo(dir, strict))\n+\tif (strict)\n+\t\tenter_repo_flags |= ENTER_REPO_STRICT;\n+\tif (!enter_repo(dir, enter_repo_flags))\n \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n \n \tswitch (determine_protocol_version_server()) {\ndiff --git c/daemon.c w/daemon.c\nindex 17d331b2f3..fb37135521 100644\n--- c/daemon.c\n+++ w/daemon.c\n@@ -149,6 +149,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \tsize_t rlen;\n \tconst char *path;\n \tconst char *dir;\n+\tunsigned enter_repo_flags;\n \n \tdir = directory;\n \n@@ -239,14 +240,15 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \t\tdir = rpath;\n \t}\n \n-\tpath = enter_repo(dir, strict_paths);\n+\tenter_repo_flags = strict_paths ? ENTER_REPO_STRICT : 0;\n+\tpath = enter_repo(dir, enter_repo_flags);\n \tif (!path && base_path && base_path_relaxed) {\n \t\t/*\n \t\t * if we fail and base_path_relaxed is enabled, try without\n \t\t * prefixing the base path\n \t\t */\n \t\tdir = directory;\n-\t\tpath = enter_repo(dir, strict_paths);\n+\t\tpath = enter_repo(dir, enter_repo_flags);\n \t}\n \n \tif (!path) {\ndiff --git c/path.c w/path.c\nindex 19f7684f38..df5aefdb8f 100644\n--- c/path.c\n+++ w/path.c\n@@ -727,7 +727,7 @@ char *interpolate_path(const char *path, int real_home)\n  * links.  User relative paths are also returned as they are given,\n  * except DWIM suffixing.\n  */\n-const char *enter_repo(const char *path, int strict)\n+const char *enter_repo(const char *path, unsigned flags)\n {\n \tstatic struct strbuf validated_path = STRBUF_INIT;\n \tstatic struct strbuf used_path = STRBUF_INIT;\n@@ -735,7 +735,7 @@ const char *enter_repo(const char *path, int strict)\n \tif (!path)\n \t\treturn NULL;\n \n-\tif (!strict) {\n+\tif (!(flags & ENTER_REPO_STRICT)) {\n \t\tstatic const char *suffix[] = {\n \t\t\t\"/.git\", \"\", \".git/.git\", \".git\", NULL,\n \t\t};\n@@ -779,7 +779,8 @@ const char *enter_repo(const char *path, int strict)\n \t\tif (!suffix[i])\n \t\t\treturn NULL;\n \t\tgitfile = read_gitfile(used_path.buf);\n-\t\tdie_upon_dubious_ownership(gitfile, NULL, used_path.buf);\n+\t\tif (!(flags & ENTER_REPO_ANY_OWNER_OK))\n+\t\t\tdie_upon_dubious_ownership(gitfile, NULL, used_path.buf);\n \t\tif (gitfile) {\n \t\t\tstrbuf_reset(&used_path);\n \t\t\tstrbuf_addstr(&used_path, gitfile);\n@@ -790,7 +791,8 @@ const char *enter_repo(const char *path, int strict)\n \t}\n \telse {\n \t\tconst char *gitfile = read_gitfile(path);\n-\t\tdie_upon_dubious_ownership(gitfile, NULL, path);\n+\t\tif (!(flags & ENTER_REPO_ANY_OWNER_OK))\n+\t\t\tdie_upon_dubious_ownership(gitfile, NULL, path);\n \t\tif (gitfile)\n \t\t\tpath = gitfile;\n \t\tif (chdir(path))\ndiff --git c/path.h w/path.h\nindex a6f0b70692..a0021a1425 100644\n--- c/path.h\n+++ w/path.h\n@@ -187,7 +187,12 @@ int calc_shared_perm(int mode);\n int adjust_shared_perm(const char *path);\n \n char *interpolate_path(const char *path, int real_home);\n-const char *enter_repo(const char *path, int strict);\n+\n+enum {\n+\tENTER_REPO_STRICT = (1<<0),\n+\tENTER_REPO_ANY_OWNER_OK = (1<<1),\n+};\n+const char *enter_repo(const char *path, unsigned flags);\n const char *remove_leading_path(const char *in, const char *prefix);\n const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);\n int normalize_path_copy_len(char *dst, const char *src, int *prefix_len);\n"},{"id":"499766","messageId":"20240801060813.GC621899@coredump.intra.peff.net","threadId":"61854","inReplyTo":"xmqqo76d7coa.fsf@gitster.g","subject":"Re: Git clone reads safe.directory differently?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-01T06:08:13Z","receivedAt":"2024-08-01T06:08:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 31, 2024 at 09:23:49AM -0700, Junio C Hamano wrote:\n\n> > And then upload-pack\n> > could be free to drop the ownership check. Certainly a lot of people\n> > have complained about it (I had actually thought we reverted it in\n> > v2.45.2, but that was just the extra hooks defense-in-depth; so again, I\n> > may be getting confused about the extra value of the enter_repo()\n> > ownership check that came at the same time).\n> \n> As enter_repo() is about the protocol driver thing and not about\n> normal users working inside a repository, calls to it appear only in\n> receive-pack, upload-pack, upload-archive, http-backend, and daemon.\n> \n> Among them, upload-pack is the only thing we promise that is safe to\n> work even in a hostile repository?  If we push into a repository\n> over the local transport, we would trigger post-receive hook as\n> ourselves, which we would probably not want.  The same story goes\n> for daemon, http-backend, and upload-archive.\n\nYes, upload-pack is the only safe one.\n\n> So we probably need to add another axis to the \"strict\" parameter\n> enter_repo() takes to selectively disable the ownership checks only\n> for upload-pack, or something like that.\n\nAgreed.\n\n> We may want to restrict \"tar.<format>.command\" only to protected\n> configuration and then we may be able to loosen the ownership check\n> for the upload-archive command.\n\nYes, though I don't now how valuable that is in practice (versus the\nregression for folks who have a custom tar.*.command in their local-repo\nconfig).\n\n-Peff\n"},{"id":"499767","messageId":"20240801061417.GD621899@coredump.intra.peff.net","threadId":"61854","inReplyTo":"xmqq1q391afc.fsf@gitster.g","subject":"Re: Git clone reads safe.directory differently?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-01T06:14:17Z","receivedAt":"2024-08-01T06:14:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 31, 2024 at 03:08:55PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > So we probably need to add another axis to the \"strict\" parameter\n> > enter_repo() takes to selectively disable the ownership checks only\n> > for upload-pack, or something like that.\n> \n> So, here is a rough sketch for the above.  Interested parties may\n> build on top of it, perhaps by adding separate knobs to loosen or\n> tighten the second parameter given to enter_repo() at different\n> callsites, by writing tests to make sure they work as intended, and\n> by documenting the security story around it none of which I do here\n> ;-).\n\nThis looks roughly like I'd expect, but...\n\n> diff --git c/builtin/upload-pack.c w/builtin/upload-pack.c\n> index 46d93278d9..fe50ce3eed 100644\n> --- c/builtin/upload-pack.c\n> +++ w/builtin/upload-pack.c\n> @@ -36,6 +36,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n>  \t\t\t    N_(\"interrupt transfer after <n> seconds of inactivity\")),\n>  \t\tOPT_END()\n>  \t};\n> +\tunsigned enter_repo_flags = ENTER_REPO_ANY_OWNER_OK;\n>  \n>  \tpacket_trace_identity(\"upload-pack\");\n>  \tdisable_replace_refs();\n> @@ -51,7 +52,9 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n>  \n>  \tdir = argv[0];\n>  \n> -\tif (!enter_repo(dir, strict))\n> +\tif (strict)\n> +\t\tenter_repo_flags |= ENTER_REPO_STRICT;\n> +\tif (!enter_repo(dir, enter_repo_flags))\n>  \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n>  \n>  \tswitch (determine_protocol_version_server()) {\n\n...this is doing that loosening for upload-pack by default. I'm not sure\nif that is OK or not. My mental model has remained \"it is OK to run\nupload-pack on an untrusted repository\", but it would make sense to get\ninput from folks who looked at this in the past, like Dscho, and/or to\nreassess the threat model from scratch.\n\nIn particular I did not follow all of the potential issues with linked\nlocal files. Are we good now after other fixes (in which case this patch\nis OK)? Are we good only for non-local clones (so this patch is OK only\ncombined with a fix for clone to check ownership for --local mode)? Or\nare there still problems if an attacker controls the repo paths, in\nwhich case upload-pack should remain conservative?\n\n-Peff\n"},{"id":"499856","messageId":"xmqqbk2cxp98.fsf@gitster.g","threadId":"61854","inReplyTo":"20240801061417.GD621899@coredump.intra.peff.net","subject":"Re: Git clone reads safe.directory differently?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-01T14:59:47Z","receivedAt":"2024-08-01T14:59:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... if that is OK or not. My mental model has remained \"it is OK to run\n> upload-pack on an untrusted repository\", but it would make sense to get\n> input from folks who looked at this in the past, like Dscho, and/or to\n> reassess the threat model from scratch.\n>\n> In particular I did not follow all of the potential issues with linked\n> local files. Are we good now after other fixes (in which case this patch\n> is OK)? Are we good only for non-local clones (so this patch is OK only\n> combined with a fix for clone to check ownership for --local mode)? Or\n> are there still problems if an attacker controls the repo paths, in\n> which case upload-pack should remain conservative?\n\nGood questions.\n"},{"id":"499907","messageId":"Zqv9b_B5wKGp331o@tapette.crustytoothpaste.net","threadId":"61854","inReplyTo":"20240801061417.GD621899@coredump.intra.peff.net","subject":"Re: Git clone reads safe.directory differently?","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-08-01T21:26:07Z","receivedAt":"2024-08-01T21:26:17Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-08-01 at 06:14:17, Jeff King wrote:\n> ...this is doing that loosening for upload-pack by default. I'm not sure\n> if that is OK or not. My mental model has remained \"it is OK to run\n> upload-pack on an untrusted repository\", but it would make sense to get\n> input from folks who looked at this in the past, like Dscho, and/or to\n> reassess the threat model from scratch.\n> \n> In particular I did not follow all of the potential issues with linked\n> local files. Are we good now after other fixes (in which case this patch\n> is OK)? Are we good only for non-local clones (so this patch is OK only\n> combined with a fix for clone to check ownership for --local mode)? Or\n> are there still problems if an attacker controls the repo paths, in\n> which case upload-pack should remain conservative?\n\nI think we already have such a patch.  In clone, clone_local either has\noption_shared (in which case we simply refer to the other repository via\nan alternates file and don't touch it in any way), or it calls\ncopy_or_link_directory, which in turn calls die_upon_dubious_ownership.\n\nThe other case, where is_local is not set (and thus clone_local is not\ncalled), calls transport_fetch_refs, which either calls\nfetch_refs_via_pack or fetch_refs_via_bundle, both of which I assume\nactually make a git-upload-pack call.\n\nOne related topic that is potentially interesting as well is whether\n`git bundle create` also offers the same security guarantees as `git\nupload-pack` in that it can be safely run on an untrusted repository.\nEither way, we may want to document that.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"499908","messageId":"xmqqplqrud17.fsf@gitster.g","threadId":"61854","inReplyTo":"Zqv9b_B5wKGp331o@tapette.crustytoothpaste.net","subject":"Re: Git clone reads safe.directory differently?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-01T21:52:04Z","receivedAt":"2024-08-01T21:52:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> The other case, where is_local is not set (and thus clone_local is not\n> called), calls transport_fetch_refs, which either calls\n> fetch_refs_via_pack or fetch_refs_via_bundle, both of which I assume\n> actually make a git-upload-pack call.\n\nOK.\n\n> One related topic that is potentially interesting as well is whether\n> `git bundle create` also offers the same security guarantees as `git\n> upload-pack` in that it can be safely run on an untrusted repository.\n> Either way, we may want to document that.\n\nTrue.  I think \"bundle create\" in that regard can be viewed as a\nthin wrapper around pack-objects and there is no customization\npossibilities (smudge/clean filters, hooks, etc.) that malicious\nrepositories can take advantage of.\n\nBut what worries me more is the fact that any such evaluation can\nonly be about the current state.  A careless change to say\npack-objects [*] that allows innocent looking customzation to take\nplace _could_ turn out to be triggerable by the repository when\nupload-pack is run, and the \"innocent looking\" customization may be\nmore generic than necessary and can be used creatively to cause\ndamage.  \"Don't allow any customizations to 'rev-list' because its\ninternal is shared with 'pack-objects' that in turn is run from\n'upload-pack'\" would not be an answer.\n\nIt is unclear to me how to make sure such an evaluation done once in\nthe past will stay valid.  That is something we need to come up with\na viable approach and document, too.\n\n\n[Footnote]\n\n * ... or rev-list or any pieces of machinery that are recursively\n   relied on by a command that ought to be kept safe.\n"},{"id":"500053","messageId":"20240805094709.GA632664@coredump.intra.peff.net","threadId":"61854","inReplyTo":"Zqv9b_B5wKGp331o@tapette.crustytoothpaste.net","subject":"Re: Git clone reads safe.directory differently?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-05T09:47:09Z","receivedAt":"2024-08-05T09:47:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 01, 2024 at 09:26:07PM +0000, brian m. carlson wrote:\n\n> On 2024-08-01 at 06:14:17, Jeff King wrote:\n> > ...this is doing that loosening for upload-pack by default. I'm not sure\n> > if that is OK or not. My mental model has remained \"it is OK to run\n> > upload-pack on an untrusted repository\", but it would make sense to get\n> > input from folks who looked at this in the past, like Dscho, and/or to\n> > reassess the threat model from scratch.\n> > \n> > In particular I did not follow all of the potential issues with linked\n> > local files. Are we good now after other fixes (in which case this patch\n> > is OK)? Are we good only for non-local clones (so this patch is OK only\n> > combined with a fix for clone to check ownership for --local mode)? Or\n> > are there still problems if an attacker controls the repo paths, in\n> > which case upload-pack should remain conservative?\n> \n> I think we already have such a patch.  In clone, clone_local either has\n> option_shared (in which case we simply refer to the other repository via\n> an alternates file and don't touch it in any way), or it calls\n> copy_or_link_directory, which in turn calls die_upon_dubious_ownership.\n\nAh, thanks, I didn't realize that. Looks like it comes from 1204e1a824\n(builtin/clone: refuse local clones of unsafe repositories, 2024-04-15).\nI'm not sure if this is redundant currently; we still run\ngit-upload-pack even in the --local case, to get the list of refs. So\npresumably it would fail on the same ownership check before even getting\nto the local copy code.\n\nBut regardless, it would not be redundant if we loosen upload-pack. And\nit means that my first two questions have the same answer.\n\nThe third one (does upload-pack have race problems with an attacker who\nowns the repo?) I'm still not sure of.\n\n> One related topic that is potentially interesting as well is whether\n> `git bundle create` also offers the same security guarantees as `git\n> upload-pack` in that it can be safely run on an untrusted repository.\n> Either way, we may want to document that.\n\nI suspect it could be made to give similar guarantees, but I don't think\nanybody has ever made a conscious effort. At the very least this isn't\ngreat:\n\n  $ git config pager.bundle 'echo yikes'\n  $ git bundle create foo.bundle --all\n  yikes\n\nupload-pack doesn't trigger this because the pager setup is tied to\nthe builtin RUN_SETUP option (arguably it should be made more explicit,\nthough). I think pack-objects actually exhibits the same problem, but\nthe pager is only triggered if isatty(1), and that would never be the\ncase when upload-pack runs it.\n\n-Peff\n"},{"id":"500087","messageId":"ZrDxHPh-daYPxzT2@imp.flyn.org","threadId":"61854","inReplyTo":"20240805094709.GA632664@coredump.intra.peff.net","subject":"Re: Git clone reads safe.directory differently?","fromName":"W. Michael Petullo","fromEmail":"mike@flyn.org","sentAt":"2024-08-05T15:34:52Z","receivedAt":"2024-08-05T15:35:01Z","isPatch":false,"sender":{"key":"mike@flyn.org","avatar":null},"body":"Thank you---from a somewhat sophisticated Git user, but not a Git\ndeveloper---for all of the discussion that followed my initial inquiry.\nI thought I would follow up with some comments that follow from my reading\nof the responses and the SECURITY sections of various man pages.\n\n> [From Jeff:] So there's an open question on the degree to which\n> running upload-pack is actually dangerous. It's not _supposed_ to be,\n> but the ownership check is a defense-in-depth approach to safety.\n\nThe discussion here is partially an attempt to better understand some\nof the internal workings and guarantees to determine whether or not the\nprotections applied in some cases are spurious, right?\n\n> [From Brian:] Both of these commands should work correctly and do not,\n> and that's a bug (assuming tr1 is owned by a different user):\n>\n>   git clone --no-local --no-hardlinks $PWD/tr1 tr2\n>   git clone --no-local --no-hardlinks ssh://localhost$PWD/tr1 tr2\n\nThe remarks above closely resemble the use cases that prompted my\ninitial inquiry. It would help a great deal if these worked, and also if\n\"--no-local --no-hardlinks\" remains the default for the SSH variant.\nIs it fair to conclude that these not working represent a bug?\n\n> [From Brian:] The git(1) manual page also says this:\n>   [...] but beware that the surface area for attack against\n>   `upload-pack` is large\n\nDoes the use of attack surface here mean the general idea of \"the volume\nof lines of code in upload-pack is large and thus upload-pack likely\ncontains bugs that might represent vulnerabilities\", or is there\nsomething specific about upload-pack that should raise concern?\n\nI do have a related question on the SSH/push side. Let us imagine a host\nthat has user accounts bearing git-shell as their shell. Thus these users\ncan only interact with the host (and the repositories therein) using Git.\nFurther imagine a trusted agent on the host creates these repositories and\nany hooks therein. Can a special account, with permission to access all\nof the repositories, safely clone and push other users' repositories? Or,\ncould a user introduce a dangerous hook or configuration option to their\nremote repository using only Git by way of git-shell?\n\nThank you again, especially Jeff, who provided me with some practical\nworkarounds early in this thread.\n\n-- \nMike\n\n:wq\n"},{"id":"500090","messageId":"xmqqzfpr3r7i.fsf@gitster.g","threadId":"61854","inReplyTo":"20240805094709.GA632664@coredump.intra.peff.net","subject":"Re: Git clone reads safe.directory differently?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-05T15:49:21Z","receivedAt":"2024-08-05T15:49:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I suspect it could be made to give similar guarantees, but I don't think\n> anybody has ever made a conscious effort.\n\nI also recall that we thought upload-pack was safe because it does\nnothing more than what it was asked to do, until we realized that in\na lazy clone it would slurp what it is missing from its promisors,\nat which point it does more than just \"serve things from here\".\n\nWhat worries me more is the fact that any such evaluation can only\nbe about the current state.  A careless change to say pack-objects\nthat allows innocent looking customization to take place _could_\nturn out to be triggerable by the repository when upload-pack is\nrun, and the \"innocent looking\" customization may be more generic\nthan necessary and can be used creatively to cause damage.  \"Don't\nallow any customizations to 'rev-list' because its internal is\nshared with 'pack-objects' that in turn is run from 'upload-pack'\"\nwould not be an answer.\n\nIt is unclear to me how to make sure such an evaluation, that was\ndone once in the past, will stay valid.  That is something we need\nto come up with a viable approach and document, too.\n\nThanks.\n"}]}