{"thread":{"id":"33453","subject":"regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","startedAt":"2013-04-10T05:33:35Z","lastAt":"2013-05-25T11:35:25Z","messageCount":39,"participants":["Mike Galbraith","W. Trevor King","Jeff King","Junio C Hamano","Jonathan Nieder","Jason A. Donenfeld"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"213745","messageId":"1365572015.4658.51.camel@marge.simpson.net","threadId":"33453","inReplyTo":null,"subject":"regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Mike Galbraith","fromEmail":"bitbucket@online.de","sentAt":"2013-04-10T05:33:35Z","receivedAt":"2013-04-10T05:33:35Z","isPatch":false,"sender":{"key":"bitbucket@online.de","avatar":null},"body":"Greetings,\n\nI use git-daemon as the keeper of all source (love it). git is a normal\nuser, running as git:daemon, with all repositories living in ~git.\n\ngit-daemon is started like so:\n\n/usr/lib/git/git-daemon --syslog --detach --reuseaddr --user=git --group=daemon --pid-file=/var/run/git-daemon.pid --export-all --user-path --enable=receive-pack\n\nTry to pull as root or normal user results in:\n\n[pid 26786] access(\"/root/.config/git/config\", R_OK) = -1 EACCES (Permission denied)\n[pid 26786] write(2, \"fatal: unable to access '/root/.\"..., 70) = 70\n[pid 26785] <... read resumed> \"fatal: unable to access '/root/.\"..., 4096) = 70\n[pid 26786] exit_group(128)\n\nBisection fingered this commit, though it looks like it's really due to\nnot forgetting who it was at birth. It's not root, so has no business\nrummaging around in /root. It used to not care, but this commit made\n\"go away\" while looking for non-existent config file terminal.\n\n-Mike\n"},{"id":"213878","messageId":"20130410135605.GB4694@odin.tremily.us","threadId":"33453","inReplyTo":"1365572015.4658.51.camel@marge.simpson.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"W. Trevor King","fromEmail":"wking@tremily.us","sentAt":"2013-04-10T13:56:05Z","receivedAt":"2013-04-10T13:56:05Z","isPatch":false,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Wed, Apr 10, 2013 at 07:33:35AM +0200, Mike Galbraith wrote:\n> /usr/lib/git/git-daemon --syslog --detach --reuseaddr --user=git --group=daemon --pid-file=/var/run/git-daemon.pid --export-all --user-path --enable=receive-pack\n> \n> Try to pull as root or normal user results in:\n> \n> [pid 26786] access(\"/root/.config/git/config\", R_OK) = -1 EACCES (Permission denied)\n> [pid 26786] write(2, \"fatal: unable to access '/root/.\"..., 70) = 70\n> [pid 26785] <... read resumed> \"fatal: unable to access '/root/.\"..., 4096) = 70\n> [pid 26786] exit_group(128)\n> \n> Bisection fingered this commit, though it looks like it's really due to\n> not forgetting who it was at birth. It's not root, so has no business\n> rummaging around in /root. It used to not care, but this commit made\n> \"go away\" while looking for non-existent config file terminal.\n\nI ran into this too, although I'm running git-daemon via spawn-fcgi.\nIn order to convince newer Gits that you know what you're doing, you\njust need to set HOME to somewhere Git can look. For example:\n\n HOME=/ /usr/lib/git/git-daemon …\n\nshould work. On Gentoo, I added the following to\n/etc/conf.d/spawn-fcgi.fcgiwrap:\n\n ALLOWED_ENV=\"PATH HOME\"\n HOME=/\n\nCheers,\nTrevor\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"213892","messageId":"1365651583.19620.8.camel@marge.simpson.net","threadId":"33453","inReplyTo":"20130410135605.GB4694@odin.tremily.us","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Mike Galbraith","fromEmail":"bitbucket@online.de","sentAt":"2013-04-11T03:39:43Z","receivedAt":"2013-04-11T03:39:43Z","isPatch":false,"sender":{"key":"bitbucket@online.de","avatar":null},"body":"On Wed, 2013-04-10 at 09:56 -0400, W. Trevor King wrote: \n> On Wed, Apr 10, 2013 at 07:33:35AM +0200, Mike Galbraith wrote:\n> > /usr/lib/git/git-daemon --syslog --detach --reuseaddr --user=git --group=daemon --pid-file=/var/run/git-daemon.pid --export-all --user-path --enable=receive-pack\n> > \n> > Try to pull as root or normal user results in:\n> > \n> > [pid 26786] access(\"/root/.config/git/config\", R_OK) = -1 EACCES (Permission denied)\n> > [pid 26786] write(2, \"fatal: unable to access '/root/.\"..., 70) = 70\n> > [pid 26785] <... read resumed> \"fatal: unable to access '/root/.\"..., 4096) = 70\n> > [pid 26786] exit_group(128)\n> > \n> > Bisection fingered this commit, though it looks like it's really due to\n> > not forgetting who it was at birth. It's not root, so has no business\n> > rummaging around in /root. It used to not care, but this commit made\n> > \"go away\" while looking for non-existent config file terminal.\n> \n> I ran into this too, although I'm running git-daemon via spawn-fcgi.\n> In order to convince newer Gits that you know what you're doing, you\n> just need to set HOME to somewhere Git can look. For example:\n> \n> HOME=/ /usr/lib/git/git-daemon …\n> \n> should work. On Gentoo, I added the following to\n> /etc/conf.d/spawn-fcgi.fcgiwrap:\n> \n> ALLOWED_ENV=\"PATH HOME\"\n> HOME=/\n\nI can work around it by changing the init script to use su - git -c \"bla\nbla\" to launch the thing, instead of using --user=git --group=daemon,\nbut that's just a bandaid for the busted environment setup those\nswitches were supposed to make happen, no?\n\n-Mike\n"},{"id":"213904","messageId":"20130411054207.GE27795@sigill.intra.peff.net","threadId":"33453","inReplyTo":"1365651583.19620.8.camel@marge.simpson.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-11T05:42:07Z","receivedAt":"2013-04-11T05:42:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 11, 2013 at 05:39:43AM +0200, Mike Galbraith wrote:\n\n> > ALLOWED_ENV=\"PATH HOME\"\n> > HOME=/\n> \n> I can work around it by changing the init script to use su - git -c \"bla\n> bla\" to launch the thing, instead of using --user=git --group=daemon,\n> but that's just a bandaid for the busted environment setup those\n> switches were supposed to make happen, no?\n\nYeah, I think the bug here is that git-daemon should be setting $HOME\nwhen it switches privileges with --user. Does this patch fix it for you?\n\ndiff --git a/daemon.c b/daemon.c\nindex 6aeddcb..a4451fd 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1091,6 +1091,7 @@ static void drop_privileges(struct credentials *cred)\n \tif (cred && (initgroups(cred->pass->pw_name, cred->gid) ||\n \t setgid (cred->gid) || setuid(cred->pass->pw_uid)))\n \t\tdie(\"cannot drop privileges\");\n+\tsetenv(\"HOME\", cred->pass->pw_dir, 1);\n }\n \n static struct credentials *prepare_credentials(const char *user_name,\n\nI guess that would technically break anybody who was trying to do\nsomething clever with HOME (i.e., point it somewhere besides --user's\nHOME where they had put some config files). But the obvious clever\nthing would be to also set the user's passwd homedir to the same place.\n\n-Peff\n"},{"id":"213912","messageId":"1365667159.19620.116.camel@marge.simpson.net","threadId":"33453","inReplyTo":"20130411054207.GE27795@sigill.intra.peff.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Mike Galbraith","fromEmail":"bitbucket@online.de","sentAt":"2013-04-11T07:59:19Z","receivedAt":"2013-04-11T07:59:19Z","isPatch":false,"sender":{"key":"bitbucket@online.de","avatar":null},"body":"On Thu, 2013-04-11 at 01:42 -0400, Jeff King wrote: \n> On Thu, Apr 11, 2013 at 05:39:43AM +0200, Mike Galbraith wrote:\n> \n> > > ALLOWED_ENV=\"PATH HOME\"\n> > > HOME=/\n> > \n> > I can work around it by changing the init script to use su - git -c \"bla\n> > bla\" to launch the thing, instead of using --user=git --group=daemon,\n> > but that's just a bandaid for the busted environment setup those\n> > switches were supposed to make happen, no?\n> \n> Yeah, I think the bug here is that git-daemon should be setting $HOME\n> when it switches privileges with --user. Does this patch fix it for you?\n> \n> diff --git a/daemon.c b/daemon.c\n> index 6aeddcb..a4451fd 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -1091,6 +1091,7 @@ static void drop_privileges(struct credentials *cred)\n> \tif (cred && (initgroups(cred->pass->pw_name, cred->gid) ||\n> \t setgid (cred->gid) || setuid(cred->pass->pw_uid)))\n> \t\tdie(\"cannot drop privileges\");\n> +\tsetenv(\"HOME\", cred->pass->pw_dir, 1);\n> }\n> \n> static struct credentials *prepare_credentials(const char *user_name,\n> \n> I guess that would technically break anybody who was trying to do\n> something clever with HOME (i.e., point it somewhere besides --user's\n> HOME where they had put some config files). But the obvious clever\n> thing would be to also set the user's passwd homedir to the same place.\n\nI did exactly that yesterday, and it didn't work, which rather surprised\nme. Let me double check that I didn't screw trivial all up...\n\nHeh, if ya don't plunk new binary into the old oddly placed bucket :)\nYeah, that works just fine.\n\n-Mike \n"},{"id":"213962","messageId":"7vwqs9jd0t.fsf@alter.siamese.dyndns.org","threadId":"33453","inReplyTo":"20130411054207.GE27795@sigill.intra.peff.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-11T15:35:46Z","receivedAt":"2013-04-11T15:35:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Apr 11, 2013 at 05:39:43AM +0200, Mike Galbraith wrote:\n>\n>> > ALLOWED_ENV=\"PATH HOME\"\n>> > HOME=/\n>> \n>> I can work around it by changing the init script to use su - git -c \"bla\n>> bla\" to launch the thing, instead of using --user=git --group=daemon,\n>> but that's just a bandaid for the busted environment setup those\n>> switches were supposed to make happen, no?\n>\n> Yeah, I think the bug here is that git-daemon should be setting $HOME\n> when it switches privileges with --user. Does this patch fix it for you?\n>\n> diff --git a/daemon.c b/daemon.c\n> index 6aeddcb..a4451fd 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -1091,6 +1091,7 @@ static void drop_privileges(struct credentials *cred)\n> \tif (cred && (initgroups(cred->pass->pw_name, cred->gid) ||\n> \t setgid (cred->gid) || setuid(cred->pass->pw_uid)))\n> \t\tdie(\"cannot drop privileges\");\n> +\tsetenv(\"HOME\", cred->pass->pw_dir, 1);\n> }\n> \n> static struct credentials *prepare_credentials(const char *user_name,\n\nYeah, that sounds like the obvious fix to me.\n"},{"id":"213975","messageId":"20130411172424.GC1255@sigill.intra.peff.net","threadId":"33453","inReplyTo":"7vwqs9jd0t.fsf@alter.siamese.dyndns.org","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-11T17:24:25Z","receivedAt":"2013-04-11T17:24:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 11, 2013 at 08:35:46AM -0700, Junio C Hamano wrote:\n\n> > Yeah, I think the bug here is that git-daemon should be setting $HOME\n> > when it switches privileges with --user. Does this patch fix it for you?\n> [...]\n> Yeah, that sounds like the obvious fix to me.\n\nHere it is with a commit message.\n\n-- >8 --\nSubject: [PATCH] daemon: set HOME when we switch to --user\n\nIf git-daemon is invoked with the \"--user foo\" option, we\nsetuid and setgid to the \"foo\" user. However, we do not\ncurrently touch $HOME or any other environment variables.\n\nThis means that a git-daemon (and its git subprocesses)\ninvoked as root will look at ~root/.gitconfig,\n~root/.config/git, etc. This is probably not what the admin\nexpected; it would make more sense to load user-wide config\nfrom ~foo.\n\nTraditionally this wasn't that big a deal, as most sites do\nnot put config in either homedir (they would use the\nsystem-wide /etc/gitconfig if they wanted global config).\nHowever, since 96b9e0e (config: treat user and xdg config\npermission problems as errors, 2012-10-13), it is now an\nerror to try to read from an inaccessible config file (which\na file in ~root is very likely to be), meaning that\ngit-daemon will not run at all in such a case.\n\nWe can fix this by setting HOME appropriately when we switch\nusers. Note that this is a regression for any site that uses\n--user but depends on putting config in the $HOME of the\nuser invoking git-daemon. Since the original behavior was\nnever documented, and the new behavior is much more\nsensible, we can consider this a bugfix.\n\nReported-by: Mike Galbraith \nSigned-off-by: Jeff King \n---\nI don't have any problem calling this a bugfix and claiming that anyone\nwho was depending on the original behavior is stupid and wrong. But it\nshould probably get a prominent slot in the ReleaseNotes.\n\n daemon.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/daemon.c b/daemon.c\nindex 6aeddcb..a4451fd 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1091,6 +1091,7 @@ static void drop_privileges(struct credentials *cred)\n \tif (cred && (initgroups(cred->pass->pw_name, cred->gid) ||\n \t setgid (cred->gid) || setuid(cred->pass->pw_uid)))\n \t\tdie(\"cannot drop privileges\");\n+\tsetenv(\"HOME\", cred->pass->pw_dir, 1);\n }\n \n static struct credentials *prepare_credentials(const char *user_name,\n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213982","messageId":"20130411181103.GJ27070@google.com","threadId":"33453","inReplyTo":"20130411172424.GC1255@sigill.intra.peff.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-04-11T18:11:03Z","receivedAt":"2013-04-11T18:11:03Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Here it is with a commit message.\n>\n> -- >8 --\n> Subject: [PATCH] daemon: set HOME when we switch to --user\n\nThanks for taking care of it. For what it's worth,\n\nAcked-by: Jonathan Nieder \n\nI'm not sure whether to keep 96b9e0e (config: treat user and xdg\nconfig permission problem as errors) in the long run, BTW. There have\nbeen multiple reports about dropping privileges and not being able to\naccess the old HOME, and I'm not convinced any more that the\npredictability is worth the breakage for such people. Though checking\nif $HOME is inaccessible and treating that case specially would be\neven worse...\n\nInsights welcome.\n\nJonathan\n"},{"id":"213983","messageId":"20130411181439.GA2820@sigill.intra.peff.net","threadId":"33453","inReplyTo":"20130411181103.GJ27070@google.com","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-11T18:14:39Z","receivedAt":"2013-04-11T18:14:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 11, 2013 at 11:11:03AM -0700, Jonathan Nieder wrote:\n\n> > -- >8 --\n> > Subject: [PATCH] daemon: set HOME when we switch to --user\n> \n> Thanks for taking care of it. For what it's worth,\n> \n> Acked-by: Jonathan Nieder \n> \n> I'm not sure whether to keep 96b9e0e (config: treat user and xdg\n> config permission problem as errors) in the long run, BTW. There have\n> been multiple reports about dropping privileges and not being able to\n> access the old HOME, and I'm not convinced any more that the\n> predictability is worth the breakage for such people. Though checking\n> if $HOME is inaccessible and treating that case specially would be\n> even worse...\n> \n> Insights welcome.\n\nI could go either way. I think 96b9e0e is the right thing to do\nconceptually, but I kind of doubt it was affecting all that many people.\nAnd though it's _possible_ for it to be a security problem, I find it\nmuch more likely that the site admin tries to set some config, gets\nannoyed when it doesn't work, and debugs it. So from a practical\nperspective, 96b9e0e may be doing more harm than good, even though it's\nthe right thing.\n\n-Peff\n"},{"id":"213984","messageId":"20130411182547.GK27070@google.com","threadId":"33453","inReplyTo":"20130411181439.GA2820@sigill.intra.peff.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-04-11T18:25:47Z","receivedAt":"2013-04-11T18:25:47Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I could go either way. I think 96b9e0e is the right thing to do\n> conceptually, but I kind of doubt it was affecting all that many people.\n> And though it's _possible_ for it to be a security problem, I find it\n> much more likely that the site admin tries to set some config, gets\n> annoyed when it doesn't work, and debugs it. So from a practical\n> perspective, 96b9e0e may be doing more harm than good, even though it's\n> the right thing.\n\nOk. By the way, another commit to blame is v1.7.12.1~2^2~4 (config:\nwarn on inaccessible files, 2012-08-21), which made it into a warnable\noffense instead of just a strange but accepted configuration. ;-)\n\nI'm still leaning toward keeping v1.7.12.1~2^2~4 and 96b9e0e as being\nworth it, though I could be easily swayed in the other direction (for\nexample via a patch by an interested user with documentation that\nexplains how to debug and makes it unlikely for the behavior to keep\nflipping in the future). Thanks for spelling out the trade-offs.\n\nSincerely,\nJonathan\n"},{"id":"213999","messageId":"7vwqs8hmh1.fsf@alter.siamese.dyndns.org","threadId":"33453","inReplyTo":"20130411181439.GA2820@sigill.intra.peff.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-11T19:54:34Z","receivedAt":"2013-04-11T19:54:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Apr 11, 2013 at 11:11:03AM -0700, Jonathan Nieder wrote:\n>\n>> > -- >8 --\n>> > Subject: [PATCH] daemon: set HOME when we switch to --user\n>> \n>> Thanks for taking care of it. For what it's worth,\n>> \n>> Acked-by: Jonathan Nieder \n>> \n>> I'm not sure whether to keep 96b9e0e (config: treat user and xdg\n>> config permission problem as errors) in the long run, BTW. There have\n>> been multiple reports about dropping privileges and not being able to\n>> access the old HOME, and I'm not convinced any more that the\n>> predictability is worth the breakage for such people. Though checking\n>> if $HOME is inaccessible and treating that case specially would be\n>> even worse...\n>> \n>> Insights welcome.\n>\n> I could go either way. I think 96b9e0e is the right thing to do\n> conceptually, but I kind of doubt it was affecting all that many people.\n> And though it's _possible_ for it to be a security problem, I find it\n> much more likely that the site admin tries to set some config, gets\n> annoyed when it doesn't work, and debugs it. So from a practical\n> perspective, 96b9e0e may be doing more harm than good, even though it's\n> the right thing.\n\nRecent reports in this thread make us think so, I guess.\n\nBut reverting 96b9e0e alone would not help these people very much\nthough. They will have reams of warning messages in their server\nlogs, and the way to \"fix\" it would be the same as the way to work\naround the access_or_die(), namely, to set $HOME to point at a more\nappropriate place before running \"git daemon\".\n\nI also have a suspicion that your patch makes things worse for\npeople who are more adept at these issues around running daemons\nthan the people who introduced this problem in the first place (eh,\nthat's \"us\"). It is plausible that they may run multiple instances\nof \"initially root but setuid() to an unprivileged user\" daemons,\ngiving each of them a separate play area by setting $HOME to\ndifferent values, just for management's ease not necessarily for\nsecurity (hence sharing the same unprivileged user), which will be\nbroken by the patch that unconditionally overrides $HOME.\n\nA trade off to make things slightly easier for one sysadmin by\nmaking another thing impossible to do for another sysadmin does not\nsound like a good one.\n"},{"id":"214006","messageId":"20130411200330.GA15667@odin.tremily.us","threadId":"33453","inReplyTo":"7vwqs8hmh1.fsf@alter.siamese.dyndns.org","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"W. Trevor King","fromEmail":"wking@tremily.us","sentAt":"2013-04-11T20:03:30Z","receivedAt":"2013-04-11T20:03:30Z","isPatch":false,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Thu, Apr 11, 2013 at 12:54:34PM -0700, Junio C Hamano wrote:\n> I also have a suspicion that your patch makes things worse for\n> people who are more adept at these issues around running daemons\n> than the people who introduced this problem in the first place (eh,\n> that's \"us\")…\n> \n> A trade off to make things slightly easier for one sysadmin by\n> making another thing impossible to do for another sysadmin does not\n> sound like a good one.\n\nAs one of the less experienced folks tripped up by this issue, I think\nthat setting HOME explicitly before invoking the daemon is simple\nenough (which is why I just fixed my invocation and didn't post to the\nlist). The difficulty was figuring out why the daemon was dying in\nthe first place (which involved bisection for me as well). Maybe\nthere could be an additional note about HOME to flesh out:\n\n fatal: unable to access '/root/.config/git/config': Permission denied\n\nwhen there's an EACCES error for the per-user config? On the other\nhand, the code to handle this might be more trouble than it's worth.\n\nCheers,\nTrevor\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"214000","messageId":"20130411200818.GE1255@sigill.intra.peff.net","threadId":"33453","inReplyTo":"7vwqs8hmh1.fsf@alter.siamese.dyndns.org","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-11T20:08:18Z","receivedAt":"2013-04-11T20:08:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 11, 2013 at 12:54:34PM -0700, Junio C Hamano wrote:\n\n> > I could go either way. I think 96b9e0e is the right thing to do\n> > conceptually, but I kind of doubt it was affecting all that many people.\n> > And though it's _possible_ for it to be a security problem, I find it\n> > much more likely that the site admin tries to set some config, gets\n> > annoyed when it doesn't work, and debugs it. So from a practical\n> > perspective, 96b9e0e may be doing more harm than good, even though it's\n> > the right thing.\n> \n> Recent reports in this thread make us think so, I guess.\n> \n> But reverting 96b9e0e alone would not help these people very much\n> though. They will have reams of warning messages in their server\n> logs, and the way to \"fix\" it would be the same as the way to work\n> around the access_or_die(), namely, to set $HOME to point at a more\n> appropriate place before running \"git daemon\".\n\nYeah, if we revert 96b9e0e, it would only make sense to revert the\nwarnings, too. Going halfway does not help anyone.\n\n> I also have a suspicion that your patch makes things worse for\n> people who are more adept at these issues around running daemons\n> than the people who introduced this problem in the first place (eh,\n> that's \"us\"). It is plausible that they may run multiple instances\n> of \"initially root but setuid() to an unprivileged user\" daemons,\n> giving each of them a separate play area by setting $HOME to\n> different values, just for management's ease not necessarily for\n> security (hence sharing the same unprivileged user), which will be\n> broken by the patch that unconditionally overrides $HOME.\n\nYes, we would definitely be breaking them with this patch. I don't know\nhow common that is. As you noted, it is a bad idea security-wise (if\neverything runs as \"nobody\", then the services are not insulated from\neach other), but I can perhaps see a case where all git repos are owned\nby the \"git\" user, but they may be accessed by different config\nprofiles, which are managed by $HOME.\n\nYou could still accomplish the same thing with git by setting\nXDG_CONFIG_HOME, though that of course requires effort from the admin.\nSub-programs may not necessarily respect $XDG_CONFIG_HOME, though (e.g.,\nanything run from a post-receive hook). On the other hand, people do not\ngenerally push through git-daemon. But that feels like a weak argument.\n\n-Peff\n"},{"id":"214014","messageId":"7vip3shfpd.fsf@alter.siamese.dyndns.org","threadId":"33453","inReplyTo":"20130411200330.GA15667@odin.tremily.us","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-11T22:20:46Z","receivedAt":"2013-04-11T22:20:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"W. Trevor King\" writes:\n\n> As one of the less experienced folks tripped up by this issue, I think\n> that setting HOME explicitly before invoking the daemon is simple\n> enough (which is why I just fixed my invocation and didn't post to the\n> list).\n\nSounds like we need a documentation update somewhere.\n\n> The difficulty was figuring out why the daemon was dying in\n> the first place (which involved bisection for me as well). Maybe\n> there could be an additional note about HOME to flesh out:\n>\n> fatal: unable to access '/root/.config/git/config': Permission denied\n>\n> when there's an EACCES error for the per-user config?\n\nDoesn't access_or_die() say\n\n die_errno(_(\"unable to access '%s'\"), path);\n\nalready? I am puzzled...\n"},{"id":"214015","messageId":"20130411222301.GA11283@sigill.intra.peff.net","threadId":"33453","inReplyTo":"7vip3shfpd.fsf@alter.siamese.dyndns.org","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-11T22:23:01Z","receivedAt":"2013-04-11T22:23:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 11, 2013 at 03:20:46PM -0700, Junio C Hamano wrote:\n\n> \"W. Trevor King\" writes:\n> \n> > As one of the less experienced folks tripped up by this issue, I think\n> > that setting HOME explicitly before invoking the daemon is simple\n> > enough (which is why I just fixed my invocation and didn't post to the\n> > list).\n> \n> Sounds like we need a documentation update somewhere.\n\nYeah, I'd be happy to drop my patch if somebody wants to write a\ndocumentation update instead.\n\n> > The difficulty was figuring out why the daemon was dying in\n> > the first place (which involved bisection for me as well). Maybe\n> > there could be an additional note about HOME to flesh out:\n> >\n> > fatal: unable to access '/root/.config/git/config': Permission denied\n> >\n> > when there's an EACCES error for the per-user config?\n> \n> Doesn't access_or_die() say\n> \n> die_errno(_(\"unable to access '%s'\"), path);\n> \n> already? I am puzzled...\n\nI think the point is that it could add\n\n ...and I was looking in /root, because that is where your HOME points.\n Shouldn't you be able to read your own HOME directory?\n\nwhich should make it painfully obvious to the user what is going on.\n\n-Peff\n"},{"id":"214021","messageId":"20130412005748.GA17116@odin.tremily.us","threadId":"33453","inReplyTo":"20130411222301.GA11283@sigill.intra.peff.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"W. Trevor King","fromEmail":"wking@tremily.us","sentAt":"2013-04-12T00:57:48Z","receivedAt":"2013-04-12T00:57:48Z","isPatch":false,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Thu, Apr 11, 2013 at 06:23:01PM -0400, Jeff King wrote:\n> On Thu, Apr 11, 2013 at 03:20:46PM -0700, Junio C Hamano wrote:\n> > \"W. Trevor King\" writes:\n> > > The difficulty was figuring out why the daemon was dying in\n> > > the first place (which involved bisection for me as well). Maybe\n> > > there could be an additional note about HOME to flesh out:\n> > >\n> > > fatal: unable to access '/root/.config/git/config': Permission denied\n> > >\n> > > when there's an EACCES error for the per-user config?\n> > \n> > Doesn't access_or_die() say\n> > \n> > die_errno(_(\"unable to access '%s'\"), path);\n> > \n> > already? I am puzzled...\n> \n> I think the point is that it could add\n> \n> ...and I was looking in /root, because that is where your HOME points.\n> Shouldn't you be able to read your own HOME directory?\n> \n> which should make it painfully obvious to the user what is going on.\n\nThat's more or less what I had in mind. The 1.8.1.1 release notes\njust say:\n\n * When attempting to read the XDG-style $HOME/.config/git/config and\n finding that $HOME/.config/git is a file, we gave a wrong error\n message, instead of treating the case as \"a custom config file does\n not exist there\" and moving on.\n\nwithout saying anything about permission problems becoming errors, or\nnoting that oddball HOME configurations might cause problems. Since\nthe release notes are already out, a notice like this should probably\ngo somewhere else. However, this is a lot of hand holding to be\nprinted along side the error message… Since git-daemon (or gitweb) is\nthe most likely place for this problem to crop up, maybe a note in its\n(their) man pages would be a good idea? This thread may also be\nsufficient documentation, assuming good enough search engines ;).\n\nCheers,\nTrevor\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"214027","messageId":"7vvc7sfkwn.fsf@alter.siamese.dyndns.org","threadId":"33453","inReplyTo":"20130412005748.GA17116@odin.tremily.us","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-12T04:11:20Z","receivedAt":"2013-04-12T04:11:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"W. Trevor King\" writes:\n\n> On Thu, Apr 11, 2013 at 06:23:01PM -0400, Jeff King wrote:\n> ...\n>> I think the point is that it could add\n>> \n>> ...and I was looking in /root, because that is where your HOME points.\n>> Shouldn't you be able to read your own HOME directory?\n>> \n>> which should make it painfully obvious to the user what is going on.\n>\n> That's more or less what I had in mind.\n> ... However, this is a lot of hand holding to be\n> printed along side the error message… Since git-daemon (or gitweb) is\n> the most likely place for this problem to crop up, maybe a note in its\n> (their) man pages would be a good idea?\n\nThe --user option to git-daemon would be a good place to do that, I\nthink. Depending on what other \"setuid to less privileged before\nrunning\" programs do (I do not know offhand), we can say something\nlike this perhaps?\n\n --user::\n\t... current description ...\n +\n (Like|Unlike) many programs that let you run programs as\n specified user, the daemon does not reset environment variables\n such as $HOME when it runs git programs like upload-pack and\n receive-pack. Set and export HOME to point at the home directory\n of the user you specify with this option before you start the\n daemon, and make sure the Git configuration files in that\n directory is readable by that user.\n\nIf we have to say \"Unlike\" above, then we probably should bite the\nbullet and use Peff's patch, perhaps with an addition to the manual\npage, perhaps like this.\n\n --user::\n\t... current description ...\n +\n Like many programs that let you run programs as specified user,\n the daemon resets $HOME environment variable to that of the user\n you specify with this option when it runs git programs like\n upload-pack and receive-pack. Make sure that the Git\n configuration files in that directory is readable by that user.\n"},{"id":"214028","messageId":"20130412043501.GA12942@sigill.intra.peff.net","threadId":"33453","inReplyTo":"7vvc7sfkwn.fsf@alter.siamese.dyndns.org","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-12T04:35:01Z","receivedAt":"2013-04-12T04:35:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 11, 2013 at 09:11:20PM -0700, Junio C Hamano wrote:\n\n> The --user option to git-daemon would be a good place to do that, I\n> think. Depending on what other \"setuid to less privileged before\n> running\" programs do (I do not know offhand), we can say something\n> like this perhaps?\n\nThat's a good question. I looked at (just sampling a few off the top of\nmy head):\n\n xinetd\n openbsd-inetd\n inetutils-inetd\n postfix\n dovecot\n courier\n\nand none of them sets HOME when dropping privileges. Admittedly some of\nthem do not drop privileges immediately in the same way (e.g., the imap\nservers need to remain root so that they can switch to the right user to\nread mail). Postfix does set HOME, but only when actually \"becoming\" the\nuser to do deliveries, not at startup.\n\nI could also be wrong on one or more of those, as that is from some\nquick grepping, but I think it's clear that the norm is not to set HOME\nalongside setuid (of all of them, I would say git-daemon behaves most\nlike the inetd utils, as it does not ever \"become\" users at all).\n\n> --user::\n> \t... current description ...\n> +\n> (Like|Unlike) many programs that let you run programs as\n> specified user, the daemon does not reset environment variables\n> such as $HOME when it runs git programs like upload-pack and\n> receive-pack. Set and export HOME to point at the home directory\n> of the user you specify with this option before you start the\n> daemon, and make sure the Git configuration files in that\n> directory is readable by that user.\n\nSo choosing \"Like\" here, I think this makes sense.\n\n-Peff\n"},{"id":"214029","messageId":"7vr4igfj9w.fsf@alter.siamese.dyndns.org","threadId":"33453","inReplyTo":"20130412043501.GA12942@sigill.intra.peff.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-12T04:46:35Z","receivedAt":"2013-04-12T04:46:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Apr 11, 2013 at 09:11:20PM -0700, Junio C Hamano wrote:\n>\n>> The --user option to git-daemon would be a good place to do that, I\n>> think. Depending on what other \"setuid to less privileged before\n>> running\" programs do (I do not know offhand), we can say something\n>> like this perhaps?\n>\n> That's a good question. I looked at (just sampling a few off the top of\n> my head):\n>\n> xinetd\n> openbsd-inetd\n> inetutils-inetd\n> postfix\n> dovecot\n> courier\n>\n> and none of them sets HOME when dropping privileges. Admittedly some of\n> them do not drop privileges immediately in the same way (e.g., the imap\n> servers need to remain root so that they can switch to the right user to\n> read mail). Postfix does set HOME, but only when actually \"becoming\" the\n> user to do deliveries, not at startup.\n\nThanks for checking.\n\nFor $HOME, it is sufficient to do an unsetenv-equivalent, and I\nsuspect some of them do just that, though. Vanilla openbsd-inetd\ndoesn't, but Debian seems to have a patch to sanitize the\nenvironment, for example.\n\nBut still...\n\n> I could also be wrong on one or more of those, as that is from some\n> quick grepping, but I think it's clear that the norm is not to set HOME\n> alongside setuid (of all of them, I would say git-daemon behaves most\n> like the inetd utils, as it does not ever \"become\" users at all).\n>\n>> --user::\n>> \t... current description ...\n>> +\n>> (Like|Unlike) many programs that let you run programs as\n>> specified user, the daemon does not reset environment variables\n>> such as $HOME when it runs git programs like upload-pack and\n>> receive-pack. Set and export HOME to point at the home directory\n>> of the user you specify with this option before you start the\n>> daemon, and make sure the Git configuration files in that\n>> directory is readable by that user.\n>\n> So choosing \"Like\" here, I think this makes sense.\n\nI would prefer the simplicity ;-)\n\n\"Set and export HOME to point at the home directory of the user you\nspecify with this option\" screams that it wants to be rephrased at\nme, though. It somehow sounds as if this option is a way to set and\nexport the environment variable unless re-read carefully X-<.\n"},{"id":"214030","messageId":"20130412050550.GA15724@sigill.intra.peff.net","threadId":"33453","inReplyTo":"7vr4igfj9w.fsf@alter.siamese.dyndns.org","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-12T05:05:50Z","receivedAt":"2013-04-12T05:05:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 11, 2013 at 09:46:35PM -0700, Junio C Hamano wrote:\n\n> >> --user::\n> >> \t... current description ...\n> >> +\n> >> (Like|Unlike) many programs that let you run programs as\n> >> specified user, the daemon does not reset environment variables\n> >> such as $HOME when it runs git programs like upload-pack and\n> >> receive-pack. Set and export HOME to point at the home directory\n> >> of the user you specify with this option before you start the\n> >> daemon, and make sure the Git configuration files in that\n> >> directory is readable by that user.\n> >\n> > So choosing \"Like\" here, I think this makes sense.\n> \n> I would prefer the simplicity ;-)\n> \n> \"Set and export HOME to point at the home directory of the user you\n> specify with this option\" screams that it wants to be rephrased at\n> me, though. It somehow sounds as if this option is a way to set and\n> export the environment variable unless re-read carefully X-<.\n\nPerhaps:\n\n Like many programs that switch user id, the daemon does not reset\n environment variables such a `$HOME` when it runs git programs like\n `upload-pack` and `receive-pack`. When using this option, you may also\n want to set and export `HOME` to point at the home directory of\n `` before starting the daemon, and make sure the Git\n configuration file in that directory are readable by ``.\n\nI tried to address your concern above (which I agree with), smooth over\na few clunky wordings, and use \"\", which is defined in the heading\nof the option:\n\n --user=, --group=\n\n-Peff\n"},{"id":"214034","messageId":"1365745585.4539.18.camel@marge.simpson.net","threadId":"33453","inReplyTo":"20130412050550.GA15724@sigill.intra.peff.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Mike Galbraith","fromEmail":"bitbucket@online.de","sentAt":"2013-04-12T05:46:25Z","receivedAt":"2013-04-12T05:46:25Z","isPatch":false,"sender":{"key":"bitbucket@online.de","avatar":null},"body":"On Fri, 2013-04-12 at 01:05 -0400, Jeff King wrote: \n> On Thu, Apr 11, 2013 at 09:46:35PM -0700, Junio C Hamano wrote:\n> \n> > >> --user::\n> > >> \t... current description ...\n> > >> +\n> > >> (Like|Unlike) many programs that let you run programs as\n> > >> specified user, the daemon does not reset environment variables\n> > >> such as $HOME when it runs git programs like upload-pack and\n> > >> receive-pack. Set and export HOME to point at the home directory\n> > >> of the user you specify with this option before you start the\n> > >> daemon, and make sure the Git configuration files in that\n> > >> directory is readable by that user.\n> > >\n> > > So choosing \"Like\" here, I think this makes sense.\n> > \n> > I would prefer the simplicity ;-)\n> > \n> > \"Set and export HOME to point at the home directory of the user you\n> > specify with this option\" screams that it wants to be rephrased at\n> > me, though. It somehow sounds as if this option is a way to set and\n> > export the environment variable unless re-read carefully X-<.\n> \n> Perhaps:\n> \n> Like many programs that switch user id, the daemon does not reset\n> environment variables such a `$HOME` when it runs git programs like\n> `upload-pack` and `receive-pack`. When using this option, you may also\n> want to set and export `HOME` to point at the home directory of\n> `` before starting the daemon, and make sure the Git\n> configuration file in that directory are readable by ``.\n> \n> I tried to address your concern above (which I agree with), smooth over\n> a few clunky wordings, and use \"\", which is defined in the heading\n> of the option:\n> \n> --user=, --group=\n\nI just updated my local rpm to set HOME, so the surprise of no workee\nafter upgrade here is forever gone. Looks like suses (at least)\npackagers will need to do the same, else canned setup ain't gonna work.\n\n-Mike\n"},{"id":"214054","messageId":"20130412112636.GC20178@odin.tremily.us","threadId":"33453","inReplyTo":"20130412050550.GA15724@sigill.intra.peff.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"W. Trevor King","fromEmail":"wking@tremily.us","sentAt":"2013-04-12T11:26:36Z","receivedAt":"2013-04-12T11:26:36Z","isPatch":false,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Fri, Apr 12, 2013 at 01:05:50AM -0400, Jeff King wrote:\n> Like many programs that switch user id, the daemon does not reset\n> environment variables such a `$HOME` when it runs git programs like\n> `upload-pack` and `receive-pack`. When using this option, you may also\n> want to set and export `HOME` to point at the home directory of\n> `` before starting the daemon, and make sure the Git\n> configuration file in that directory are readable by ``.\n\nHow about \"and make sure any Git configuration files\", since there\nmight not be any Git configuration files.\n\nCheers,\nTrevor\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"214060","messageId":"20130412144855.GA17968@sigill.intra.peff.net","threadId":"33453","inReplyTo":"20130412112636.GC20178@odin.tremily.us","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-12T14:48:55Z","receivedAt":"2013-04-12T14:48:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 12, 2013 at 07:26:36AM -0400, W. Trevor King wrote:\n\n> On Fri, Apr 12, 2013 at 01:05:50AM -0400, Jeff King wrote:\n> > Like many programs that switch user id, the daemon does not reset\n> > environment variables such a `$HOME` when it runs git programs like\n> > `upload-pack` and `receive-pack`. When using this option, you may also\n> > want to set and export `HOME` to point at the home directory of\n> > `` before starting the daemon, and make sure the Git\n> > configuration file in that directory are readable by ``.\n> \n> How about \"and make sure any Git configuration files\", since there\n> might not be any Git configuration files.\n\nYeah, that is better. Thanks.\n\n-Peff\n"},{"id":"214068","messageId":"7vk3o7g29s.fsf@alter.siamese.dyndns.org","threadId":"33453","inReplyTo":"20130412144855.GA17968@sigill.intra.peff.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-12T16:08:31Z","receivedAt":"2013-04-12T16:08:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> How about \"and make sure any Git configuration files\", since there\n>> might not be any Git configuration files.\n>\n> Yeah, that is better. Thanks.\n\nOK, then...\n\n-- >8 --\nSubject: [PATCH] doc: clarify that \"git daemon --user=\" option does not export HOME=~user\n\nSigned-off-by: Jeff King \nHelped-by: W. Trevor King \nSigned-off-by: Junio C Hamano \n---\n Documentation/git-daemon.txt | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex 7e5098a..2ac07ba 100644\n--- a/Documentation/git-daemon.txt\n+++ b/Documentation/git-daemon.txt\n@@ -147,6 +147,13 @@ OPTIONS\n Giving these options is an error when used with `--inetd`; use\n the facility of inet daemon to achieve the same before spawning\n 'git daemon' if needed.\n++\n+Like many programs that switch user id, the daemon does not reset\n+environment variables such as `$HOME` when it runs git programs,\n+e.g. `upload-pack` and `receive-pack`. When using this option, you\n+may also want to set and export `HOME` to point at the home\n+directory of `` before starting the daemon, and make sure any\n+Git configuration files in that directory are readable by ``.\n \n --enable=::\n --disable=::\n-- \n1.8.2.1-472-g6c5785c\n"},{"id":"214072","messageId":"20130412161600.GA20492@sigill.intra.peff.net","threadId":"33453","inReplyTo":"7vk3o7g29s.fsf@alter.siamese.dyndns.org","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-12T16:16:01Z","receivedAt":"2013-04-12T16:16:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 12, 2013 at 09:08:31AM -0700, Junio C Hamano wrote:\n\n> OK, then...\n\n> -- >8 --\n> Subject: [PATCH] doc: clarify that \"git daemon --user=\" option does not export HOME=~user\n\nI'd add this motiviation to the body of the commit message:\n\n The fact that we don't set $HOME may confuse admins who\n expect $HOME/.gitconfig to be respected. And worse, since\n 96b9e0e3, a git-daemon started by root is likely to fail\n to run at all, as the user we switch to generally cannot\n read ~root.\n\nThis still feels ugly, like we are documenting some gotcha\nthat is going to hit most admins, when we could be helping\nthem in the code.\n\nOne option we have not explored is an environment variable\nto loosen git's requirement. I'm thinking something like\nGIT_INACCESSIBLE_HOMEDIR_OK, which could be set by default\nwhen git-daemon uses --user.\n\nThat would leave all existing setups working, but would\nstill enable the extra protections for people not running\ngit-daemon (and people who use git via sudo could choose to\nset it, too, if they would prefer that to setting up HOME).\n\n-Peff\n"},{"id":"214074","messageId":"1365783666.5814.21.camel@marge.simpson.net","threadId":"33453","inReplyTo":"7vk3o7g29s.fsf@alter.siamese.dyndns.org","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Mike Galbraith","fromEmail":"bitbucket@online.de","sentAt":"2013-04-12T16:21:06Z","receivedAt":"2013-04-12T16:21:06Z","isPatch":false,"sender":{"key":"bitbucket@online.de","avatar":null},"body":"On Fri, 2013-04-12 at 09:08 -0700, Junio C Hamano wrote: \n> Jeff King writes:\n> \n> >> How about \"and make sure any Git configuration files\", since there\n> >> might not be any Git configuration files.\n> >\n> > Yeah, that is better. Thanks.\n> \n> OK, then...\n> \n> -- >8 --\n> Subject: [PATCH] doc: clarify that \"git daemon --user=\" option does not export HOME=~user\n> \n> Signed-off-by: Jeff King \n> Helped-by: W. Trevor King \n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/git-daemon.txt | 7 +++++++\n> 1 file changed, 7 insertions(+)\n> \n> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\n> index 7e5098a..2ac07ba 100644\n> --- a/Documentation/git-daemon.txt\n> +++ b/Documentation/git-daemon.txt\n> @@ -147,6 +147,13 @@ OPTIONS\n> Giving these options is an error when used with `--inetd`; use\n> the facility of inet daemon to achieve the same before spawning\n> 'git daemon' if needed.\n> ++\n> +Like many programs that switch user id, the daemon does not reset\n> +environment variables such as `$HOME` when it runs git programs,\n> +e.g. `upload-pack` and `receive-pack`. When using this option, you\n> +may also want to set and export `HOME` to point at the home\n> +directory of `` before starting the daemon, and make sure any\n> +Git configuration files in that directory are readable by ``.\n\nThe \"you may want to..\" is perhaps a little understated given it will\nfail -EGOAWAY if git-daemon is started via init scripts if you don't.\n(but otoh, that's enough of a hint to anyone setting the thing up, no\nneed to write paragraphs of legal-beagle boiler-plate for dinky bug;)\n\n-Mike \n"},{"id":"214077","messageId":"20130412170505.GA2383@sigill.intra.peff.net","threadId":"33453","inReplyTo":"20130412161600.GA20492@sigill.intra.peff.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-12T17:05:05Z","receivedAt":"2013-04-12T17:05:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 12, 2013 at 12:16:00PM -0400, Jeff King wrote:\n\n> One option we have not explored is an environment variable\n> to loosen git's requirement. I'm thinking something like\n> GIT_INACCESSIBLE_HOMEDIR_OK, which could be set by default\n> when git-daemon uses --user.\n> \n> That would leave all existing setups working, but would\n> still enable the extra protections for people not running\n> git-daemon (and people who use git via sudo could choose to\n> set it, too, if they would prefer that to setting up HOME).\n\nSo here's what I came up with. I tried to make the exception as tight as\npossible by checking that $HOME was actually the problem, as that is the\ncommon problem (you switch users, but HOME is pointing to the old user).\n\nIt means that we would still die if ~root is readable, but\n~root/.gitconfig is not (or ~root/.config is not). I think that is\nprobably a good thing, though, as it is an indication of something more\ninteresting going on that the user should investigate.\n\nGiven how tight the exception is, I almost wonder if we should drop the\nenvironment variable completely, and just never complain about this case\n(in other words, just make it a loosening of 96b9e0e3).\n\n-Peff\n\n---\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 6a875f2..e004ee8 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -806,6 +806,15 @@ for further details.\n \ttemporarily to avoid using a buggy `/etc/gitconfig` file while\n \twaiting for someone with sufficient permissions to fix it.\n \n+'GIT_INACCESSIBLE_HOME_OK'::\n+\tNormally git will complain and die if it cannot access\n+\t`$HOME/.gitconfig`. If this variable is set to \"1\", then\n+\ta `$HOME` directory which cannot be accessed will not be\n+\tconsidered an error. This can be useful if you are switching\n+\tuser ids without resetting `$HOME`, and are willing to take the\n+\trisk that some configuration options might not be used (because\n+\tgit could not read the config file that contained them).\n+\n 'GIT_FLUSH'::\n \tIf this environment variable is set to \"1\", then commands such\n \tas 'git blame' (in incremental mode), 'git rev-list', 'git log',\ndiff --git a/cache.h b/cache.h\nindex e1e8ce8..01f300f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -358,6 +358,7 @@ static inline enum object_type object_type(unsigned int mode)\n #define GIT_NOTES_REWRITE_REF_ENVIRONMENT \"GIT_NOTES_REWRITE_REF\"\n #define GIT_NOTES_REWRITE_MODE_ENVIRONMENT \"GIT_NOTES_REWRITE_MODE\"\n #define GIT_LITERAL_PATHSPECS_ENVIRONMENT \"GIT_LITERAL_PATHSPECS\"\n+#define GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT \"GIT_INACCESSIBLE_HOME_OK\"\n \n /*\n * This environment variable is expected to contain a boolean indicating\ndiff --git a/daemon.c b/daemon.c\nindex 131b049..6c56cc0 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1091,7 +1091,7 @@ static void drop_privileges(struct credentials *cred)\n \tif (cred && (initgroups(cred->pass->pw_name, cred->gid) ||\n \t setgid (cred->gid) || setuid(cred->pass->pw_uid)))\n \t\tdie(\"cannot drop privileges\");\n-\tsetenv(\"GIT_CONFIG_INACCESSIBLE_HOME_OK\", \"1\", 0);\n+\tsetenv(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, \"1\", 0);\n }\n \n static struct credentials *prepare_credentials(const char *user_name,\ndiff --git a/wrapper.c b/wrapper.c\nindex bac59d2..644f867 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -416,11 +416,52 @@ int access_or_die(const char *path, int mode)\n \treturn ret;\n }\n \n+/*\n+ * Returns true iff the path is under $HOME, that directory is inaccessible,\n+ * and the user has told us through the environment that an inaccessible HOME\n+ * is OK. The results are cached so that repeated calls will not make multiple\n+ * syscalls.\n+ */\n+static int inaccessible_home_ok(const char *path)\n+{\n+\tstatic int gentle = -1;\n+\tstatic int home_inaccessible = -1;\n+\tconst char *home;\n+\n+\tif (gentle < 0)\n+\t\tgentle = git_env_bool(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, 0);\n+\tif (!gentle)\n+\t\treturn 0;\n+\n+\thome = getenv(\"HOME\");\n+\tif (!home)\n+\t\treturn 0;\n+\t/*\n+\t * We do not bother with normalizing PATHs to avoid symlinks\n+\t * here, since the point is to catch paths that are\n+\t * constructed as \"$HOME/...\".\n+\t */\n+\tif (prefixcmp(path, home) && path[strlen(home)] == '/')\n+\t\treturn 0;\n+\n+\tif (home_inaccessible < 0)\n+\t\thome_inaccessible = access(home, R_OK|X_OK) < 0 && errno == EACCES;\n+\treturn home_inaccessible;\n+}\n+\n int access_or_die(const char *path, int mode)\n {\n \tint ret = access(path, mode);\n-\tif (ret && errno != ENOENT && errno != ENOTDIR)\n+\tif (ret && errno != ENOENT && errno != ENOTDIR) {\n+\t\t/*\n+\t\t * We do not have to worry about clobbering errno\n+\t\t * in inaccessible_home_ok, because we get either EACCES\n+\t\t * again, or we die.\n+\t\t */\n+\t\tif (errno == EACCES && inaccessible_home_ok(path))\n+\t\t\treturn ret;\n \t\tdie_errno(_(\"unable to access '%s'\"), path);\n+\t}\n \treturn ret;\n }\n \n\n> \n> -Peff\n"},{"id":"214082","messageId":"7vr4ifejvv.fsf@alter.siamese.dyndns.org","threadId":"33453","inReplyTo":"20130412161600.GA20492@sigill.intra.peff.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-12T17:31:00Z","receivedAt":"2013-04-12T17:31:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Apr 12, 2013 at 09:08:31AM -0700, Junio C Hamano wrote:\n>\n>> OK, then...\n>\n>> -- >8 --\n>> Subject: [PATCH] doc: clarify that \"git daemon --user=\" option does not export HOME=~user\n>\n> I'd add this motiviation to the body of the commit message:\n>\n> The fact that we don't set $HOME may confuse admins who\n> expect $HOME/.gitconfig to be respected. And worse, since\n> 96b9e0e3, a git-daemon started by root is likely to fail\n> to run at all, as the user we switch to generally cannot\n> read ~root.\n>\n> This still feels ugly, like we are documenting some gotcha\n> that is going to hit most admins, when we could be helping\n> them in the code.\n\nI agree that it feels a bit wrong to sound as if we are blaming the\nmessanger (the one that notices a possible misconfiguration), but\nyou are correct that we should make a note on why we think it is a\ngood idea to add this piece of extra documentation in the history.\n\nWill add the above before queuing.\n\n> One option we have not explored is an environment variable\n> to loosen git's requirement. I'm thinking something like\n> GIT_INACCESSIBLE_HOMEDIR_OK, which could be set by default\n> when git-daemon uses --user.\n>\n> That would leave all existing setups working, but would\n> still enable the extra protections for people not running\n> git-daemon (and people who use git via sudo could choose to\n> set it, too, if they would prefer that to setting up HOME).\n\nPerhaps.\n\nRight now, the only case people noticed was that we complain when\nthe effective user cannot even tell if config file(s) exists or not.\nLabelling this option as \"Treat unreable as missing\" is fine, but\n\"an inaccessible homedir is OK\" is vastly different. Imagine a new\nversion where we start _requiring_ something to exist (and we read\nfrom it) and imagine further that the expected place of that thing\nis somewhere inside $HOME. We cannot keep the promise to those who\nset \"an inaccessible homedir is OK\" option when that happens, as we\nmay need that piece of information we wanted to read from there in\norder to properly operate.\n\nIn any case, I think the loosening is an independent issue.\n"},{"id":"214085","messageId":"7vbo9jehfx.fsf@alter.siamese.dyndns.org","threadId":"33453","inReplyTo":"20130412170505.GA2383@sigill.intra.peff.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-12T18:23:46Z","receivedAt":"2013-04-12T18:23:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So here's what I came up with. I tried to make the exception as tight as\n> possible by checking that $HOME was actually the problem, as that is the\n> common problem (you switch users, but HOME is pointing to the old user).\n> ...\n> diff --git a/daemon.c b/daemon.c\n> index 131b049..6c56cc0 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -1091,7 +1091,7 @@ static void drop_privileges(struct credentials *cred)\n> \tif (cred && (initgroups(cred->pass->pw_name, cred->gid) ||\n> \t setgid (cred->gid) || setuid(cred->pass->pw_uid)))\n> \t\tdie(\"cannot drop privileges\");\n> -\tsetenv(\"GIT_CONFIG_INACCESSIBLE_HOME_OK\", \"1\", 0);\n> +\tsetenv(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, \"1\", 0);\n> }\n\nCompared against an unpublished diffbase???\n\n> \n> static struct credentials *prepare_credentials(const char *user_name,\n> diff --git a/wrapper.c b/wrapper.c\n> index bac59d2..644f867 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -416,11 +416,52 @@ int access_or_die(const char *path, int mode)\n> \treturn ret;\n> }\n> \n> +/*\n> + * Returns true iff the path is under $HOME, that directory is inaccessible,\n> + * and the user has told us through the environment that an inaccessible HOME\n> + * is OK. The results are cached so that repeated calls will not make multiple\n> + * syscalls.\n> + */\n> +static int inaccessible_home_ok(const char *path)\n> +{\n> +\tstatic int gentle = -1;\n> +\tstatic int home_inaccessible = -1;\n> +\tconst char *home;\n> +\n> +\tif (gentle < 0)\n> +\t\tgentle = git_env_bool(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, 0);\n> +\tif (!gentle)\n> +\t\treturn 0;\n> +\n> +\thome = getenv(\"HOME\");\n> +\tif (!home)\n> +\t\treturn 0;\n> +\t/*\n> +\t * We do not bother with normalizing PATHs to avoid symlinks\n> +\t * here, since the point is to catch paths that are\n> +\t * constructed as \"$HOME/...\".\n> +\t */\n> +\tif (prefixcmp(path, home) && path[strlen(home)] == '/')\n> +\t\treturn 0;\n> +\n> +\tif (home_inaccessible < 0)\n> +\t\thome_inaccessible = access(home, R_OK|X_OK) < 0 && errno == EACCES;\n> +\treturn home_inaccessible;\n> +}\n> +\n> int access_or_die(const char *path, int mode)\n> {\n> \tint ret = access(path, mode);\n> -\tif (ret && errno != ENOENT && errno != ENOTDIR)\n> +\tif (ret && errno != ENOENT && errno != ENOTDIR) {\n> +\t\t/*\n> +\t\t * We do not have to worry about clobbering errno\n> +\t\t * in inaccessible_home_ok, because we get either EACCES\n> +\t\t * again, or we die.\n> +\t\t */\n> +\t\tif (errno == EACCES && inaccessible_home_ok(path))\n> +\t\t\treturn ret;\n\nOK, so the idea is\n\n - The environment can tell us to ignore permission errors for paths\n under $HOME if (and only if) $HOME itself is not readable;\n\n - We got a permission error here. inaccessible_home_ok() will tell\n us if the path is under $HOME and the above condition holds (in\n which case it will say \"ok, ignore that error\").\n\nwhich sounds good, but it relies on the caller of this function not\nto try actually reading from the path.\n\nIf the access() failed due to ENOENT, the caller will get a negative\nreturn from this function and will treat it as \"ok, it does not\nexist\", with the original or the updated code. This new case is\ntreated the same way by the existing callers, i.e. pretending as if\nthere is _no_ file in that unreadable $HOME directory.\n\nThat semantics sounds sane and safe to me.\n"},{"id":"214090","messageId":"20130412190152.GB4108@sigill.intra.peff.net","threadId":"33453","inReplyTo":"7vbo9jehfx.fsf@alter.siamese.dyndns.org","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-12T19:01:52Z","receivedAt":"2013-04-12T19:01:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 12, 2013 at 11:23:46AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > So here's what I came up with. I tried to make the exception as tight as\n> > possible by checking that $HOME was actually the problem, as that is the\n> > common problem (you switch users, but HOME is pointing to the old user).\n> > ...\n> > diff --git a/daemon.c b/daemon.c\n> > index 131b049..6c56cc0 100644\n> > --- a/daemon.c\n> > +++ b/daemon.c\n> > @@ -1091,7 +1091,7 @@ static void drop_privileges(struct credentials *cred)\n> > \tif (cred && (initgroups(cred->pass->pw_name, cred->gid) ||\n> > \t setgid (cred->gid) || setuid(cred->pass->pw_uid)))\n> > \t\tdie(\"cannot drop privileges\");\n> > -\tsetenv(\"GIT_CONFIG_INACCESSIBLE_HOME_OK\", \"1\", 0);\n> > +\tsetenv(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, \"1\", 0);\n> > }\n> \n> Compared against an unpublished diffbase???\n\nOops. Forgot I had made a WIP commit before running the diff.\n\n> OK, so the idea is\n> \n> - The environment can tell us to ignore permission errors for paths\n> under $HOME if (and only if) $HOME itself is not readable;\n> \n> - We got a permission error here. inaccessible_home_ok() will tell\n> us if the path is under $HOME and the above condition holds (in\n> which case it will say \"ok, ignore that error\").\n\nExactly.\n\n> which sounds good, but it relies on the caller of this function not\n> to try actually reading from the path.\n\nYes, but that is the only sane thing for the caller to do, since it gets\nthe same exit code from ENOENT and ENOTDIR already. Probably a comment\ndescribing the return value is in order.\n\n> If the access() failed due to ENOENT, the caller will get a negative\n> return from this function and will treat it as \"ok, it does not\n> exist\", with the original or the updated code. This new case is\n> treated the same way by the existing callers, i.e. pretending as if\n> there is _no_ file in that unreadable $HOME directory.\n\nExactly.\n\n> That semantics sounds sane and safe to me.\n\nThanks. I'll re-roll with a proper commit message and the fixups I\nmentioned above. I think we should still do the documentation for\ngit-daemon. But it is no longer about \"oops, we broke git-daemon\", but\n\"you may want know that we do not set HOME in case you are doing\nsomething tricky with config\". I'll submit that with the re-roll, too.\n\nDo you have an opinion on just dropping the environment variable\ncompletely and behaving this way all the time? It would \"just fix\" the\ncases people running into using su/sudo, too.\n\n-Peff\n"},{"id":"214093","messageId":"20130412191433.GR27070@google.com","threadId":"33453","inReplyTo":"20130412170505.GA2383@sigill.intra.peff.net","subject":"[PATCH] config: allow inaccessible configuration under $HOME","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-04-12T19:14:33Z","receivedAt":"2013-04-12T19:14:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"One unexpected downside to the changes v1.7.12.1~2^2~4 (config: warn\non inaccessible files, 2012-08-21) and v1.8.1.1~22^2~2 (config: treat\nuser and xdg config permission problems as errors, 2012-10-13) is that\nthey often trip when git is being run as a server. The appropriate\ndaemon (sshd, inetd, git-daemon, etc) starts as \"root\", creates a\nlistening socket, and then drops privileges, meaning that when git\ncommands are invoked they cannot access $HOME and die with\n\n fatal: unable to access '/root/.config/git/config': Permission denied\n\nThe intent was always to prevent important configuration (think\n\"[transfer] fsckobjects\") from being ignored when the configuration is\nunintentionally unreadable (for example with ENOMEM due to a DoS\nattack). But that is not worth breaking these cases of\ndrop-privileges-without-resetting-HOME that were working fine before.\n\nTreat user and xdg configuration that is inaccessible due to\npermissions (EACCES) as though no user configuration was provided at\nall.\n\nAn alternative approach would be to check if $HOME is readable, but\nthat would not solve the problem in cases where the user who dropped\nprivileges had a globally readable HOME with only .config or\n.gitconfig being private.\n\nThis does not change the behavior when \"git config\" is being used to\nwrite to a user's config file, when /etc/gitconfig or .git/config is\nunreadable (since those are more serious configuration errors), or\nwhen ~/.gitconfig or ~/.config/git is unreadable due to problems other\nthan permissions.\n\nThanks to Mike Galbraith, W. Trevor King, and Jeff King for their\npatient explanations.\n\nSigned-off-by: Jonathan Nieder \n---\nJeff King wrote:\n\n> Given how tight the exception is, I almost wonder if we should drop the\n> environment variable completely, and just never complain about this case\n> (in other words, just make it a loosening of 96b9e0e3).\n\nYeah, I think that would be better.\n\nHow about this? (Still needs tests.)\n\n builtin/config.c | 4 ++--\n config.c | 10 +++++-----\n dir.c | 4 ++--\n git-compat-util.h | 5 +++--\n wrapper.c | 10 ++++++----\n 5 files changed, 18 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 33c9bf9..19ffcaf 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -379,8 +379,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\t */\n \t\t\tdie(\"$HOME not set\");\n \n-\t\tif (access_or_warn(user_config, R_OK) &&\n-\t\t xdg_config && !access_or_warn(xdg_config, R_OK))\n+\t\tif (access_or_warn(user_config, R_OK, 0) &&\n+\t\t xdg_config && !access_or_warn(xdg_config, R_OK, 0))\n \t\t\tgiven_config_file = xdg_config;\n \t\telse\n \t\t\tgiven_config_file = user_config;\ndiff --git a/config.c b/config.c\nindex aefd80b..830ee14 100644\n--- a/config.c\n+++ b/config.c\n@@ -58,7 +58,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \t\tpath = buf.buf;\n \t}\n \n-\tif (!access_or_die(path, R_OK)) {\n+\tif (!access_or_die(path, R_OK, 0)) {\n \t\tif (++inc->depth > MAX_INCLUDE_DEPTH)\n \t\t\tdie(include_depth_advice, MAX_INCLUDE_DEPTH, path,\n \t\t\t cf && cf->name ? cf->name : \"the command line\");\n@@ -954,23 +954,23 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n \n \thome_config_paths(&user_config, &xdg_config, \"config\");\n \n-\tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK)) {\n+\tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {\n \t\tret += git_config_from_file(fn, git_etc_gitconfig(),\n \t\t\t\t\t data);\n \t\tfound += 1;\n \t}\n \n-\tif (xdg_config && !access_or_die(xdg_config, R_OK)) {\n+\tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {\n \t\tret += git_config_from_file(fn, xdg_config, data);\n \t\tfound += 1;\n \t}\n \n-\tif (user_config && !access_or_die(user_config, R_OK)) {\n+\tif (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) {\n \t\tret += git_config_from_file(fn, user_config, data);\n \t\tfound += 1;\n \t}\n \n-\tif (repo_config && !access_or_die(repo_config, R_OK)) {\n+\tif (repo_config && !access_or_die(repo_config, R_OK, 0)) {\n \t\tret += git_config_from_file(fn, repo_config, data);\n \t\tfound += 1;\n \t}\ndiff --git a/dir.c b/dir.c\nindex 91cfd99..9cb2f3d 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1637,9 +1637,9 @@ void setup_standard_excludes(struct dir_struct *dir)\n \t\thome_config_paths(NULL, &xdg_path, \"ignore\");\n \t\texcludes_file = xdg_path;\n \t}\n-\tif (!access_or_warn(path, R_OK))\n+\tif (!access_or_warn(path, R_OK, 0))\n \t\tadd_excludes_from_file(dir, path);\n-\tif (excludes_file && !access_or_warn(excludes_file, R_OK))\n+\tif (excludes_file && !access_or_warn(excludes_file, R_OK, 0))\n \t\tadd_excludes_from_file(dir, excludes_file);\n }\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex cde442f..51a29b8 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -691,8 +691,9 @@ int remove_or_warn(unsigned int mode, const char *path);\n * Call access(2), but warn for any error except \"missing file\"\n * (ENOENT or ENOTDIR).\n */\n-int access_or_warn(const char *path, int mode);\n-int access_or_die(const char *path, int mode);\n+#define ACCESS_EACCES_OK (1U << 0)\n+int access_or_warn(const char *path, int mode, unsigned flag);\n+int access_or_die(const char *path, int mode, unsigned flag);\n \n /* Warn on an inaccessible file that ought to be accessible */\n void warn_on_inaccessible(const char *path);\ndiff --git a/wrapper.c b/wrapper.c\nindex bac59d2..e860ad1 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -408,18 +408,20 @@ void warn_on_inaccessible(const char *path)\n \twarning(_(\"unable to access '%s': %s\"), path, strerror(errno));\n }\n \n-int access_or_warn(const char *path, int mode)\n+int access_or_warn(const char *path, int mode, unsigned flag)\n {\n \tint ret = access(path, mode);\n-\tif (ret && errno != ENOENT && errno != ENOTDIR)\n+\tif (ret && errno != ENOENT && errno != ENOTDIR &&\n+\t (!(flag & ACCESS_EACCES_OK) || errno != EACCES))\n \t\twarn_on_inaccessible(path);\n \treturn ret;\n }\n \n-int access_or_die(const char *path, int mode)\n+int access_or_die(const char *path, int mode, unsigned flag)\n {\n \tint ret = access(path, mode);\n-\tif (ret && errno != ENOENT && errno != ENOTDIR)\n+\tif (ret && errno != ENOENT && errno != ENOTDIR &&\n+\t (!(flag & ACCESS_EACCES_OK) || errno != EACCES))\n \t\tdie_errno(_(\"unable to access '%s'\"), path);\n \treturn ret;\n }\n-- \n1.8.2.1\n"},{"id":"214095","messageId":"20130412193755.GA5329@sigill.intra.peff.net","threadId":"33453","inReplyTo":"20130412191433.GR27070@google.com","subject":"Re: [PATCH] config: allow inaccessible configuration under $HOME","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-12T19:37:55Z","receivedAt":"2013-04-12T19:37:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 12, 2013 at 12:14:33PM -0700, Jonathan Nieder wrote:\n\n> One unexpected downside to the changes v1.7.12.1~2^2~4 (config: warn\n> on inaccessible files, 2012-08-21) and v1.8.1.1~22^2~2 (config: treat\n> user and xdg config permission problems as errors, 2012-10-13) is that\n> they often trip when git is being run as a server. The appropriate\n> daemon (sshd, inetd, git-daemon, etc) starts as \"root\", creates a\n> listening socket, and then drops privileges, meaning that when git\n> commands are invoked they cannot access $HOME and die with\n> \n> fatal: unable to access '/root/.config/git/config': Permission denied\n> \n> The intent was always to prevent important configuration (think\n> \"[transfer] fsckobjects\") from being ignored when the configuration is\n> unintentionally unreadable (for example with ENOMEM due to a DoS\n> attack). But that is not worth breaking these cases of\n> drop-privileges-without-resetting-HOME that were working fine before.\n> \n> Treat user and xdg configuration that is inaccessible due to\n> permissions (EACCES) as though no user configuration was provided at\n> all.\n\nI kind of wonder if we are doing anything with the check at this point.\nI suppose ENOMEM and EIO are the only interesting things left at this\npoint. The resulting code would be much nicer if the patch were just:\n\n -access_or_die(...);\n +access(...);\n\nbut I guess those conditions are still worth checking for, especially if\nwe think an attacker could trigger ENOMEM intentionally. To be honest, I\nam not sure how possible that is, but it is not that hard to do so.\n\n> An alternative approach would be to check if $HOME is readable, but\n> that would not solve the problem in cases where the user who dropped\n> privileges had a globally readable HOME with only .config or\n> .gitconfig being private.\n\nYeah, although I wonder if those are signs of a misconfiguration that\nshould be brought to the user's attention (~/.gitconfig I feel more\nconfident about; less so about $HOME/.config, which might be locked down\nfor reasons unrelated to git).\n\n> > Given how tight the exception is, I almost wonder if we should drop the\n> > environment variable completely, and just never complain about this case\n> > (in other words, just make it a loosening of 96b9e0e3).\n> \n> Yeah, I think that would be better.\n> \n> How about this? (Still needs tests.)\n\nI think it's probably OK. Like all of the proposed solutions, it is a\nbit of compromise about what is worth mentioning to the user and what is\nnot. But we cannot have our cake and eat it, too, so I'd be fine with\nthis.\n\nI agree a test would be nice for whatever solution we choose (both to\ncheck that we fail when we should, and do not when we should not).\n\n> -\tif (xdg_config && !access_or_die(xdg_config, R_OK)) {\n> +\tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {\n\nI know you are trying to be flexible with the flag, but I wonder if the\nresulting code would read better if we just added a new\n\"access_or_die_lenient\" helper, which would embody the wisdom of\nignoring EACCES, or anything else that comes up later. It seems like all\ncallers would want either the vanilla form or the lenient form.\n\nI do not feel too strongly about it, though.\n\n> -int access_or_warn(const char *path, int mode)\n> +int access_or_warn(const char *path, int mode, unsigned flag)\n> {\n> \tint ret = access(path, mode);\n> -\tif (ret && errno != ENOENT && errno != ENOTDIR)\n> +\tif (ret && errno != ENOENT && errno != ENOTDIR &&\n> +\t (!(flag & ACCESS_EACCES_OK) || errno != EACCES))\n> \t\twarn_on_inaccessible(path);\n> \treturn ret;\n> }\n> \n> -int access_or_die(const char *path, int mode)\n> +int access_or_die(const char *path, int mode, unsigned flag)\n> {\n> \tint ret = access(path, mode);\n> -\tif (ret && errno != ENOENT && errno != ENOTDIR)\n> +\tif (ret && errno != ENOENT && errno != ENOTDIR &&\n> +\t (!(flag & ACCESS_EACCES_OK) || errno != EACCES))\n> \t\tdie_errno(_(\"unable to access '%s'\"), path);\n> \treturn ret;\n> }\n\nNow that these conditions are getting more complex, we should perhaps\ncombine them, like:\n\n static int access_error_is_ok(int err, int flag)\n {\n return err == ENOENT || errno == ENOTDIR ||\n (flag & ACCESS_EACCES_OK && err == EACCES);\n }\n\n-Peff\n"},{"id":"214097","messageId":"7vr4ifcytk.fsf@alter.siamese.dyndns.org","threadId":"33453","inReplyTo":"20130412190152.GB4108@sigill.intra.peff.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-12T19:51:19Z","receivedAt":"2013-04-12T19:51:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> If the access() failed due to ENOENT, the caller will get a negative\n>> return from this function and will treat it as \"ok, it does not\n>> exist\", with the original or the updated code. This new case is\n>> treated the same way by the existing callers, i.e. pretending as if\n>> there is _no_ file in that unreadable $HOME directory.\n>\n> Exactly.\n\nThe explanation you are replying to was meant to illustrate how this\nis not \"inaccessible is OK\", but is \"treat inaccessible as missing\",\nby the way.\n\n>> That semantics sounds sane and safe to me.\n>\n> Thanks. I'll re-roll with a proper commit message and the fixups I\n> mentioned above. I think we should still do the documentation for\n> git-daemon. But it is no longer about \"oops, we broke git-daemon\", but\n> \"you may want know that we do not set HOME in case you are doing\n> something tricky with config\". I'll submit that with the re-roll, too.\n\nWell, at least to me, the documentation update was never about\n\"oops, we broke it\", but was about \"be careful where the HOME you\nare using actually is\" from the beginning of the suggestion. I was\nactually planning to apply it to maint-1.8.1 that predates the xdg\nstuff, and that is why the text only suggests to set HOME for the\nconfig.\n\n> Do you have an opinion on just dropping the environment variable\n> completely and behaving this way all the time? It would \"just fix\" the\n> cases people running into using su/sudo, too.\n\nWith the tightening, people who used --user=daemon, expecting that\nthey can later tweak the behaviour by touching ~daemon/.gitconfig,\ngot an early warning that they need to set HOME themselves, but with\nany variant of the patch under discussion, as long as loosening is\non by default, will no longer get that benefit.\n\nI am not yet convinced if that is a real \"fix/cure\".\n\nSo, no, I have not even reached the point where I can form an\nopinion if this behaviour should be the default.\n"},{"id":"214098","messageId":"20130412195854.GA7795@sigill.intra.peff.net","threadId":"33453","inReplyTo":"7vr4ifcytk.fsf@alter.siamese.dyndns.org","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-12T19:58:54Z","receivedAt":"2013-04-12T19:58:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 12, 2013 at 12:51:19PM -0700, Junio C Hamano wrote:\n\n> >> If the access() failed due to ENOENT, the caller will get a negative\n> >> return from this function and will treat it as \"ok, it does not\n> >> exist\", with the original or the updated code. This new case is\n> >> treated the same way by the existing callers, i.e. pretending as if\n> >> there is _no_ file in that unreadable $HOME directory.\n> >\n> > Exactly.\n> \n> The explanation you are replying to was meant to illustrate how this\n> is not \"inaccessible is OK\", but is \"treat inaccessible as missing\",\n> by the way.\n\nAh, I see the distinction you were making. Yes, that is what I was\nthinking (and what the patch does); I just used the word \"OK\" instead.\n\n> Well, at least to me, the documentation update was never about\n> \"oops, we broke it\", but was about \"be careful where the HOME you\n> are using actually is\" from the beginning of the suggestion. I was\n> actually planning to apply it to maint-1.8.1 that predates the xdg\n> stuff, and that is why the text only suggests to set HOME for the\n> config.\n\nYes; I think the only change needed would be to the commit message I\nproposed (if you even picked that up; I didn't look).\n\n> > Do you have an opinion on just dropping the environment variable\n> > completely and behaving this way all the time? It would \"just fix\" the\n> > cases people running into using su/sudo, too.\n> \n> With the tightening, people who used --user=daemon, expecting that\n> they can later tweak the behaviour by touching ~daemon/.gitconfig,\n> got an early warning that they need to set HOME themselves, but with\n> any variant of the patch under discussion, as long as loosening is\n> on by default, will no longer get that benefit.\n> \n> I am not yet convinced if that is a real \"fix/cure\".\n> \n> So, no, I have not even reached the point where I can form an\n> opinion if this behaviour should be the default.\n\nOK. I'll hold off for now while we stew on it. Jonathan's patch looks OK\nto me, but it has the same issue. But I think every path has to be one\nof:\n\n 1. We annoy sysadmins who need to take an extra step to handle the\n HOME situation with --user (the current behavior, or any other\n proposal that they have to opt into).\n\n 2. We annoy sysadmins who want to set HOME with --user, either by\n making what they want to do impossible, or making them set an extra\n variable or option to accomplish what used to work (my patch to set\n HOME with --user).\n\n 3. We loosen the check, so some cases which might be noteworthy are\n not caught (my patch, Jonathan's patch, etc).\n\nI think any solution will have to fall into one of those slots. So we\nneed to pick the least evil one, and then hammer out its least evil\nform.\n\n-Peff\n"},{"id":"214099","messageId":"20130412203442.GT27070@google.com","threadId":"33453","inReplyTo":"20130412193755.GA5329@sigill.intra.peff.net","subject":"[PATCH] fixup! config: allow inaccessible configuration under $HOME","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-04-12T20:34:42Z","receivedAt":"2013-04-12T20:34:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"A cleanup from Jeff King.\n\nSigned-off-by: Jonathan Nieder \n---\n wrapper.c | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\nJeff King wrote:\n\n> I kind of wonder if we are doing anything with the check at this point.\n> I suppose ENOMEM and EIO are the only interesting things left at this\n> point. The resulting code would be much nicer if the patch were just:\n>\n> -access_or_die(...);\n> +access(...);\n>\n> but I guess those conditions are still worth checking for, especially if\n> we think an attacker could trigger ENOMEM intentionally. To be honest, I\n> am not sure how possible that is, but it is not that hard to do so.\n\nYeah, I was tempted to go the plain access() route. The case that\nconvinced me not to is that I wouldn't want to think about whether\nthere could have been a blip in $HOME producing EIO when wondering how\nsome malformed object had been accepted. The ENOMEM case just seemed\nsimpler to explain.\n\nI would also be surprised if it is easy to control kernel ENOMEM\ncarefully enough to exploit (a more likely DoS outcome is crashing\nprograms or a kernel assertion failure, which are more harmless in\ntheir way). I've given up on predicting what is too hard or easy\nenough for security folks. I couldn't think of an obviously easier\nvulnerability that is already accepted to put my mind at ease.\n\n[...]\n> I know you are trying to be flexible with the flag,\n\nI was mostly worried about damage to readability if we end up needing\nto go further down this direction. The opportunity to look at all\ncall sites was also nice.\n\n[...]\n> static int access_error_is_ok(int err, int flag)\n> {\n> return err == ENOENT || errno == ENOTDIR ||\n> (flag & ACCESS_EACCES_OK && err == EACCES);\n> }\n\nNice. Here's a patch for squashing in.\n\ndiff --git a/wrapper.c b/wrapper.c\nindex e860ad1..29a866b 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -408,11 +408,16 @@ void warn_on_inaccessible(const char *path)\n \twarning(_(\"unable to access '%s': %s\"), path, strerror(errno));\n }\n \n+static int access_error_is_ok(int err, unsigned flag)\n+{\n+\treturn errno == ENOENT || errno == ENOTDIR ||\n+\t\t((flag & ACCESS_EACCES_OK) && errno == EACCES);\n+}\n+\n int access_or_warn(const char *path, int mode, unsigned flag)\n {\n \tint ret = access(path, mode);\n-\tif (ret && errno != ENOENT && errno != ENOTDIR &&\n-\t (!(flag & ACCESS_EACCES_OK) || errno != EACCES))\n+\tif (ret && !access_error_is_ok(errno, flag))\n \t\twarn_on_inaccessible(path);\n \treturn ret;\n }\n@@ -420,8 +425,7 @@ int access_or_warn(const char *path, int mode, unsigned flag)\n int access_or_die(const char *path, int mode, unsigned flag)\n {\n \tint ret = access(path, mode);\n-\tif (ret && errno != ENOENT && errno != ENOTDIR &&\n-\t (!(flag & ACCESS_EACCES_OK) || errno != EACCES))\n+\tif (ret && !access_error_is_ok(errno, flag))\n \t\tdie_errno(_(\"unable to access '%s'\"), path);\n \treturn ret;\n }\n-- \n1.8.2.1\n"},{"id":"214101","messageId":"7vli8ncwao.fsf@alter.siamese.dyndns.org","threadId":"33453","inReplyTo":"20130412195854.GA7795@sigill.intra.peff.net","subject":"Re: regression: \"96b9e0e3 config: treat user and xdg config permission problems as errors\" busted git-daemon","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-12T20:45:51Z","receivedAt":"2013-04-12T20:45:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> OK. I'll hold off for now while we stew on it. Jonathan's patch looks OK\n> to me, but it has the same issue. But I think every path has to be one\n> of:\n>\n> 1. We annoy sysadmins who need to take an extra step to handle the\n> HOME situation with --user (the current behavior, or any other\n> proposal that they have to opt into).\n>\n> 2. We annoy sysadmins who want to set HOME with --user, either by\n> making what they want to do impossible, or making them set an extra\n> variable or option to accomplish what used to work (my patch to set\n> HOME with --user).\n>\n> 3. We loosen the check, so some cases which might be noteworthy are\n> not caught (my patch, Jonathan's patch, etc).\n>\n> I think any solution will have to fall into one of those slots. So we\n> need to pick the least evil one, and then hammer out its least evil\n> form.\n\nThat summarizes our options nicely, I think. Thanks.\n"},{"id":"214102","messageId":"20130412210318.GU27070@google.com","threadId":"33453","inReplyTo":"20130412203442.GT27070@google.com","subject":"[PATCH v2] config: allow inaccessible configuration under $HOME","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-04-12T21:03:18Z","receivedAt":"2013-04-12T21:03:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The changes v1.7.12.1~2^2~4 (config: warn on inaccessible files,\n2012-08-21) and v1.8.1.1~22^2~2 (config: treat user and xdg config\npermission problems as errors, 2012-10-13) were intended to prevent\nimportant configuration (think \"[transfer] fsckobjects\") from being\nignored when the configuration is unintentionally unreadable (for\nexample with EIO on a flaky filesystem, or with ENOMEM due to a DoS\nattack). Usually ~/.gitconfig and ~/.config/git are readable by the\ncurrent user, and if they aren't then it would be easy to fix those\npermissions, so the damage from adding this check should have been\nminimal.\n\nUnfortunately the access() check often trips when git is being run as\na server. A daemon (such as inetd or git-daemon) starts as \"root\",\ncreates a listening socket, and then drops privileges, meaning that\nwhen git commands are invoked they cannot access $HOME and die with\n\n fatal: unable to access '/root/.config/git/config': Permission denied\n\nAny patch to fix this would have one of three problems:\n\n 1. We annoy sysadmins who need to take an extra step to handle HOME\n when dropping privileges (the current behavior, or any other\n proposal that they have to opt into).\n\n 2. We annoy sysadmins who want to set HOME when dropping privileges,\n either by making what they want to do impossible, or making them\n set an extra variable or option to accomplish what used to work\n (e.g., a patch to git-daemon to set HOME when --user is passed).\n\n 3. We loosen the check, so some cases which might be noteworthy are\n not caught.\n\nThis patch is of type (3).\n\nTreat user and xdg configuration that are inaccessible due to\npermissions (EACCES) as though no user configuration was provided at\nall.\n\nAn alternative method would be to check if $HOME is readable, but that\nwould not help in cases where the user who dropped privileges had a\nglobally readable HOME with only .config or .gitconfig being private.\n\nThis does not change the behavior when /etc/gitconfig or .git/config\nis unreadable (since those are more serious configuration errors),\nnor when ~/.gitconfig or ~/.config/git is unreadable due to problems\nother than permissions.\n\nSigned-off-by: Jonathan Nieder \nImproved-by: Jeff King \n---\nJonathan Nieder wrote:\n\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -408,11 +408,16 @@ void warn_on_inaccessible(const char *path)\n> \twarning(_(\"unable to access '%s': %s\"), path, strerror(errno));\n> }\n> \n> +static int access_error_is_ok(int err, unsigned flag)\n> +{\n> +\treturn errno == ENOENT || errno == ENOTDIR ||\n\nSigh, I can't spell \"err\". Here's a v2 incorporating that change and\nwith commit message incorporating the latest discussion.\n\n builtin/config.c | 4 ++--\n config.c | 10 +++++-----\n dir.c | 4 ++--\n git-compat-util.h | 5 +++--\n wrapper.c | 14 ++++++++++----\n 5 files changed, 22 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 33c9bf9..19ffcaf 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -379,8 +379,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\t */\n \t\t\tdie(\"$HOME not set\");\n \n-\t\tif (access_or_warn(user_config, R_OK) &&\n-\t\t xdg_config && !access_or_warn(xdg_config, R_OK))\n+\t\tif (access_or_warn(user_config, R_OK, 0) &&\n+\t\t xdg_config && !access_or_warn(xdg_config, R_OK, 0))\n \t\t\tgiven_config_file = xdg_config;\n \t\telse\n \t\t\tgiven_config_file = user_config;\ndiff --git a/config.c b/config.c\nindex aefd80b..830ee14 100644\n--- a/config.c\n+++ b/config.c\n@@ -58,7 +58,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \t\tpath = buf.buf;\n \t}\n \n-\tif (!access_or_die(path, R_OK)) {\n+\tif (!access_or_die(path, R_OK, 0)) {\n \t\tif (++inc->depth > MAX_INCLUDE_DEPTH)\n \t\t\tdie(include_depth_advice, MAX_INCLUDE_DEPTH, path,\n \t\t\t cf && cf->name ? cf->name : \"the command line\");\n@@ -954,23 +954,23 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n \n \thome_config_paths(&user_config, &xdg_config, \"config\");\n \n-\tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK)) {\n+\tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {\n \t\tret += git_config_from_file(fn, git_etc_gitconfig(),\n \t\t\t\t\t data);\n \t\tfound += 1;\n \t}\n \n-\tif (xdg_config && !access_or_die(xdg_config, R_OK)) {\n+\tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {\n \t\tret += git_config_from_file(fn, xdg_config, data);\n \t\tfound += 1;\n \t}\n \n-\tif (user_config && !access_or_die(user_config, R_OK)) {\n+\tif (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) {\n \t\tret += git_config_from_file(fn, user_config, data);\n \t\tfound += 1;\n \t}\n \n-\tif (repo_config && !access_or_die(repo_config, R_OK)) {\n+\tif (repo_config && !access_or_die(repo_config, R_OK, 0)) {\n \t\tret += git_config_from_file(fn, repo_config, data);\n \t\tfound += 1;\n \t}\ndiff --git a/dir.c b/dir.c\nindex 91cfd99..9cb2f3d 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1637,9 +1637,9 @@ void setup_standard_excludes(struct dir_struct *dir)\n \t\thome_config_paths(NULL, &xdg_path, \"ignore\");\n \t\texcludes_file = xdg_path;\n \t}\n-\tif (!access_or_warn(path, R_OK))\n+\tif (!access_or_warn(path, R_OK, 0))\n \t\tadd_excludes_from_file(dir, path);\n-\tif (excludes_file && !access_or_warn(excludes_file, R_OK))\n+\tif (excludes_file && !access_or_warn(excludes_file, R_OK, 0))\n \t\tadd_excludes_from_file(dir, excludes_file);\n }\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex cde442f..51a29b8 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -691,8 +691,9 @@ int remove_or_warn(unsigned int mode, const char *path);\n * Call access(2), but warn for any error except \"missing file\"\n * (ENOENT or ENOTDIR).\n */\n-int access_or_warn(const char *path, int mode);\n-int access_or_die(const char *path, int mode);\n+#define ACCESS_EACCES_OK (1U << 0)\n+int access_or_warn(const char *path, int mode, unsigned flag);\n+int access_or_die(const char *path, int mode, unsigned flag);\n \n /* Warn on an inaccessible file that ought to be accessible */\n void warn_on_inaccessible(const char *path);\ndiff --git a/wrapper.c b/wrapper.c\nindex bac59d2..dd7ecbb 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -408,18 +408,24 @@ void warn_on_inaccessible(const char *path)\n \twarning(_(\"unable to access '%s': %s\"), path, strerror(errno));\n }\n \n-int access_or_warn(const char *path, int mode)\n+static int access_error_is_ok(int err, unsigned flag)\n+{\n+\treturn err == ENOENT || err == ENOTDIR ||\n+\t\t((flag & ACCESS_EACCES_OK) && err == EACCES);\n+}\n+\n+int access_or_warn(const char *path, int mode, unsigned flag)\n {\n \tint ret = access(path, mode);\n-\tif (ret && errno != ENOENT && errno != ENOTDIR)\n+\tif (ret && !access_error_is_ok(errno, flag))\n \t\twarn_on_inaccessible(path);\n \treturn ret;\n }\n \n-int access_or_die(const char *path, int mode)\n+int access_or_die(const char *path, int mode, unsigned flag)\n {\n \tint ret = access(path, mode);\n-\tif (ret && errno != ENOENT && errno != ENOTDIR)\n+\tif (ret && !access_error_is_ok(errno, flag))\n \t\tdie_errno(_(\"unable to access '%s'\"), path);\n \treturn ret;\n }\n-- \n1.8.2.1\n"},{"id":"214120","messageId":"1365827296.4643.9.camel@marge.simpson.net","threadId":"33453","inReplyTo":"20130412210318.GU27070@google.com","subject":"Re: [PATCH v2] config: allow inaccessible configuration under $HOME","fromName":"Mike Galbraith","fromEmail":"bitbucket@online.de","sentAt":"2013-04-13T04:28:16Z","receivedAt":"2013-04-13T04:28:16Z","isPatch":true,"sender":{"key":"bitbucket@online.de","avatar":null},"body":"Tested, original setup works fine.\n\nOn Fri, 2013-04-12 at 14:03 -0700, Jonathan Nieder wrote: \n> The changes v1.7.12.1~2^2~4 (config: warn on inaccessible files,\n> 2012-08-21) and v1.8.1.1~22^2~2 (config: treat user and xdg config\n> permission problems as errors, 2012-10-13) were intended to prevent\n> important configuration (think \"[transfer] fsckobjects\") from being\n> ignored when the configuration is unintentionally unreadable (for\n> example with EIO on a flaky filesystem, or with ENOMEM due to a DoS\n> attack). Usually ~/.gitconfig and ~/.config/git are readable by the\n> current user, and if they aren't then it would be easy to fix those\n> permissions, so the damage from adding this check should have been\n> minimal.\n> \n> Unfortunately the access() check often trips when git is being run as\n> a server. A daemon (such as inetd or git-daemon) starts as \"root\",\n> creates a listening socket, and then drops privileges, meaning that\n> when git commands are invoked they cannot access $HOME and die with\n> \n> fatal: unable to access '/root/.config/git/config': Permission denied\n> \n> Any patch to fix this would have one of three problems:\n> \n> 1. We annoy sysadmins who need to take an extra step to handle HOME\n> when dropping privileges (the current behavior, or any other\n> proposal that they have to opt into).\n> \n> 2. We annoy sysadmins who want to set HOME when dropping privileges,\n> either by making what they want to do impossible, or making them\n> set an extra variable or option to accomplish what used to work\n> (e.g., a patch to git-daemon to set HOME when --user is passed).\n> \n> 3. We loosen the check, so some cases which might be noteworthy are\n> not caught.\n> \n> This patch is of type (3).\n> \n> Treat user and xdg configuration that are inaccessible due to\n> permissions (EACCES) as though no user configuration was provided at\n> all.\n> \n> An alternative method would be to check if $HOME is readable, but that\n> would not help in cases where the user who dropped privileges had a\n> globally readable HOME with only .config or .gitconfig being private.\n> \n> This does not change the behavior when /etc/gitconfig or .git/config\n> is unreadable (since those are more serious configuration errors),\n> nor when ~/.gitconfig or ~/.config/git is unreadable due to problems\n> other than permissions.\n> \n> Signed-off-by: Jonathan Nieder \n> Improved-by: Jeff King \n> ---\n> Jonathan Nieder wrote:\n> \n> > --- a/wrapper.c\n> > +++ b/wrapper.c\n> > @@ -408,11 +408,16 @@ void warn_on_inaccessible(const char *path)\n> > \twarning(_(\"unable to access '%s': %s\"), path, strerror(errno));\n> > }\n> > \n> > +static int access_error_is_ok(int err, unsigned flag)\n> > +{\n> > +\treturn errno == ENOENT || errno == ENOTDIR ||\n> \n> Sigh, I can't spell \"err\". Here's a v2 incorporating that change and\n> with commit message incorporating the latest discussion.\n> \n> builtin/config.c | 4 ++--\n> config.c | 10 +++++-----\n> dir.c | 4 ++--\n> git-compat-util.h | 5 +++--\n> wrapper.c | 14 ++++++++++----\n> 5 files changed, 22 insertions(+), 15 deletions(-)\n> \n> diff --git a/builtin/config.c b/builtin/config.c\n> index 33c9bf9..19ffcaf 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -379,8 +379,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n> \t\t\t */\n> \t\t\tdie(\"$HOME not set\");\n> \n> -\t\tif (access_or_warn(user_config, R_OK) &&\n> -\t\t xdg_config && !access_or_warn(xdg_config, R_OK))\n> +\t\tif (access_or_warn(user_config, R_OK, 0) &&\n> +\t\t xdg_config && !access_or_warn(xdg_config, R_OK, 0))\n> \t\t\tgiven_config_file = xdg_config;\n> \t\telse\n> \t\t\tgiven_config_file = user_config;\n> diff --git a/config.c b/config.c\n> index aefd80b..830ee14 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -58,7 +58,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n> \t\tpath = buf.buf;\n> \t}\n> \n> -\tif (!access_or_die(path, R_OK)) {\n> +\tif (!access_or_die(path, R_OK, 0)) {\n> \t\tif (++inc->depth > MAX_INCLUDE_DEPTH)\n> \t\t\tdie(include_depth_advice, MAX_INCLUDE_DEPTH, path,\n> \t\t\t cf && cf->name ? cf->name : \"the command line\");\n> @@ -954,23 +954,23 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n> \n> \thome_config_paths(&user_config, &xdg_config, \"config\");\n> \n> -\tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK)) {\n> +\tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {\n> \t\tret += git_config_from_file(fn, git_etc_gitconfig(),\n> \t\t\t\t\t data);\n> \t\tfound += 1;\n> \t}\n> \n> -\tif (xdg_config && !access_or_die(xdg_config, R_OK)) {\n> +\tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {\n> \t\tret += git_config_from_file(fn, xdg_config, data);\n> \t\tfound += 1;\n> \t}\n> \n> -\tif (user_config && !access_or_die(user_config, R_OK)) {\n> +\tif (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) {\n> \t\tret += git_config_from_file(fn, user_config, data);\n> \t\tfound += 1;\n> \t}\n> \n> -\tif (repo_config && !access_or_die(repo_config, R_OK)) {\n> +\tif (repo_config && !access_or_die(repo_config, R_OK, 0)) {\n> \t\tret += git_config_from_file(fn, repo_config, data);\n> \t\tfound += 1;\n> \t}\n> diff --git a/dir.c b/dir.c\n> index 91cfd99..9cb2f3d 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1637,9 +1637,9 @@ void setup_standard_excludes(struct dir_struct *dir)\n> \t\thome_config_paths(NULL, &xdg_path, \"ignore\");\n> \t\texcludes_file = xdg_path;\n> \t}\n> -\tif (!access_or_warn(path, R_OK))\n> +\tif (!access_or_warn(path, R_OK, 0))\n> \t\tadd_excludes_from_file(dir, path);\n> -\tif (excludes_file && !access_or_warn(excludes_file, R_OK))\n> +\tif (excludes_file && !access_or_warn(excludes_file, R_OK, 0))\n> \t\tadd_excludes_from_file(dir, excludes_file);\n> }\n> \n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index cde442f..51a29b8 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -691,8 +691,9 @@ int remove_or_warn(unsigned int mode, const char *path);\n> * Call access(2), but warn for any error except \"missing file\"\n> * (ENOENT or ENOTDIR).\n> */\n> -int access_or_warn(const char *path, int mode);\n> -int access_or_die(const char *path, int mode);\n> +#define ACCESS_EACCES_OK (1U << 0)\n> +int access_or_warn(const char *path, int mode, unsigned flag);\n> +int access_or_die(const char *path, int mode, unsigned flag);\n> \n> /* Warn on an inaccessible file that ought to be accessible */\n> void warn_on_inaccessible(const char *path);\n> diff --git a/wrapper.c b/wrapper.c\n> index bac59d2..dd7ecbb 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -408,18 +408,24 @@ void warn_on_inaccessible(const char *path)\n> \twarning(_(\"unable to access '%s': %s\"), path, strerror(errno));\n> }\n> \n> -int access_or_warn(const char *path, int mode)\n> +static int access_error_is_ok(int err, unsigned flag)\n> +{\n> +\treturn err == ENOENT || err == ENOTDIR ||\n> +\t\t((flag & ACCESS_EACCES_OK) && err == EACCES);\n> +}\n> +\n> +int access_or_warn(const char *path, int mode, unsigned flag)\n> {\n> \tint ret = access(path, mode);\n> -\tif (ret && errno != ENOENT && errno != ENOTDIR)\n> +\tif (ret && !access_error_is_ok(errno, flag))\n> \t\twarn_on_inaccessible(path);\n> \treturn ret;\n> }\n> \n> -int access_or_die(const char *path, int mode)\n> +int access_or_die(const char *path, int mode, unsigned flag)\n> {\n> \tint ret = access(path, mode);\n> -\tif (ret && errno != ENOENT && errno != ENOTDIR)\n> +\tif (ret && !access_error_is_ok(errno, flag))\n> \t\tdie_errno(_(\"unable to access '%s'\"), path);\n> \treturn ret;\n> }\n"},{"id":"218535","messageId":"CAHmME9rYH2kuvxafg10kYRa8kiOLeBeY9UuwUMNOeQdTbvhf+g@mail.gmail.com","threadId":"33453","inReplyTo":"20130412210318.GU27070@google.com","subject":"Re: [PATCH v2] config: allow inaccessible configuration under $HOME","fromName":"Jason A. Donenfeld","fromEmail":"jason@zx2c4.com","sentAt":"2013-05-25T11:35:25Z","receivedAt":"2013-05-25T11:35:25Z","isPatch":true,"sender":{"key":"jason@zx2c4.com","avatar":"https://gravatar.com/avatar/8a3d6c2049b8353b1e0fb78142042d142c9dc72975698d944933997974d9fac5?d=mp&s=160"},"body":"Jonathan's patch would indeed be nice. In cgit, we are forced to do\nugly things like this:\n\n /* Do not look in /etc/ for gitconfig and gitattributes. */\n setenv(\"GIT_CONFIG_NOSYSTEM\", \"1\", 1);\n setenv(\"GIT_ATTR_NOSYSTEM\", \"1\", 1);\n\n /* We unset HOME and XDG_CONFIG_HOME before calling the git\nsetup function\n * so that we don't make unneccessary filesystem accesses. */\n user_home = getenv(\"HOME\");\n xdg_home = getenv(\"XDG_CONFIG_HOME\");\n unsetenv(\"HOME\");\n unsetenv(\"XDG_CONFIG_HOME\");\n\n /* Setup the git directory and initialize the notes system.\nBoth of these\n * load local configuration from the git repository, so we do\nthem both while\n * the HOME variables are unset. */\n setup_git_directory_gently(&nongit);\n init_display_notes(NULL);\n\n /* We restore the unset variables afterward. */\n if (user_home)\n setenv(\"HOME\", user_home, 1);\n if (xdg_home)\n setenv(\"XDG_CONFIG_HOME\", xdg_home, 1);\n\nA big nasty guard around the git setup function, so that we don't error out.\n"}]}