{"thread":{"id":"60696","subject":"Limited operations in unsafe repositories","startedAt":"2024-01-07T19:40:28Z","lastAt":"2024-01-11T07:30:08Z","messageCount":7,"participants":["brian m. carlson","Jeff King","Junio C Hamano","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"486378","messageId":"ZZr-JLxubCvWe0EU@tapette.crustytoothpaste.net","threadId":"60696","inReplyTo":null,"subject":"Limited operations in unsafe repositories","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-01-07T19:40:20Z","receivedAt":"2024-01-07T19:40:28Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Right now, any time we try to set up a repository in that's owned by\nanother user, we die.  While good for security, this is inconvenient in\na bunch of ways.\n\nFor example, when Git LFS wants to push data locally, it needs to know\nwhere the `.git` directory is because it pushes the objects into\n`.git/lfs`.  Thus, we want to do `git rev-parse --absolute-git-dir` to\nfind the remote Git directory, but we can't do that if the repository is\nowned by a different user.\n\nThat issue also affects the Git LFS SSH transfer server (Scutiger),\nwhich also needs to read the configuration (to set the umask\nappropriately for `core.sharedrepository`).\n\nI had looked at sending a patch to make `git rev-parse` operate in a\nspecial mode where it's impossible to invoke any binaries at all, but\nunfortunately, `get_superproject_working_tree` invokes binaries, so\nthat's not possible.  (If anyone is interested in picking this up, there\nis a start on it, failing many tests, in the `rev-parse-safe-directory`\non my GitHub remote.)\n\nI guess I'm looking for us to provide some basic functionality that is\nguaranteed to work in this case, including `git rev-parse` and `git\nconfig -l`.  I don't think it's useful for every program that wants to\nwork with Git to need to implement its own repository discovery and\nconfig parsing, and those are essential needs for tooling that needs to\nwork with untrusted repositories.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"486522","messageId":"20240110120531.GA25541@coredump.intra.peff.net","threadId":"60696","inReplyTo":"ZZr-JLxubCvWe0EU@tapette.crustytoothpaste.net","subject":"Re: Limited operations in unsafe repositories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-10T12:05:31Z","receivedAt":"2024-01-10T12:05:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 07, 2024 at 07:40:20PM +0000, brian m. carlson wrote:\n\n> I had looked at sending a patch to make `git rev-parse` operate in a\n> special mode where it's impossible to invoke any binaries at all, but\n> unfortunately, `get_superproject_working_tree` invokes binaries, so\n> that's not possible.  (If anyone is interested in picking this up, there\n> is a start on it, failing many tests, in the `rev-parse-safe-directory`\n> on my GitHub remote.)\n> \n> I guess I'm looking for us to provide some basic functionality that is\n> guaranteed to work in this case, including `git rev-parse` and `git\n> config -l`.  I don't think it's useful for every program that wants to\n> work with Git to need to implement its own repository discovery and\n> config parsing, and those are essential needs for tooling that needs to\n> work with untrusted repositories.\n\nIf I understand correctly, you want to have some very limited code paths\nthat we know are OK to run even in an untrusted repo. My concern there\nwould be that it's easy for \"basic functionality\" to accidentally call\ninto less-limited code, which suddenly becomes a vulnerability.\n\nMy thinking is to flip that around: run all code, but put protection in\nthe spots that do unsafe things, like loading config or examining\nhooks. I.e., a patch like this:\n\ndiff --git a/config.c b/config.c\nindex 9ff6ae1cb9..c7bbd6bdda 100644\n--- a/config.c\n+++ b/config.c\n@@ -2026,7 +2026,7 @@ static int do_git_config_sequence(const struct config_options *opts,\n \tif (!opts->git_dir != !opts->commondir)\n \t\tBUG(\"only one of commondir and git_dir is non-NULL\");\n \n-\tif (opts->commondir) {\n+\tif (opts->commondir && is_safe_repository())\n \t\trepo_config = mkpathdup(\"%s/config\", opts->commondir);\n \t\tworktree_config = mkpathdup(\"%s/config.worktree\", opts->git_dir);\n \t} else {\ndiff --git a/hook.c b/hook.c\nindex f6306d72b3..4fcfd82dc5 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -12,6 +12,9 @@ const char *find_hook(const char *name)\n {\n \tstatic struct strbuf path = STRBUF_INIT;\n \n+\tif (!is_safe_repository())\n+\t\treturn NULL;\n+\n \tstrbuf_reset(&path);\n \tstrbuf_git_path(&path, \"hooks/%s\", name);\n \tif (access(path.buf, X_OK) < 0) {\n\nwhere is_safe_repository() is something like:\n\n  - if git_env_bool(\"GIT_ASSUME_SAFE\", 1) is true, return true\n    immediately\n\n  - otherwise, see if we are matched by safe.directory\n\nAnd then running:\n\n  git --assume-unsafe rev-parse ...\n\nwould set that variable to \"0\".\n\nAnd then most functionality would just work, modulo trusting repo hooks\nand config. I mentioned \"loading config\" earlier, but of course that\npatch is just touching the usual \"load all config sources\" code. We'd\nstill allow parsing .git/config for repo discovery, and something like:\n\n  git --assume-unsafe config -l --local\n\nwould still work if the caller knew they would be careful with the\noutput.\n\nThe downside, of course, is that we might fail to include other\ndangerous spots. Those two are the big ones, I think, but would we want\nto prevent people from pointing to /dev/mem in info/alternates? I dunno.\nIt certainly is more protection than we offer today, but maybe saying\n\"this is safe\" would produce a false sense of security.\n\nI do think something like git-prompt.sh could benefit here (it could use\n--assume-unsafe for all invocations so you don't get surprised just by\nchdir-ing into a downloaded tarball).\n\n-Peff\n"},{"id":"486576","messageId":"ZZ8pbAMNaBDFgf3G@tapette.crustytoothpaste.net","threadId":"60696","inReplyTo":"20240110120531.GA25541@coredump.intra.peff.net","subject":"Re: Limited operations in unsafe repositories","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-01-10T23:34:04Z","receivedAt":"2024-01-10T23:34:11Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-01-10 at 12:05:31, Jeff King wrote:\n> My thinking is to flip that around: run all code, but put protection in\n> the spots that do unsafe things, like loading config or examining\n> hooks. I.e., a patch like this:\n\nI think that's much what I had intended to do with not invoking binaries\nat all, except that it was limited to rev-parse.  I wonder if perhaps we\ncould do something similar if we had the `--assume-unsafe` argument you\nproposed, except that we would only allow the `git` binary and always\npass that argument to it in such a case.\n\nI don't think reading config is intrinsically unsafe; it's more of what\nwe do with it, which is spawning external processes, that's the problem.\nI suppose an argument could be made for injecting terminal sequences or\nsuch, though.  Hooks, obviously, are definitely unsafe.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"486579","messageId":"xmqq7ckg91m8.fsf@gitster.g","threadId":"60696","inReplyTo":"ZZ8pbAMNaBDFgf3G@tapette.crustytoothpaste.net","subject":"Re: Limited operations in unsafe repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-11T00:04:31Z","receivedAt":"2024-01-11T00:04:39Z","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> On 2024-01-10 at 12:05:31, Jeff King wrote:\n>> My thinking is to flip that around: run all code, but put protection in\n>> the spots that do unsafe things, like loading config or examining\n>> hooks. I.e., a patch like this:\n>\n> I think that's much what I had intended to do with not invoking binaries\n> at all, except that it was limited to rev-parse.  I wonder if perhaps we\n> could do something similar if we had the `--assume-unsafe` argument you\n> proposed, except that we would only allow the `git` binary and always\n> pass that argument to it in such a case.\n>\n> I don't think reading config is intrinsically unsafe; it's more of what\n> we do with it, which is spawning external processes, that's the problem.\n> I suppose an argument could be made for injecting terminal sequences or\n> such, though.  Hooks, obviously, are definitely unsafe.\n\nSure.  And we allow the location of hook programs to be specified as\nconfiguration variable values, which would make the config even more\ndangerous X-<.\n"},{"id":"486599","messageId":"20240111070114.GB48154@coredump.intra.peff.net","threadId":"60696","inReplyTo":"ZZ8pbAMNaBDFgf3G@tapette.crustytoothpaste.net","subject":"Re: Limited operations in unsafe repositories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-11T07:01:14Z","receivedAt":"2024-01-11T07:01:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 10, 2024 at 11:34:04PM +0000, brian m. carlson wrote:\n\n> On 2024-01-10 at 12:05:31, Jeff King wrote:\n> > My thinking is to flip that around: run all code, but put protection in\n> > the spots that do unsafe things, like loading config or examining\n> > hooks. I.e., a patch like this:\n> \n> I think that's much what I had intended to do with not invoking binaries\n> at all, except that it was limited to rev-parse.  I wonder if perhaps we\n> could do something similar if we had the `--assume-unsafe` argument you\n> proposed, except that we would only allow the `git` binary and always\n> pass that argument to it in such a case.\n\nI'm not sure what you mean by \"invoking binaries\". I had assumed that\nmeant just running other Git sub-processes. But if \"--assume-unsafe\" is\njust setting an environment variable, they'd automatically be protected.\n\n> I don't think reading config is intrinsically unsafe; it's more of what\n> we do with it, which is spawning external processes, that's the problem.\n> I suppose an argument could be made for injecting terminal sequences or\n> such, though.  Hooks, obviously, are definitely unsafe.\n\nRight, it's not config itself that's unsafe; it's that many options are.\nWe could try to annotate them to say \"it is OK to parse core.eol but not\ncore.pager\", presumably with an allow-known-good approach (since so many\nard bad!). But that feels like an ongoing maintenance headache, and an\neasy way to make a mistake (your mention of terminal sequences makes me\nassume you're thinking of \"color.diff.*\", etc). A rule like \"we do not\nread repo-level config at all\" seems easier to explain (to me, anyway).\n\n-Peff\n"},{"id":"486601","messageId":"ZZ-V-vwnm2hOkrMC@tanuki","threadId":"60696","inReplyTo":"20240111070114.GB48154@coredump.intra.peff.net","subject":"Re: Limited operations in unsafe repositories","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-11T07:17:14Z","receivedAt":"2024-01-11T07:17:21Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 11, 2024 at 02:01:14AM -0500, Jeff King wrote:\n> On Wed, Jan 10, 2024 at 11:34:04PM +0000, brian m. carlson wrote:\n> \n> > On 2024-01-10 at 12:05:31, Jeff King wrote:\n> > > My thinking is to flip that around: run all code, but put protection in\n> > > the spots that do unsafe things, like loading config or examining\n> > > hooks. I.e., a patch like this:\n> > \n> > I think that's much what I had intended to do with not invoking binaries\n> > at all, except that it was limited to rev-parse.  I wonder if perhaps we\n> > could do something similar if we had the `--assume-unsafe` argument you\n> > proposed, except that we would only allow the `git` binary and always\n> > pass that argument to it in such a case.\n> \n> I'm not sure what you mean by \"invoking binaries\". I had assumed that\n> meant just running other Git sub-processes. But if \"--assume-unsafe\" is\n> just setting an environment variable, they'd automatically be protected.\n> \n> > I don't think reading config is intrinsically unsafe; it's more of what\n> > we do with it, which is spawning external processes, that's the problem.\n> > I suppose an argument could be made for injecting terminal sequences or\n> > such, though.  Hooks, obviously, are definitely unsafe.\n> \n> Right, it's not config itself that's unsafe; it's that many options are.\n> We could try to annotate them to say \"it is OK to parse core.eol but not\n> core.pager\", presumably with an allow-known-good approach (since so many\n> ard bad!). But that feels like an ongoing maintenance headache, and an\n> easy way to make a mistake (your mention of terminal sequences makes me\n> assume you're thinking of \"color.diff.*\", etc). A rule like \"we do not\n> read repo-level config at all\" seems easier to explain (to me, anyway).\n\nWith the exemption of the repository format, I assume? We have to parse\nthings like `core.repositoryFormatVersion` and extensions in order to\nfigure out how a repository has to be accessed. So I agree that we\nshould not partition config based on safeness, which is going to be a\nheadache as you rightly point out. But we can partition based on whether\nor not config is required in order to access the repository, where the\nset of relevant config keys is a whole lot smaller.\n\nPatrick\n"},{"id":"486603","messageId":"20240111073007.GE48154@coredump.intra.peff.net","threadId":"60696","inReplyTo":"ZZ-V-vwnm2hOkrMC@tanuki","subject":"Re: Limited operations in unsafe repositories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-11T07:30:07Z","receivedAt":"2024-01-11T07:30:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 11, 2024 at 08:17:14AM +0100, Patrick Steinhardt wrote:\n\n> > Right, it's not config itself that's unsafe; it's that many options are.\n> > We could try to annotate them to say \"it is OK to parse core.eol but not\n> > core.pager\", presumably with an allow-known-good approach (since so many\n> > ard bad!). But that feels like an ongoing maintenance headache, and an\n> > easy way to make a mistake (your mention of terminal sequences makes me\n> > assume you're thinking of \"color.diff.*\", etc). A rule like \"we do not\n> > read repo-level config at all\" seems easier to explain (to me, anyway).\n> \n> With the exemption of the repository format, I assume? We have to parse\n> things like `core.repositoryFormatVersion` and extensions in order to\n> figure out how a repository has to be accessed. So I agree that we\n> should not partition config based on safeness, which is going to be a\n> headache as you rightly point out. But we can partition based on whether\n> or not config is required in order to access the repository, where the\n> set of relevant config keys is a whole lot smaller.\n\nRight. See the pseudo-patch I posted earlier. I think we just want to\ntouch do_git_config_sequence(), which leaves repo discovery and stuff\nlike \"git config --local\" working.\n\n-Peff\n"}]}