{"thread":{"id":"58729","subject":"Odd git-config behavior","startedAt":"2022-10-31T20:52:03Z","lastAt":"2022-11-09T07:02:31Z","messageCount":4,"participants":["J. Paul Reed","Thomas Guyot"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"466119","messageId":"Y2A1bdiw6kGC65f/@sigkill.com","threadId":"58729","inReplyTo":null,"subject":"Odd git-config behavior","fromName":"J. Paul Reed","fromEmail":"preed@sigkill.com","sentAt":"2022-10-31T20:51:57Z","receivedAt":"2022-10-31T20:52:03Z","isPatch":false,"sender":{"key":"preed@sigkill.com","avatar":"https://gravatar.com/avatar/a6d898ae24f856b851376d22623fd35bb37134ad6df0483b84b2b135c4878237?d=mp&s=160"},"body":"\nHey all,\n\nI recently ran into interesting behavior with git-config (which I\noriginally thought was a bug).\n\nI was converting repos from the deprecated core.fsyncObjectFiles to the new\ncore.fsync option suite; I wrote some automation to do that, using\n\"git config -l\" to detect previously-converted repos.\n\nBut then some weekly repo maintenance automation complained about the\ndeprecated fsync option being present in some of the repo configs, even\nthough I thought those repos had been converted to the new settings.\n\nI did some digging, and it turns out that \"git config -l\" was reporting\nnothing (no output) in Git 2.37.4. I did some testing, and found that Git\n2.35.1 correctly reporting the repo's config settings.\n\nInterestingly, the maintenance automation runs fsck and some other things,\nand reports the presence of the deprecated fsync setting (which is what\nmade me notice); so that code path does read the config and run (and\ncomplain about the presence of deprecated settings).\n\nI did a git bisect between 2.35.1 and 2.37.4, and it looks like the\nfollowing commit changes the behavior:\n\n8959555cee7ec045958f9b6dd62e541affb7e7d9 is the first bad commit\ncommit 8959555cee7ec045958f9b6dd62e541affb7e7d9\nAuthor: Johannes Schindelin <johannes.schindelin@gmx.de>\nDate:   Wed Mar 2 12:23:04 2022 +0100\n\n    setup_git_directory(): add an owner check for the top-level directory\n    \n    It poses a security risk to search for a git directory outside of the\n    directories owned by the current user.\n\n    [full commit message clipped]\n\nSo... my maintenance automation runs as root, and the repo directories are\nuid/gid'd someone else (though, the config file inside the [bare] repo\nhappens to be owned by root)... so I suppose what I'm observing is expected\nbehavior?\n\nI guess this leaves me with two questions:\n\n    1. Why does \"git config\" refuse to run due to this security check, but\n       other git commands (\"git fsck,\" at least) run?\n\n    2. I think it might be useful to warn the user that the behavior they're\n       expecting isn't happening due to this security check, instead of just\n       outputting objectively wrong information (i.e. that no config options\n       exist when they actually do exist); I'd be curious what others think.\n\nbest,\npreed\n-- \nJ. Paul Reed\nhttps://jpaulreed.com\nPGP: 0x41AA0EF1\n\n"},{"id":"466504","messageId":"bc3aa4b1-4716-cf9c-5dff-22b25793f66c@gmail.com","threadId":"58729","inReplyTo":"Y2A1bdiw6kGC65f/@sigkill.com","subject":"Re: Odd git-config behavior","fromName":"Thomas Guyot","fromEmail":"tguyot@gmail.com","sentAt":"2022-11-04T11:47:46Z","receivedAt":"2022-11-04T11:47:57Z","isPatch":false,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"Hi Paul,\n\nOn 2022-10-31 16:51, J. Paul Reed wrote:\n> So... my maintenance automation runs as root, and the repo directories are\n> uid/gid'd someone else (though, the config file inside the [bare] repo\n> happens to be owned by root)... so I suppose what I'm observing is expected\n> behavior?\n\nYou definitively shouldn't run these checks as root. Git is a highly \nflexible/extensible product and god knows (and likely a few others in \nthis ML too) what could happen while you run git commands as root on an \n\"untrusted\" user repository.\n\nWhat prevents you from getting the owned uid or the repos and forking a \nprocess as that user to run the check?\n\n> I guess this leaves me with two questions:\n>\n>      1. Why does \"git config\" refuse to run due to this security check, but\n>         other git commands (\"git fsck,\" at least) run?\n\nArguably a read-only config operation could likely be allowed, unless \nthere is a possibility for some untrusted commands to be executed as \npart of this. I'm going to guess this was an easy entry point to cover \ndangerous commands, but having a finer-grained check would require \nmaking sure there's no dangerous code paths that could be exposed. The \nchange was to address CVE-2022-24765 - commit 8959555cee7 - so there was \nlikely time constraints to consider as well.\n\n>      2. I think it might be useful to warn the user that the behavior they're\n>         expecting isn't happening due to this security check, instead of just\n>         outputting objectively wrong information (i.e. that no config options\n>         exist when they actually do exist); I'd be curious what others think.\n\nWhat was the return code for the git config command? If it was zero when \nit didn't parse/output the config option you asked for that is \ndefinitively a bug. If you failed to check the return code of git-config \nthen you should fix your script/tool instead.\n\nRegards,\n\n--\nThomas\n"},{"id":"466921","messageId":"Y2rhfTYDEGQ7EhaS@sigkill.com","threadId":"58729","inReplyTo":"bc3aa4b1-4716-cf9c-5dff-22b25793f66c@gmail.com","subject":"Re: Odd git-config behavior","fromName":"J. Paul Reed","fromEmail":"preed@sigkill.com","sentAt":"2022-11-08T23:08:45Z","receivedAt":"2022-11-08T23:08:57Z","isPatch":false,"sender":{"key":"preed@sigkill.com","avatar":"https://gravatar.com/avatar/a6d898ae24f856b851376d22623fd35bb37134ad6df0483b84b2b135c4878237?d=mp&s=160"},"body":"On 04 Nov 2022 at 07:47:46, Thomas Guyot arranged the bits on my disk to say:\n\n> What prevents you from getting the owned uid or the repos and forking a \n> process as that user to run the check?\n\nLaziness?\n\nI should note: these aren't really \"untrusted\" user repositories, so I'm\nnot very concerned about it (though I understand your point).\n\nThis does beg the question: does running \"git fsck\" on an untrusted\nrepository as another user present a [security] problem?\n\nIf so, should it?\n\n> >      2. I think it might be useful to warn the user that the behavior they're\n> >         expecting isn't happening due to this security check, instead of just\n> >         outputting objectively wrong information (i.e. that no config options\n> >         exist when they actually do exist); I'd be curious what others think.\n> \n> What was the return code for the git config command? If it was zero when \n> it didn't parse/output the config option you asked for that is \n> definitively a bug. If you failed to check the return code of git-config \n> then you should fix your script/tool instead.\n\nunderworld # ~preed/src/git/git --version\ngit version 2.30.2.4.g8959555cee\nunderworld # GIT_PAGER=cat ~preed/src/git/git-config -l\nunderworld # echo $?\n0\n\nbest,\npreed\n-- \nJ. Paul Reed\nhttps://jpaulreed.com\nPGP: 0x41AA0EF1\n"},{"id":"466935","messageId":"536bcbc6-df12-e3b8-f995-35adfd311a84@gmail.com","threadId":"58729","inReplyTo":"Y2rhfTYDEGQ7EhaS@sigkill.com","subject":"Re: Odd git-config behavior","fromName":"Thomas Guyot","fromEmail":"tguyot@gmail.com","sentAt":"2022-11-09T07:02:25Z","receivedAt":"2022-11-09T07:02:31Z","isPatch":false,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"On 2022-11-08 18:08, J. Paul Reed wrote:\n> This does beg the question: does running \"git fsck\" on an untrusted\n> repository as another user present a [security] problem?\n>\n> If so, should it?\n\nProbably not, but I can't say for sure. Even some seemingly safe \ncommands can be dangerous in this context; for example \"git gc --auto\" \ninvokes a hook which could execute arbitrary code if run on an untrusted \nrepo.\n\nI haven't read the CVE but did notice the change - the primary issue if \nI'm not mistaken is when git behaves differently when there is a .git \ndir that could have been placed by a malicious user. I believe a safe \napproach has been taken where we have to explicitly whitelist repos or \npaths where the repos are trusted\n>> What was the return code for the git config command? If it was zero when\n>> it didn't parse/output the config option you asked for that is\n>> definitively a bug. If you failed to check the return code of git-config\n>> then you should fix your script/tool instead.\n> underworld # ~preed/src/git/git --version\n> git version 2.30.2.4.g8959555cee\n> underworld # GIT_PAGER=cat ~preed/src/git/git-config -l\n> underworld # echo $?\n> 0\n\nWe should test with the latest version... If git ignores the config it \nshould warn (like other commands do) and not return 0.\n\nSince git normally uses the global config when not a repo, it appears it \nkeeps looking for the global config after it decides the local one is no \ngood. What you see with this command is your global config not your \nrepo's config.\n\nRegards,\n\n--\nThomas\n"}]}