{"thread":{"id":"44441","subject":"Bug: git config does not respect read-only .gitconfig file","startedAt":"2016-11-08T15:22:58Z","lastAt":"2016-11-09T13:52:24Z","messageCount":7,"participants":["Jonathan Word","Markus Hitter","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"305560","messageId":"CAD9aWChH14eviop=0_Ma_2Pa-2OyWJp9KjimH8dyqy-XDn9Rhw@mail.gmail.com","threadId":"44441","inReplyTo":null,"subject":"Bug: git config does not respect read-only .gitconfig file","fromName":"Jonathan Word","fromEmail":"argoday@argoday.com","sentAt":"2016-11-08T15:22:14Z","receivedAt":"2016-11-08T15:22:58Z","isPatch":false,"sender":{"key":"argoday@argoday.com","avatar":"https://gravatar.com/avatar/ce7cd96c8f84f3d9168000ad9b10f22dc7f02b04fe02900c5982f52716e2069e?d=mp&s=160"},"body":"All,\n\nI recently discovered that `git config` does not respect read-only files.\n\nThis caused unexpected difficulty in managing the global .gitconfig\nfor a system account shared by a large team. A team member was able to\nexecute a `git config --global` command without any notice or warning\nthat the underlying config file had been marked read-only in an\nattempt to prevent unintentional changes. If instead git had raised a\nwarning saying that the \"gitconfig is read-only\" this would have\nprevented that team member from accidentally breaking our git config.\n\n\nBug detail:\n\nDue to the implementation strategy of\nconfig::git_config_set_multivar_in_file_gently (\nhttps://github.com/git/git/blob/5b33cb1fd733f581da07ae8afa7e9547eafd248e/config.c#L2074\n) the file permissions of the target .gitconfig file are not\nrespected.\n\n\nProposal:\n\nPart 1) Add a .gitconfig variable to respect a read-only gitconfig\nfile and optional \"--force\" override option for the `git config`\ncommand\n\nSuch a gitconfig variable could be defined as:\nconfig.respectFileMode: [ \"never\", \"allow-override\", \"always\" ]\n\nWhere:\n* never - read-only file mode of config files are ignored (aka:\nexisting behavior)\n* allow-override - read-only file mode of config files is respected\nunless the user provides a \"--force\" option to `git config`\n* always - read-only file mode of config files is respected (and the\n\"--force\" option does not work)\n\nPart 2) Change config::git_config_set_multivar_in_file_gently (\nhttps://github.com/git/git/blob/5b33cb1fd733f581da07ae8afa7e9547eafd248e/config.c#L2077\n) to verify write permissions on the destination depending on the\nspecified config.respectFileMode variable and \"--force\" option.\n\n\n\nI think that this is a reasonably sized change that enables users to\nopt-in to a 'strict mode' while preserving current behavior.\n\n\nThoughts?\n\n\nTested with:\nOS: Linux\nVersion: 2.9.0 (issue exists in current master branch)\n"},{"id":"305561","messageId":"40608c85-f870-87f7-daee-7fa98f5d19c1@jump-ing.de","threadId":"44441","inReplyTo":"CAD9aWChH14eviop=0_Ma_2Pa-2OyWJp9KjimH8dyqy-XDn9Rhw@mail.gmail.com","subject":"Re: Bug: git config does not respect read-only .gitconfig file","fromName":"Markus Hitter","fromEmail":"mah@jump-ing.de","sentAt":"2016-11-08T16:49:27Z","receivedAt":"2016-11-08T16:49:53Z","isPatch":false,"sender":{"key":"mah@jump-ing.de","avatar":"https://avatars.githubusercontent.com/u/318581?v=4"},"body":"Am 08.11.2016 um 16:22 schrieb Jonathan Word:\n> Proposal:\n> \n> Part 1) Add a .gitconfig variable to respect a read-only gitconfig\n> file and optional \"--force\" override option for the `git config`\n> command\n> \n> Such a gitconfig variable could be defined as:\n> config.respectFileMode: [ \"never\", \"allow-override\", \"always\" ]\n> [...]\n> Thoughts?\n\nI'd consider disrespecting file permissions to be a bug. Only very few tools allow to do so ('rm' is the only other one coming to mind right now), for good reason. If they do, only with additional parameters or by additional user interaction. Git should follow this strategy.\n\nWhich means: respect file permissions, no additional config variable and only if there's very substantial reason, add a --force. KISS.\n\nThat said, disrespecting permissions requires additional code, so it'd be interesting to know why this code was added. The relevant commit in the git.git repo should tell.\n\n\nMarkus\n\n-- \n- - - - - - - - - - - - - - - - - - -\nDipl. Ing. (FH) Markus Hitter\nhttp://www.jump-ing.de/\n"},{"id":"305563","messageId":"CAD9aWCgZkuaZNMDparVZE_WNFpOp7ud6iyCueGVbnU8s_EYtrQ@mail.gmail.com","threadId":"44441","inReplyTo":"40608c85-f870-87f7-daee-7fa98f5d19c1@jump-ing.de","subject":"Re: Bug: git config does not respect read-only .gitconfig file","fromName":"Jonathan Word","fromEmail":"argoday@argoday.com","sentAt":"2016-11-08T17:18:22Z","receivedAt":"2016-11-08T17:18:49Z","isPatch":false,"sender":{"key":"argoday@argoday.com","avatar":"https://gravatar.com/avatar/ce7cd96c8f84f3d9168000ad9b10f22dc7f02b04fe02900c5982f52716e2069e?d=mp&s=160"},"body":"I proposed a variant that would be fully backwards-compatible (don't\nknow who might rely on the functionality http://xkcd.com/1172/ )\nhowever I'd be happy to see the change without additional config +1\n... that's a call for this list as maintainers.\n\nThe root of the issue is that tempfile::rename_tempfile (\nhttps://github.com/git/git/blob/35f6318d44379452d8d33e880d8df0267b4a0cd0/tempfile.c#L288\n) relies on http://man7.org/linux/man-pages/man2/rename.2.html which,\nonly requires directory write permissions - not file write\npermissions. As you point out 'rm' is another example of this paradigm\nand it works exactly the same way.\n\nThe point of confusion to users ( / my team) is that `git config`\ngives the appearance of editing / modifying the .gitconfig file\nin-place (where file permissions would be respected) however the\nactual implementation performs the equivalent of a rm+mv which only\nrespects directory permissions.\n\nThe `git config` command is only one of many that leverage that\nrename_tempfile function, if opting to respect file-level permissions\nacross the board then the desired change is probably at that level\nrather than in config::git_config_set_multivar_in_file_gently which\nwould only add respect for file-level permissions to the one command.\n\nCheeers,\n\n\nOn Tue, Nov 8, 2016 at 11:49 AM, Markus Hitter <mah@jump-ing.de> wrote:\n> Am 08.11.2016 um 16:22 schrieb Jonathan Word:\n>> Proposal:\n>>\n>> Part 1) Add a .gitconfig variable to respect a read-only gitconfig\n>> file and optional \"--force\" override option for the `git config`\n>> command\n>>\n>> Such a gitconfig variable could be defined as:\n>> config.respectFileMode: [ \"never\", \"allow-override\", \"always\" ]\n>> [...]\n>> Thoughts?\n>\n> I'd consider disrespecting file permissions to be a bug. Only very few tools allow to do so ('rm' is the only other one coming to mind right now), for good reason. If they do, only with additional parameters or by additional user interaction. Git should follow this strategy.\n>\n> Which means: respect file permissions, no additional config variable and only if there's very substantial reason, add a --force. KISS.\n>\n> That said, disrespecting permissions requires additional code, so it'd be interesting to know why this code was added. The relevant commit in the git.git repo should tell.\n>\n>\n> Markus\n>\n> --\n> - - - - - - - - - - - - - - - - - - -\n> Dipl. Ing. (FH) Markus Hitter\n> http://www.jump-ing.de/\n"},{"id":"305566","messageId":"20161108200110.zvqdm2nlu5zxfyv5@sigill.intra.peff.net","threadId":"44441","inReplyTo":"CAD9aWCgZkuaZNMDparVZE_WNFpOp7ud6iyCueGVbnU8s_EYtrQ@mail.gmail.com","subject":"Re: Bug: git config does not respect read-only .gitconfig file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-08T20:01:10Z","receivedAt":"2016-11-08T20:01:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 08, 2016 at 12:18:22PM -0500, Jonathan Word wrote:\n\n> The point of confusion to users ( / my team) is that `git config`\n> gives the appearance of editing / modifying the .gitconfig file\n> in-place (where file permissions would be respected) however the\n> actual implementation performs the equivalent of a rm+mv which only\n> respects directory permissions.\n\nThe reason for the tmpfile/rename is that git-config actually takes a\ndot-lock on the file while writing it. Simultaneous writers are blocked,\nand simultaneous readers see an atomic view of the file (either the\nstate before or after the write, but never a half-written file).  Most\nof git's file-writes are done this way.\n\n> The `git config` command is only one of many that leverage that\n> rename_tempfile function, if opting to respect file-level permissions\n> across the board then the desired change is probably at that level\n> rather than in config::git_config_set_multivar_in_file_gently which\n> would only add respect for file-level permissions to the one command.\n\nI am not convinced this is a code problem and not simply a documentation\nissue, but if you wanted to add an option to try to respect file\npermissions, then yes, I agree it should be done across the board.\nProbably converting \"rename(from, to)\" to first check \"access(to,\nW_OK)\". That's racy, but it's the best we could do.\n\n-Peff\n"},{"id":"305608","messageId":"xmqqk2cdbg5v.fsf@gitster.mtv.corp.google.com","threadId":"44441","inReplyTo":"20161108200110.zvqdm2nlu5zxfyv5@sigill.intra.peff.net","subject":"Re: Bug: git config does not respect read-only .gitconfig file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-09T01:22:52Z","receivedAt":"2016-11-09T01:23:30Z","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> Probably converting \"rename(from, to)\" to first check \"access(to,\n> W_OK)\". That's racy, but it's the best we could do.\n\nHmph, if these (possibly problematic) callers are all following the\nusual \"lock, write to temp, rename\" pattern, perhaps the lock_file()\nfunction can have access(path, W_OK) check before it returns a\ntempfile that has been successfully opened?\n\nHaving said that, I share your assessment that this is not a code or\ndesign problem.  It is unreasonable to drop the write-enable bit of\na file in a writable directory and expect it to stay unmodified. The\nW-bit on the file is not usable as a security measure, and we do not\nuse it as such.\n\nI do not offhand know how much a new feature \"this repository can be\nmodified by pushing into and fetching from, but its configuration\ncannot be modified\" is a sensible thing to have.  But it is quite\nclear that even if we were to implement such feature, we wouldn't be\nusing W-bit on .git/config to signal that.\n\n"},{"id":"305610","messageId":"20161109033441.hp4eyf5qahimrtr3@sigill.intra.peff.net","threadId":"44441","inReplyTo":"xmqqk2cdbg5v.fsf@gitster.mtv.corp.google.com","subject":"Re: Bug: git config does not respect read-only .gitconfig file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-09T03:34:41Z","receivedAt":"2016-11-09T03:34:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 08, 2016 at 05:22:52PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Probably converting \"rename(from, to)\" to first check \"access(to,\n> > W_OK)\". That's racy, but it's the best we could do.\n> \n> Hmph, if these (possibly problematic) callers are all following the\n> usual \"lock, write to temp, rename\" pattern, perhaps the lock_file()\n> function can have access(path, W_OK) check before it returns a\n> tempfile that has been successfully opened?\n\nYeah, that is a lot friendlier, as it prevents the caller from doing\nwork (which may even involve the user typing things!) when it is clear\nthat we would fail the final step anyway.\n\n-Peff\n"},{"id":"305630","messageId":"CAD9aWCi5m_eJ==yC6_X-O_Xc+nyFczKkiEmqFSEAHKyNt2g-ZQ@mail.gmail.com","threadId":"44441","inReplyTo":"xmqqk2cdbg5v.fsf@gitster.mtv.corp.google.com","subject":"Re: Bug: git config does not respect read-only .gitconfig file","fromName":"Jonathan Word","fromEmail":"argoday@argoday.com","sentAt":"2016-11-09T13:51:57Z","receivedAt":"2016-11-09T13:52:24Z","isPatch":false,"sender":{"key":"argoday@argoday.com","avatar":"https://gravatar.com/avatar/ce7cd96c8f84f3d9168000ad9b10f22dc7f02b04fe02900c5982f52716e2069e?d=mp&s=160"},"body":"> It is unreasonable to drop the write-enable bit of\n> a file in a writable directory and expect it to stay unmodified. The\n> W-bit on the file is not usable as a security measure, and we do not\n> use it as such.\n\nThe point here is not a matter of security - it is of expectations.\n\nWhen a user drops write access on the global ~/.gitconfig I think\na reasonable user would expect future `git config --global` calls to\nfail by default. The possibility of an override is a different matter,\nand my initial proposal included the details of enabling direct\noverride. I don't think there is any presumption that this is a\nsecurity related discussion.\n\n> I do not offhand know how much a new feature \"this repository can be\n> modified by pushing into and fetching from, but its configuration\n> cannot be modified\" is a sensible thing to have.\n\nI agree that per-repository files almost never run into an issue with\nthis. Our problem is strictly with the global ~/.gitconfig which in our\nuse case is owned by a shared system account and used implicitly\nby many developers. Thus any one of those devs can call\n`git config` without any signal that they are changing something\nthat ought not to be changed and should think carefully.\n\nThis would be equivalent to dropping write access to a file that\nyour account owns so that vi / emacs / etc.. will warn that the\nfile is read-only before modifying it (useful for any number of\nsensitive files). Obviously from a security perspective you have\na number of means of potential override, however all require additional\nsteps that surface the initial intention that the file should not\nchange - or should only change rarely after additional confirmation.\n\n> perhaps the lock_file()\n> function can have access(path, W_OK) check before it returns a\n> tempfile that has been successfully opened?\n\nThat sounds ideal\n\nOn Tue, Nov 8, 2016 at 8:22 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> Probably converting \"rename(from, to)\" to first check \"access(to,\n>> W_OK)\". That's racy, but it's the best we could do.\n>\n> Hmph, if these (possibly problematic) callers are all following the\n> usual \"lock, write to temp, rename\" pattern, perhaps the lock_file()\n> function can have access(path, W_OK) check before it returns a\n> tempfile that has been successfully opened?\n>\n> Having said that, I share your assessment that this is not a code or\n> design problem.  It is unreasonable to drop the write-enable bit of\n> a file in a writable directory and expect it to stay unmodified. The\n> W-bit on the file is not usable as a security measure, and we do not\n> use it as such.\n>\n> I do not offhand know how much a new feature \"this repository can be\n> modified by pushing into and fetching from, but its configuration\n> cannot be modified\" is a sensible thing to have.  But it is quite\n> clear that even if we were to implement such feature, we wouldn't be\n> using W-bit on .git/config to signal that.\n>\n"}]}