regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Mike Galbraith]

Greetings,

I use git-daemon as the keeper of all source (love it).  git is a normal
user, running as git:daemon, with all repositories living in ~git.

git-daemon is started like so:

/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

Try to pull as root or normal user results in:

[pid 26786] access("/root/.config/git/config", R_OK) = -1 EACCES (Permission denied)
[pid 26786] write(2, "fatal: unable to access '/root/."..., 70) = 70
[pid 26785] <... read resumed> "fatal: unable to access '/root/."..., 4096) = 70
[pid 26786] exit_group(128)

Bisection fingered this commit, though it looks like it's really due to
not forgetting who it was at birth.  It's not root, so has no business
rummaging around in /root.  It used to not care, but this commit made
"go away" while looking for non-existent config file terminal.

-Mike

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 1403 bytes --]

On Wed, Apr 10, 2013 at 07:33:35AM +0200, Mike Galbraith wrote:
> /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
> 
> Try to pull as root or normal user results in:
> 
> [pid 26786] access("/root/.config/git/config", R_OK) = -1 EACCES (Permission denied)
> [pid 26786] write(2, "fatal: unable to access '/root/."..., 70) = 70
> [pid 26785] <... read resumed> "fatal: unable to access '/root/."..., 4096) = 70
> [pid 26786] exit_group(128)
> 
> Bisection fingered this commit, though it looks like it's really due to
> not forgetting who it was at birth.  It's not root, so has no business
> rummaging around in /root.  It used to not care, but this commit made
> "go away" while looking for non-existent config file terminal.

I ran into this too, although I'm running git-daemon via spawn-fcgi.
In order to convince newer Gits that you know what you're doing, you
just need to set HOME to somewhere Git can look.  For example:

  HOME=/ /usr/lib/git/git-daemon …

should work.  On Gentoo, I added the following to
/etc/conf.d/spawn-fcgi.fcgiwrap:

  ALLOWED_ENV="PATH HOME"
  HOME=/

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Mike Galbraith]

On Wed, 2013-04-10 at 09:56 -0400, W. Trevor King wrote: 
> On Wed, Apr 10, 2013 at 07:33:35AM +0200, Mike Galbraith wrote:
> > /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
> > 
> > Try to pull as root or normal user results in:
> > 
> > [pid 26786] access("/root/.config/git/config", R_OK) = -1 EACCES (Permission denied)
> > [pid 26786] write(2, "fatal: unable to access '/root/."..., 70) = 70
> > [pid 26785] <... read resumed> "fatal: unable to access '/root/."..., 4096) = 70
> > [pid 26786] exit_group(128)
> > 
> > Bisection fingered this commit, though it looks like it's really due to
> > not forgetting who it was at birth.  It's not root, so has no business
> > rummaging around in /root.  It used to not care, but this commit made
> > "go away" while looking for non-existent config file terminal.
> 
> I ran into this too, although I'm running git-daemon via spawn-fcgi.
> In order to convince newer Gits that you know what you're doing, you
> just need to set HOME to somewhere Git can look.  For example:
> 
>   HOME=/ /usr/lib/git/git-daemon …
> 
> should work.  On Gentoo, I added the following to
> /etc/conf.d/spawn-fcgi.fcgiwrap:
> 
>   ALLOWED_ENV="PATH HOME"
>   HOME=/

I can work around it by changing the init script to use su - git -c "bla
bla" to launch the thing, instead of using --user=git --group=daemon,
but that's just a bandaid for the busted environment setup those
switches were supposed to make happen, no?

-Mike

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Thu, Apr 11, 2013 at 05:39:43AM +0200, Mike Galbraith wrote:

> >   ALLOWED_ENV="PATH HOME"
> >   HOME=/
> 
> I can work around it by changing the init script to use su - git -c "bla
> bla" to launch the thing, instead of using --user=git --group=daemon,
> but that's just a bandaid for the busted environment setup those
> switches were supposed to make happen, no?

Yeah, I think the bug here is that git-daemon should be setting $HOME
when it switches privileges with --user. Does this patch fix it for you?

diff --git a/daemon.c b/daemon.c
index 6aeddcb..a4451fd 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1091,6 +1091,7 @@ static void drop_privileges(struct credentials *cred)
 	if (cred && (initgroups(cred->pass->pw_name, cred->gid) ||
 	    setgid (cred->gid) || setuid(cred->pass->pw_uid)))
 		die("cannot drop privileges");
+	setenv("HOME", cred->pass->pw_dir, 1);
 }
 
 static struct credentials *prepare_credentials(const char *user_name,

I guess that would technically break anybody who was trying to do
something clever with HOME (i.e., point it somewhere besides --user's
HOME where they had put some config files). But the obvious clever
thing would be to also set the user's passwd homedir to the same place.

-Peff

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Mike Galbraith]

On Thu, 2013-04-11 at 01:42 -0400, Jeff King wrote: 
> On Thu, Apr 11, 2013 at 05:39:43AM +0200, Mike Galbraith wrote:
> 
> > >   ALLOWED_ENV="PATH HOME"
> > >   HOME=/
> > 
> > I can work around it by changing the init script to use su - git -c "bla
> > bla" to launch the thing, instead of using --user=git --group=daemon,
> > but that's just a bandaid for the busted environment setup those
> > switches were supposed to make happen, no?
> 
> Yeah, I think the bug here is that git-daemon should be setting $HOME
> when it switches privileges with --user. Does this patch fix it for you?
> 
> diff --git a/daemon.c b/daemon.c
> index 6aeddcb..a4451fd 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1091,6 +1091,7 @@ static void drop_privileges(struct credentials *cred)
>  	if (cred && (initgroups(cred->pass->pw_name, cred->gid) ||
>  	    setgid (cred->gid) || setuid(cred->pass->pw_uid)))
>  		die("cannot drop privileges");
> +	setenv("HOME", cred->pass->pw_dir, 1);
>  }
>  
>  static struct credentials *prepare_credentials(const char *user_name,
> 
> I guess that would technically break anybody who was trying to do
> something clever with HOME (i.e., point it somewhere besides --user's
> HOME where they had put some config files). But the obvious clever
> thing would be to also set the user's passwd homedir to the same place.

I did exactly that yesterday, and it didn't work, which rather surprised
me.  Let me double check that I didn't screw trivial all up...

Heh, if ya don't plunk new binary into the old oddly placed bucket :)
Yeah, that works just fine.

-Mike 

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Apr 11, 2013 at 05:39:43AM +0200, Mike Galbraith wrote:
>
>> >   ALLOWED_ENV="PATH HOME"
>> >   HOME=/
>> 
>> I can work around it by changing the init script to use su - git -c "bla
>> bla" to launch the thing, instead of using --user=git --group=daemon,
>> but that's just a bandaid for the busted environment setup those
>> switches were supposed to make happen, no?
>
> Yeah, I think the bug here is that git-daemon should be setting $HOME
> when it switches privileges with --user. Does this patch fix it for you?
>
> diff --git a/daemon.c b/daemon.c
> index 6aeddcb..a4451fd 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1091,6 +1091,7 @@ static void drop_privileges(struct credentials *cred)
>  	if (cred && (initgroups(cred->pass->pw_name, cred->gid) ||
>  	    setgid (cred->gid) || setuid(cred->pass->pw_uid)))
>  		die("cannot drop privileges");
> +	setenv("HOME", cred->pass->pw_dir, 1);
>  }
>  
>  static struct credentials *prepare_credentials(const char *user_name,

Yeah, that sounds like the obvious fix to me.

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Thu, Apr 11, 2013 at 08:35:46AM -0700, Junio C Hamano wrote:

> > Yeah, I think the bug here is that git-daemon should be setting $HOME
> > when it switches privileges with --user. Does this patch fix it for you?
> [...]
> Yeah, that sounds like the obvious fix to me.

Here it is with a commit message.

-- >8 --
Subject: [PATCH] daemon: set HOME when we switch to --user

If git-daemon is invoked with the "--user foo" option, we
setuid and setgid to the "foo" user. However, we do not
currently touch $HOME or any other environment variables.

This means that a git-daemon (and its git subprocesses)
invoked as root will look at ~root/.gitconfig,
~root/.config/git, etc. This is probably not what the admin
expected; it would make more sense to load user-wide config
from ~foo.

Traditionally this wasn't that big a deal, as most sites do
not put config in either homedir (they would use the
system-wide /etc/gitconfig if they wanted global config).
However, since 96b9e0e (config: treat user and xdg config
permission problems as errors, 2012-10-13), it is now an
error to try to read from an inaccessible config file (which
a file in ~root is very likely to be), meaning that
git-daemon will not run at all in such a case.

We can fix this by setting HOME appropriately when we switch
users. Note that this is a regression for any site that uses
--user but depends on putting config in the $HOME of the
user invoking git-daemon. Since the original behavior was
never documented, and the new behavior is much more
sensible, we can consider this a bugfix.

Reported-by: Mike Galbraith <bitbucket@online.de>
Signed-off-by: Jeff King <peff@peff.net>
---
I don't have any problem calling this a bugfix and claiming that anyone
who was depending on the original behavior is stupid and wrong. But it
should probably get a prominent slot in the ReleaseNotes.

 daemon.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/daemon.c b/daemon.c
index 6aeddcb..a4451fd 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1091,6 +1091,7 @@ static void drop_privileges(struct credentials *cred)
 	if (cred && (initgroups(cred->pass->pw_name, cred->gid) ||
 	    setgid (cred->gid) || setuid(cred->pass->pw_uid)))
 		die("cannot drop privileges");
+	setenv("HOME", cred->pass->pw_dir, 1);
 }
 
 static struct credentials *prepare_credentials(const char *user_name,
-- 
1.8.2.rc0.33.gd915649

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jonathan Nieder]

Jeff King wrote:

> Here it is with a commit message.
>
> -- >8 --
> Subject: [PATCH] daemon: set HOME when we switch to --user

Thanks for taking care of it.  For what it's worth,

Acked-by: Jonathan Nieder <jrnieder@gmail.com>

I'm not sure whether to keep 96b9e0e (config: treat user and xdg
config permission problem as errors) in the long run, BTW.  There have
been multiple reports about dropping privileges and not being able to
access the old HOME, and I'm not convinced any more that the
predictability is worth the breakage for such people.  Though checking
if $HOME is inaccessible and treating that case specially would be
even worse...

Insights welcome.

Jonathan

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Thu, Apr 11, 2013 at 11:11:03AM -0700, Jonathan Nieder wrote:

> > -- >8 --
> > Subject: [PATCH] daemon: set HOME when we switch to --user
> 
> Thanks for taking care of it.  For what it's worth,
> 
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> I'm not sure whether to keep 96b9e0e (config: treat user and xdg
> config permission problem as errors) in the long run, BTW.  There have
> been multiple reports about dropping privileges and not being able to
> access the old HOME, and I'm not convinced any more that the
> predictability is worth the breakage for such people.  Though checking
> if $HOME is inaccessible and treating that case specially would be
> even worse...
> 
> Insights welcome.

I could go either way. I think 96b9e0e is the right thing to do
conceptually, but I kind of doubt it was affecting all that many people.
And though it's _possible_ for it to be a security problem, I find it
much more likely that the site admin tries to set some config, gets
annoyed when it doesn't work, and debugs it. So from a practical
perspective, 96b9e0e may be doing more harm than good, even though it's
the right thing.

-Peff

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jonathan Nieder]

Jeff King wrote:

> I could go either way. I think 96b9e0e is the right thing to do
> conceptually, but I kind of doubt it was affecting all that many people.
> And though it's _possible_ for it to be a security problem, I find it
> much more likely that the site admin tries to set some config, gets
> annoyed when it doesn't work, and debugs it. So from a practical
> perspective, 96b9e0e may be doing more harm than good, even though it's
> the right thing.

Ok.  By the way, another commit to blame is v1.7.12.1~2^2~4 (config:
warn on inaccessible files, 2012-08-21), which made it into a warnable
offense instead of just a strange but accepted configuration. ;-)

I'm still leaning toward keeping v1.7.12.1~2^2~4 and 96b9e0e as being
worth it, though I could be easily swayed in the other direction (for
example via a patch by an interested user with documentation that
explains how to debug and makes it unlikely for the behavior to keep
flipping in the future).  Thanks for spelling out the trade-offs.

Sincerely,
Jonathan

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Apr 11, 2013 at 11:11:03AM -0700, Jonathan Nieder wrote:
>
>> > -- >8 --
>> > Subject: [PATCH] daemon: set HOME when we switch to --user
>> 
>> Thanks for taking care of it.  For what it's worth,
>> 
>> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
>> 
>> I'm not sure whether to keep 96b9e0e (config: treat user and xdg
>> config permission problem as errors) in the long run, BTW.  There have
>> been multiple reports about dropping privileges and not being able to
>> access the old HOME, and I'm not convinced any more that the
>> predictability is worth the breakage for such people.  Though checking
>> if $HOME is inaccessible and treating that case specially would be
>> even worse...
>> 
>> Insights welcome.
>
> I could go either way. I think 96b9e0e is the right thing to do
> conceptually, but I kind of doubt it was affecting all that many people.
> And though it's _possible_ for it to be a security problem, I find it
> much more likely that the site admin tries to set some config, gets
> annoyed when it doesn't work, and debugs it. So from a practical
> perspective, 96b9e0e may be doing more harm than good, even though it's
> the right thing.

Recent reports in this thread make us think so, I guess.

But reverting 96b9e0e alone would not help these people very much
though.  They will have reams of warning messages in their server
logs, and the way to "fix" it would be the same as the way to work
around the access_or_die(), namely, to set $HOME to point at a more
appropriate place before running "git daemon".

I also have a suspicion that your patch makes things worse for
people who are more adept at these issues around running daemons
than the people who introduced this problem in the first place (eh,
that's "us").  It is plausible that they may run multiple instances
of "initially root but setuid() to an unprivileged user" daemons,
giving each of them a separate play area by setting $HOME to
different values, just for management's ease not necessarily for
security (hence sharing the same unprivileged user), which will be
broken by the patch that unconditionally overrides $HOME.

A trade off to make things slightly easier for one sysadmin by
making another thing impossible to do for another sysadmin does not
sound like a good one.

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

On Thu, Apr 11, 2013 at 12:54:34PM -0700, Junio C Hamano wrote:
> I also have a suspicion that your patch makes things worse for
> people who are more adept at these issues around running daemons
> than the people who introduced this problem in the first place (eh,
> that's "us")…
> 
> A trade off to make things slightly easier for one sysadmin by
> making another thing impossible to do for another sysadmin does not
> sound like a good one.

As one of the less experienced folks tripped up by this issue, I think
that setting HOME explicitly before invoking the daemon is simple
enough (which is why I just fixed my invocation and didn't post to the
list).  The difficulty was figuring out why the daemon was dying in
the first place (which involved bisection for me as well).  Maybe
there could be an additional note about HOME to flesh out:

  fatal: unable to access '/root/.config/git/config': Permission denied

when there's an EACCES error for the per-user config?  On the other
hand, the code to handle this might be more trouble than it's worth.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Junio C Hamano]

"W. Trevor King" <wking@tremily.us> writes:

> As one of the less experienced folks tripped up by this issue, I think
> that setting HOME explicitly before invoking the daemon is simple
> enough (which is why I just fixed my invocation and didn't post to the
> list).

Sounds like we need a documentation update somewhere.

> The difficulty was figuring out why the daemon was dying in
> the first place (which involved bisection for me as well).  Maybe
> there could be an additional note about HOME to flesh out:
>
>   fatal: unable to access '/root/.config/git/config': Permission denied
>
> when there's an EACCES error for the per-user config?

Doesn't access_or_die() say

    die_errno(_("unable to access '%s'"), path);

already?  I am puzzled...

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Thu, Apr 11, 2013 at 03:20:46PM -0700, Junio C Hamano wrote:

> "W. Trevor King" <wking@tremily.us> writes:
> 
> > As one of the less experienced folks tripped up by this issue, I think
> > that setting HOME explicitly before invoking the daemon is simple
> > enough (which is why I just fixed my invocation and didn't post to the
> > list).
> 
> Sounds like we need a documentation update somewhere.

Yeah, I'd be happy to drop my patch if somebody wants to write a
documentation update instead.

> > The difficulty was figuring out why the daemon was dying in
> > the first place (which involved bisection for me as well).  Maybe
> > there could be an additional note about HOME to flesh out:
> >
> >   fatal: unable to access '/root/.config/git/config': Permission denied
> >
> > when there's an EACCES error for the per-user config?
> 
> Doesn't access_or_die() say
> 
>     die_errno(_("unable to access '%s'"), path);
> 
> already?  I am puzzled...

I think the point is that it could add

  ...and I was looking in /root, because that is where your HOME points.
  Shouldn't you be able to read your own HOME directory?

which should make it painfully obvious to the user what is going on.

-Peff

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 1998 bytes --]

On Thu, Apr 11, 2013 at 06:23:01PM -0400, Jeff King wrote:
> On Thu, Apr 11, 2013 at 03:20:46PM -0700, Junio C Hamano wrote:
> > "W. Trevor King" <wking@tremily.us> writes:
> > > The difficulty was figuring out why the daemon was dying in
> > > the first place (which involved bisection for me as well).  Maybe
> > > there could be an additional note about HOME to flesh out:
> > >
> > >   fatal: unable to access '/root/.config/git/config': Permission denied
> > >
> > > when there's an EACCES error for the per-user config?
> > 
> > Doesn't access_or_die() say
> > 
> >     die_errno(_("unable to access '%s'"), path);
> > 
> > already?  I am puzzled...
> 
> I think the point is that it could add
> 
>   ...and I was looking in /root, because that is where your HOME points.
>   Shouldn't you be able to read your own HOME directory?
> 
> which should make it painfully obvious to the user what is going on.

That's more or less what I had in mind.  The 1.8.1.1 release notes
just say:

 * When attempting to read the XDG-style $HOME/.config/git/config and
   finding that $HOME/.config/git is a file, we gave a wrong error
   message, instead of treating the case as "a custom config file does
   not exist there" and moving on.

without saying anything about permission problems becoming errors, or
noting that oddball HOME configurations might cause problems.  Since
the release notes are already out, a notice like this should probably
go somewhere else.  However, this is a lot of hand holding to be
printed along side the error message… Since git-daemon (or gitweb) is
the most likely place for this problem to crop up, maybe a note in its
(their) man pages would be a good idea?  This thread may also be
sufficient documentation, assuming good enough search engines ;).

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Junio C Hamano]

"W. Trevor King" <wking@tremily.us> writes:

> On Thu, Apr 11, 2013 at 06:23:01PM -0400, Jeff King wrote:
> ...
>> I think the point is that it could add
>> 
>>   ...and I was looking in /root, because that is where your HOME points.
>>   Shouldn't you be able to read your own HOME directory?
>> 
>> which should make it painfully obvious to the user what is going on.
>
> That's more or less what I had in mind.
> ... However, this is a lot of hand holding to be
> printed along side the error message… Since git-daemon (or gitweb) is
> the most likely place for this problem to crop up, maybe a note in its
> (their) man pages would be a good idea?

The --user option to git-daemon would be a good place to do that, I
think.  Depending on what other "setuid to less privileged before
running" programs do (I do not know offhand), we can say something
like this perhaps?

    --user::
	... current description ...
    +
    (Like|Unlike) many programs that let you run programs as
    specified user, the daemon does not reset environment variables
    such as $HOME when it runs git programs like upload-pack and
    receive-pack. Set and export HOME to point at the home directory
    of the user you specify with this option before you start the
    daemon, and make sure the Git configuration files in that
    directory is readable by that user.

If we have to say "Unlike" above, then we probably should bite the
bullet and use Peff's patch, perhaps with an addition to the manual
page, perhaps like this.

    --user::
	... current description ...
    +
    Like many programs that let you run programs as specified user,
    the daemon resets $HOME environment variable to that of the user
    you specify with this option when it runs git programs like
    upload-pack and receive-pack.  Make sure that the Git
    configuration files in that directory is readable by that user.

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Thu, Apr 11, 2013 at 09:11:20PM -0700, Junio C Hamano wrote:

> The --user option to git-daemon would be a good place to do that, I
> think.  Depending on what other "setuid to less privileged before
> running" programs do (I do not know offhand), we can say something
> like this perhaps?

That's a good question. I looked at (just sampling a few off the top of
my head):

  xinetd
  openbsd-inetd
  inetutils-inetd
  postfix
  dovecot
  courier

and none of them sets HOME when dropping privileges. Admittedly some of
them do not drop privileges immediately in the same way (e.g., the imap
servers need to remain root so that they can switch to the right user to
read mail). Postfix does set HOME, but only when actually "becoming" the
user to do deliveries, not at startup.

I could also be wrong on one or more of those, as that is from some
quick grepping, but I think it's clear that the norm is not to set HOME
alongside setuid (of all of them, I would say git-daemon behaves most
like the inetd utils, as it does not ever "become" users at all).

>     --user::
> 	... current description ...
>     +
>     (Like|Unlike) many programs that let you run programs as
>     specified user, the daemon does not reset environment variables
>     such as $HOME when it runs git programs like upload-pack and
>     receive-pack. Set and export HOME to point at the home directory
>     of the user you specify with this option before you start the
>     daemon, and make sure the Git configuration files in that
>     directory is readable by that user.

So choosing "Like" here, I think this makes sense.

-Peff

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Apr 11, 2013 at 09:11:20PM -0700, Junio C Hamano wrote:
>
>> The --user option to git-daemon would be a good place to do that, I
>> think.  Depending on what other "setuid to less privileged before
>> running" programs do (I do not know offhand), we can say something
>> like this perhaps?
>
> That's a good question. I looked at (just sampling a few off the top of
> my head):
>
>   xinetd
>   openbsd-inetd
>   inetutils-inetd
>   postfix
>   dovecot
>   courier
>
> and none of them sets HOME when dropping privileges. Admittedly some of
> them do not drop privileges immediately in the same way (e.g., the imap
> servers need to remain root so that they can switch to the right user to
> read mail). Postfix does set HOME, but only when actually "becoming" the
> user to do deliveries, not at startup.

Thanks for checking.

For $HOME, it is sufficient to do an unsetenv-equivalent, and I
suspect some of them do just that, though.  Vanilla openbsd-inetd
doesn't, but Debian seems to have a patch to sanitize the
environment, for example.

But still...

> I could also be wrong on one or more of those, as that is from some
> quick grepping, but I think it's clear that the norm is not to set HOME
> alongside setuid (of all of them, I would say git-daemon behaves most
> like the inetd utils, as it does not ever "become" users at all).
>
>>     --user::
>> 	... current description ...
>>     +
>>     (Like|Unlike) many programs that let you run programs as
>>     specified user, the daemon does not reset environment variables
>>     such as $HOME when it runs git programs like upload-pack and
>>     receive-pack. Set and export HOME to point at the home directory
>>     of the user you specify with this option before you start the
>>     daemon, and make sure the Git configuration files in that
>>     directory is readable by that user.
>
> So choosing "Like" here, I think this makes sense.

I would prefer the simplicity ;-)

"Set and export HOME to point at the home directory of the user you
specify with this option" screams that it wants to be rephrased at
me, though.  It somehow sounds as if this option is a way to set and
export the environment variable unless re-read carefully X-<.

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Thu, Apr 11, 2013 at 09:46:35PM -0700, Junio C Hamano wrote:

> >>     --user::
> >> 	... current description ...
> >>     +
> >>     (Like|Unlike) many programs that let you run programs as
> >>     specified user, the daemon does not reset environment variables
> >>     such as $HOME when it runs git programs like upload-pack and
> >>     receive-pack. Set and export HOME to point at the home directory
> >>     of the user you specify with this option before you start the
> >>     daemon, and make sure the Git configuration files in that
> >>     directory is readable by that user.
> >
> > So choosing "Like" here, I think this makes sense.
> 
> I would prefer the simplicity ;-)
> 
> "Set and export HOME to point at the home directory of the user you
> specify with this option" screams that it wants to be rephrased at
> me, though.  It somehow sounds as if this option is a way to set and
> export the environment variable unless re-read carefully X-<.

Perhaps:

  Like many programs that switch user id, the daemon does not reset
  environment variables such a `$HOME` when it runs git programs like
  `upload-pack` and `receive-pack`. When using this option, you may also
  want to set and export `HOME` to point at the home directory of
  `<user>` before starting the daemon, and make sure the Git
  configuration file in that directory are readable by `<user>`.

I tried to address your concern above (which I agree with), smooth over
a few clunky wordings, and use "<user>", which is defined in the heading
of the option:

  --user=<user>, --group=<group>

-Peff

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Mike Galbraith]

On Fri, 2013-04-12 at 01:05 -0400, Jeff King wrote: 
> On Thu, Apr 11, 2013 at 09:46:35PM -0700, Junio C Hamano wrote:
> 
> > >>     --user::
> > >> 	... current description ...
> > >>     +
> > >>     (Like|Unlike) many programs that let you run programs as
> > >>     specified user, the daemon does not reset environment variables
> > >>     such as $HOME when it runs git programs like upload-pack and
> > >>     receive-pack. Set and export HOME to point at the home directory
> > >>     of the user you specify with this option before you start the
> > >>     daemon, and make sure the Git configuration files in that
> > >>     directory is readable by that user.
> > >
> > > So choosing "Like" here, I think this makes sense.
> > 
> > I would prefer the simplicity ;-)
> > 
> > "Set and export HOME to point at the home directory of the user you
> > specify with this option" screams that it wants to be rephrased at
> > me, though.  It somehow sounds as if this option is a way to set and
> > export the environment variable unless re-read carefully X-<.
> 
> Perhaps:
> 
>   Like many programs that switch user id, the daemon does not reset
>   environment variables such a `$HOME` when it runs git programs like
>   `upload-pack` and `receive-pack`. When using this option, you may also
>   want to set and export `HOME` to point at the home directory of
>   `<user>` before starting the daemon, and make sure the Git
>   configuration file in that directory are readable by `<user>`.
> 
> I tried to address your concern above (which I agree with), smooth over
> a few clunky wordings, and use "<user>", which is defined in the heading
> of the option:
> 
>   --user=<user>, --group=<group>

I just updated my local rpm to set HOME, so the surprise of no workee
after upgrade here is forever gone.  Looks like suses (at least)
packagers will need to do the same, else canned setup ain't gonna work.

-Mike

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 770 bytes --]

On Fri, Apr 12, 2013 at 01:05:50AM -0400, Jeff King wrote:
>   Like many programs that switch user id, the daemon does not reset
>   environment variables such a `$HOME` when it runs git programs like
>   `upload-pack` and `receive-pack`. When using this option, you may also
>   want to set and export `HOME` to point at the home directory of
>   `<user>` before starting the daemon, and make sure the Git
>   configuration file in that directory are readable by `<user>`.

How about "and make sure any Git configuration files", since there
might not be any Git configuration files.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Fri, Apr 12, 2013 at 07:26:36AM -0400, W. Trevor King wrote:

> On Fri, Apr 12, 2013 at 01:05:50AM -0400, Jeff King wrote:
> >   Like many programs that switch user id, the daemon does not reset
> >   environment variables such a `$HOME` when it runs git programs like
> >   `upload-pack` and `receive-pack`. When using this option, you may also
> >   want to set and export `HOME` to point at the home directory of
> >   `<user>` before starting the daemon, and make sure the Git
> >   configuration file in that directory are readable by `<user>`.
> 
> How about "and make sure any Git configuration files", since there
> might not be any Git configuration files.

Yeah, that is better. Thanks.

-Peff

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> How about "and make sure any Git configuration files", since there
>> might not be any Git configuration files.
>
> Yeah, that is better. Thanks.

OK, then...

-- >8 --
Subject: [PATCH] doc: clarify that "git daemon --user=<user>" option does not export HOME=~user

Signed-off-by: Jeff King <peff@peff.net>
Helped-by: W. Trevor King <wking@tremily.us>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-daemon.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 7e5098a..2ac07ba 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -147,6 +147,13 @@ OPTIONS
 Giving these options is an error when used with `--inetd`; use
 the facility of inet daemon to achieve the same before spawning
 'git daemon' if needed.
++
+Like many programs that switch user id, the daemon does not reset
+environment variables such as `$HOME` when it runs git programs,
+e.g. `upload-pack` and `receive-pack`. When using this option, you
+may also want to set and export `HOME` to point at the home
+directory of `<user>` before starting the daemon, and make sure any
+Git configuration files in that directory are readable by `<user>`.
 
 --enable=<service>::
 --disable=<service>::
-- 
1.8.2.1-472-g6c5785c

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Fri, Apr 12, 2013 at 09:08:31AM -0700, Junio C Hamano wrote:

> OK, then...

> -- >8 --
> Subject: [PATCH] doc: clarify that "git daemon --user=<user>" option does not export HOME=~user

I'd add this motiviation to the body of the commit message:

  The fact that we don't set $HOME may confuse admins who
  expect $HOME/.gitconfig to be respected. And worse, since
  96b9e0e3, a git-daemon started by root is likely to fail
  to run at all, as the user we switch to generally cannot
  read ~root.

This still feels ugly, like we are documenting some gotcha
that is going to hit most admins, when we could be helping
them in the code.

One option we have not explored is an environment variable
to loosen git's requirement. I'm thinking something like
GIT_INACCESSIBLE_HOMEDIR_OK, which could be set by default
when git-daemon uses --user.

That would leave all existing setups working, but would
still enable the extra protections for people not running
git-daemon (and people who use git via sudo could choose to
set it, too, if they would prefer that to setting up HOME).

-Peff

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Fri, Apr 12, 2013 at 12:16:00PM -0400, Jeff King wrote:

> One option we have not explored is an environment variable
> to loosen git's requirement. I'm thinking something like
> GIT_INACCESSIBLE_HOMEDIR_OK, which could be set by default
> when git-daemon uses --user.
> 
> That would leave all existing setups working, but would
> still enable the extra protections for people not running
> git-daemon (and people who use git via sudo could choose to
> set it, too, if they would prefer that to setting up HOME).

So here's what I came up with. I tried to make the exception as tight as
possible by checking that $HOME was actually the problem, as that is the
common problem (you switch users, but HOME is pointing to the old user).

It means that we would still die if ~root is readable, but
~root/.gitconfig is not (or ~root/.config is not). I think that is
probably a good thing, though, as it is an indication of something more
interesting going on that the user should investigate.

Given how tight the exception is, I almost wonder if we should drop the
environment variable completely, and just never complain about this case
(in other words, just make it a loosening of 96b9e0e3).

-Peff

---
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 6a875f2..e004ee8 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -806,6 +806,15 @@ for further details.
 	temporarily to avoid using a buggy `/etc/gitconfig` file while
 	waiting for someone with sufficient permissions to fix it.
 
+'GIT_INACCESSIBLE_HOME_OK'::
+	Normally git will complain and die if it cannot access
+	`$HOME/.gitconfig`. If this variable is set to "1", then
+	a `$HOME` directory which cannot be accessed will not be
+	considered an error. This can be useful if you are switching
+	user ids without resetting `$HOME`, and are willing to take the
+	risk that some configuration options might not be used (because
+	git could not read the config file that contained them).
+
 'GIT_FLUSH'::
 	If this environment variable is set to "1", then commands such
 	as 'git blame' (in incremental mode), 'git rev-list', 'git log',
diff --git a/cache.h b/cache.h
index e1e8ce8..01f300f 100644
--- a/cache.h
+++ b/cache.h
@@ -358,6 +358,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define GIT_NOTES_REWRITE_REF_ENVIRONMENT "GIT_NOTES_REWRITE_REF"
 #define GIT_NOTES_REWRITE_MODE_ENVIRONMENT "GIT_NOTES_REWRITE_MODE"
 #define GIT_LITERAL_PATHSPECS_ENVIRONMENT "GIT_LITERAL_PATHSPECS"
+#define GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT "GIT_INACCESSIBLE_HOME_OK"
 
 /*
  * This environment variable is expected to contain a boolean indicating
diff --git a/daemon.c b/daemon.c
index 131b049..6c56cc0 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1091,7 +1091,7 @@ static void drop_privileges(struct credentials *cred)
 	if (cred && (initgroups(cred->pass->pw_name, cred->gid) ||
 	    setgid (cred->gid) || setuid(cred->pass->pw_uid)))
 		die("cannot drop privileges");
-	setenv("GIT_CONFIG_INACCESSIBLE_HOME_OK", "1", 0);
+	setenv(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, "1", 0);
 }
 
 static struct credentials *prepare_credentials(const char *user_name,
diff --git a/wrapper.c b/wrapper.c
index bac59d2..644f867 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -416,11 +416,52 @@ int access_or_die(const char *path, int mode)
 	return ret;
 }
 
+/*
+ * Returns true iff the path is under $HOME, that directory is inaccessible,
+ * and the user has told us through the environment that an inaccessible HOME
+ * is OK. The results are cached so that repeated calls will not make multiple
+ * syscalls.
+ */
+static int inaccessible_home_ok(const char *path)
+{
+	static int gentle = -1;
+	static int home_inaccessible = -1;
+	const char *home;
+
+	if (gentle < 0)
+		gentle = git_env_bool(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, 0);
+	if (!gentle)
+		return 0;
+
+	home = getenv("HOME");
+	if (!home)
+		return 0;
+	/*
+	 * We do not bother with normalizing PATHs to avoid symlinks
+	 * here, since the point is to catch paths that are
+	 * constructed as "$HOME/...".
+	 */
+	if (prefixcmp(path, home) && path[strlen(home)] == '/')
+		return 0;
+
+	if (home_inaccessible < 0)
+		home_inaccessible = access(home, R_OK|X_OK) < 0 && errno == EACCES;
+	return home_inaccessible;
+}
+
 int access_or_die(const char *path, int mode)
 {
 	int ret = access(path, mode);
-	if (ret && errno != ENOENT && errno != ENOTDIR)
+	if (ret && errno != ENOENT && errno != ENOTDIR) {
+		/*
+		 * We do not have to worry about clobbering errno
+		 * in inaccessible_home_ok, because we get either EACCES
+		 * again, or we die.
+		 */
+		if (errno == EACCES && inaccessible_home_ok(path))
+			return ret;
 		die_errno(_("unable to access '%s'"), path);
+	}
 	return ret;
 }
 

> 
> -Peff

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So here's what I came up with. I tried to make the exception as tight as
> possible by checking that $HOME was actually the problem, as that is the
> common problem (you switch users, but HOME is pointing to the old user).
> ...
> diff --git a/daemon.c b/daemon.c
> index 131b049..6c56cc0 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1091,7 +1091,7 @@ static void drop_privileges(struct credentials *cred)
>  	if (cred && (initgroups(cred->pass->pw_name, cred->gid) ||
>  	    setgid (cred->gid) || setuid(cred->pass->pw_uid)))
>  		die("cannot drop privileges");
> -	setenv("GIT_CONFIG_INACCESSIBLE_HOME_OK", "1", 0);
> +	setenv(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, "1", 0);
>  }

Compared against an unpublished diffbase???

>  
>  static struct credentials *prepare_credentials(const char *user_name,
> diff --git a/wrapper.c b/wrapper.c
> index bac59d2..644f867 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -416,11 +416,52 @@ int access_or_die(const char *path, int mode)
>  	return ret;
>  }
>  
> +/*
> + * Returns true iff the path is under $HOME, that directory is inaccessible,
> + * and the user has told us through the environment that an inaccessible HOME
> + * is OK. The results are cached so that repeated calls will not make multiple
> + * syscalls.
> + */
> +static int inaccessible_home_ok(const char *path)
> +{
> +	static int gentle = -1;
> +	static int home_inaccessible = -1;
> +	const char *home;
> +
> +	if (gentle < 0)
> +		gentle = git_env_bool(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, 0);
> +	if (!gentle)
> +		return 0;
> +
> +	home = getenv("HOME");
> +	if (!home)
> +		return 0;
> +	/*
> +	 * We do not bother with normalizing PATHs to avoid symlinks
> +	 * here, since the point is to catch paths that are
> +	 * constructed as "$HOME/...".
> +	 */
> +	if (prefixcmp(path, home) && path[strlen(home)] == '/')
> +		return 0;
> +
> +	if (home_inaccessible < 0)
> +		home_inaccessible = access(home, R_OK|X_OK) < 0 && errno == EACCES;
> +	return home_inaccessible;
> +}
> +
>  int access_or_die(const char *path, int mode)
>  {
>  	int ret = access(path, mode);
> -	if (ret && errno != ENOENT && errno != ENOTDIR)
> +	if (ret && errno != ENOENT && errno != ENOTDIR) {
> +		/*
> +		 * We do not have to worry about clobbering errno
> +		 * in inaccessible_home_ok, because we get either EACCES
> +		 * again, or we die.
> +		 */
> +		if (errno == EACCES && inaccessible_home_ok(path))
> +			return ret;

OK, so the idea is

 - The environment can tell us to ignore permission errors for paths
   under $HOME if (and only if) $HOME itself is not readable;

 - We got a permission error here.  inaccessible_home_ok() will tell
   us if the path is under $HOME and the above condition holds (in
   which case it will say "ok, ignore that error").

which sounds good, but it relies on the caller of this function not
to try actually reading from the path.

If the access() failed due to ENOENT, the caller will get a negative
return from this function and will treat it as "ok, it does not
exist", with the original or the updated code.  This new case is
treated the same way by the existing callers, i.e. pretending as if
there is _no_ file in that unreadable $HOME directory.

That semantics sounds sane and safe to me.

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Fri, Apr 12, 2013 at 11:23:46AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So here's what I came up with. I tried to make the exception as tight as
> > possible by checking that $HOME was actually the problem, as that is the
> > common problem (you switch users, but HOME is pointing to the old user).
> > ...
> > diff --git a/daemon.c b/daemon.c
> > index 131b049..6c56cc0 100644
> > --- a/daemon.c
> > +++ b/daemon.c
> > @@ -1091,7 +1091,7 @@ static void drop_privileges(struct credentials *cred)
> >  	if (cred && (initgroups(cred->pass->pw_name, cred->gid) ||
> >  	    setgid (cred->gid) || setuid(cred->pass->pw_uid)))
> >  		die("cannot drop privileges");
> > -	setenv("GIT_CONFIG_INACCESSIBLE_HOME_OK", "1", 0);
> > +	setenv(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, "1", 0);
> >  }
> 
> Compared against an unpublished diffbase???

Oops. Forgot I had made a WIP commit before running the diff.

> OK, so the idea is
> 
>  - The environment can tell us to ignore permission errors for paths
>    under $HOME if (and only if) $HOME itself is not readable;
> 
>  - We got a permission error here.  inaccessible_home_ok() will tell
>    us if the path is under $HOME and the above condition holds (in
>    which case it will say "ok, ignore that error").

Exactly.

> which sounds good, but it relies on the caller of this function not
> to try actually reading from the path.

Yes, but that is the only sane thing for the caller to do, since it gets
the same exit code from ENOENT and ENOTDIR already. Probably a comment
describing the return value is in order.

> If the access() failed due to ENOENT, the caller will get a negative
> return from this function and will treat it as "ok, it does not
> exist", with the original or the updated code.  This new case is
> treated the same way by the existing callers, i.e. pretending as if
> there is _no_ file in that unreadable $HOME directory.

Exactly.

> That semantics sounds sane and safe to me.

Thanks. I'll re-roll with a proper commit message and the fixups I
mentioned above. I think we should still do the documentation for
git-daemon. But it is no longer about "oops, we broke git-daemon", but
"you may want know that we do not set HOME in case you are doing
something tricky with config". I'll submit that with the re-roll, too.

Do you have an opinion on just dropping the environment variable
completely and behaving this way all the time? It would "just fix" the
cases people running into using su/sudo, too.

-Peff

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> If the access() failed due to ENOENT, the caller will get a negative
>> return from this function and will treat it as "ok, it does not
>> exist", with the original or the updated code.  This new case is
>> treated the same way by the existing callers, i.e. pretending as if
>> there is _no_ file in that unreadable $HOME directory.
>
> Exactly.

The explanation you are replying to was meant to illustrate how this
is not "inaccessible is OK", but is "treat inaccessible as missing",
by the way.

>> That semantics sounds sane and safe to me.
>
> Thanks. I'll re-roll with a proper commit message and the fixups I
> mentioned above. I think we should still do the documentation for
> git-daemon. But it is no longer about "oops, we broke git-daemon", but
> "you may want know that we do not set HOME in case you are doing
> something tricky with config". I'll submit that with the re-roll, too.

Well, at least to me, the documentation update was never about
"oops, we broke it", but was about "be careful where the HOME you
are using actually is" from the beginning of the suggestion.  I was
actually planning to apply it to maint-1.8.1 that predates the xdg
stuff, and that is why the text only suggests to set HOME for the
config.

> Do you have an opinion on just dropping the environment variable
> completely and behaving this way all the time? It would "just fix" the
> cases people running into using su/sudo, too.

With the tightening, people who used --user=daemon, expecting that
they can later tweak the behaviour by touching ~daemon/.gitconfig,
got an early warning that they need to set HOME themselves, but with
any variant of the patch under discussion, as long as loosening is
on by default, will no longer get that benefit.

I am not yet convinced if that is a real "fix/cure".

So, no, I have not even reached the point where I can form an
opinion if this behaviour should be the default.

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Fri, Apr 12, 2013 at 12:51:19PM -0700, Junio C Hamano wrote:

> >> If the access() failed due to ENOENT, the caller will get a negative
> >> return from this function and will treat it as "ok, it does not
> >> exist", with the original or the updated code.  This new case is
> >> treated the same way by the existing callers, i.e. pretending as if
> >> there is _no_ file in that unreadable $HOME directory.
> >
> > Exactly.
> 
> The explanation you are replying to was meant to illustrate how this
> is not "inaccessible is OK", but is "treat inaccessible as missing",
> by the way.

Ah, I see the distinction you were making. Yes, that is what I was
thinking (and what the patch does); I just used the word "OK" instead.

> Well, at least to me, the documentation update was never about
> "oops, we broke it", but was about "be careful where the HOME you
> are using actually is" from the beginning of the suggestion.  I was
> actually planning to apply it to maint-1.8.1 that predates the xdg
> stuff, and that is why the text only suggests to set HOME for the
> config.

Yes; I think the only change needed would be to the commit message I
proposed (if you even picked that up; I didn't look).

> > Do you have an opinion on just dropping the environment variable
> > completely and behaving this way all the time? It would "just fix" the
> > cases people running into using su/sudo, too.
> 
> With the tightening, people who used --user=daemon, expecting that
> they can later tweak the behaviour by touching ~daemon/.gitconfig,
> got an early warning that they need to set HOME themselves, but with
> any variant of the patch under discussion, as long as loosening is
> on by default, will no longer get that benefit.
> 
> I am not yet convinced if that is a real "fix/cure".
> 
> So, no, I have not even reached the point where I can form an
> opinion if this behaviour should be the default.

OK. I'll hold off for now while we stew on it. Jonathan's patch looks OK
to me, but it has the same issue. But I think every path has to be one
of:

  1. We annoy sysadmins who need to take an extra step to handle the
     HOME situation with --user (the current behavior, or any other
     proposal that they have to opt into).

  2. We annoy sysadmins who want to set HOME with --user, either by
     making what they want to do impossible, or making them set an extra
     variable or option to accomplish what used to work (my patch to set
     HOME with --user).

  3. We loosen the check, so some cases which might be noteworthy are
     not caught (my patch, Jonathan's patch, etc).

I think any solution will have to fall into one of those slots. So we
need to pick the least evil one, and then hammer out its least evil
form.

-Peff

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> OK. I'll hold off for now while we stew on it. Jonathan's patch looks OK
> to me, but it has the same issue. But I think every path has to be one
> of:
>
>   1. We annoy sysadmins who need to take an extra step to handle the
>      HOME situation with --user (the current behavior, or any other
>      proposal that they have to opt into).
>
>   2. We annoy sysadmins who want to set HOME with --user, either by
>      making what they want to do impossible, or making them set an extra
>      variable or option to accomplish what used to work (my patch to set
>      HOME with --user).
>
>   3. We loosen the check, so some cases which might be noteworthy are
>      not caught (my patch, Jonathan's patch, etc).
>
> I think any solution will have to fall into one of those slots. So we
> need to pick the least evil one, and then hammer out its least evil
> form.

That summarizes our options nicely, I think.  Thanks.

[PATCH] config: allow inaccessible configuration under $HOME

[Jonathan Nieder]

One unexpected downside to the changes v1.7.12.1~2^2~4 (config: warn
on inaccessible files, 2012-08-21) and v1.8.1.1~22^2~2 (config: treat
user and xdg config permission problems as errors, 2012-10-13) is that
they often trip when git is being run as a server.  The appropriate
daemon (sshd, inetd, git-daemon, etc) starts as "root", creates a
listening socket, and then drops privileges, meaning that when git
commands are invoked they cannot access $HOME and die with

 fatal: unable to access '/root/.config/git/config': Permission denied

The intent was always to prevent important configuration (think
"[transfer] fsckobjects") from being ignored when the configuration is
unintentionally unreadable (for example with ENOMEM due to a DoS
attack).  But that is not worth breaking these cases of
drop-privileges-without-resetting-HOME that were working fine before.

Treat user and xdg configuration that is inaccessible due to
permissions (EACCES) as though no user configuration was provided at
all.

An alternative approach would be to check if $HOME is readable, but
that would not solve the problem in cases where the user who dropped
privileges had a globally readable HOME with only .config or
.gitconfig being private.

This does not change the behavior when "git config" is being used to
write to a user's config file, when /etc/gitconfig or .git/config is
unreadable (since those are more serious configuration errors), or
when ~/.gitconfig or ~/.config/git is unreadable due to problems other
than permissions.

Thanks to Mike Galbraith, W. Trevor King, and Jeff King for their
patient explanations.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jeff King wrote:

> Given how tight the exception is, I almost wonder if we should drop the
> environment variable completely, and just never complain about this case
> (in other words, just make it a loosening of 96b9e0e3).

Yeah, I think that would be better.

How about this?  (Still needs tests.)

 builtin/config.c  |  4 ++--
 config.c          | 10 +++++-----
 dir.c             |  4 ++--
 git-compat-util.h |  5 +++--
 wrapper.c         | 10 ++++++----
 5 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 33c9bf9..19ffcaf 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -379,8 +379,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			 */
 			die("$HOME not set");
 
-		if (access_or_warn(user_config, R_OK) &&
-		    xdg_config && !access_or_warn(xdg_config, R_OK))
+		if (access_or_warn(user_config, R_OK, 0) &&
+		    xdg_config && !access_or_warn(xdg_config, R_OK, 0))
 			given_config_file = xdg_config;
 		else
 			given_config_file = user_config;
diff --git a/config.c b/config.c
index aefd80b..830ee14 100644
--- a/config.c
+++ b/config.c
@@ -58,7 +58,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 		path = buf.buf;
 	}
 
-	if (!access_or_die(path, R_OK)) {
+	if (!access_or_die(path, R_OK, 0)) {
 		if (++inc->depth > MAX_INCLUDE_DEPTH)
 			die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
 			    cf && cf->name ? cf->name : "the command line");
@@ -954,23 +954,23 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 
 	home_config_paths(&user_config, &xdg_config, "config");
 
-	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK)) {
+	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
 					    data);
 		found += 1;
 	}
 
-	if (xdg_config && !access_or_die(xdg_config, R_OK)) {
+	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {
 		ret += git_config_from_file(fn, xdg_config, data);
 		found += 1;
 	}
 
-	if (user_config && !access_or_die(user_config, R_OK)) {
+	if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) {
 		ret += git_config_from_file(fn, user_config, data);
 		found += 1;
 	}
 
-	if (repo_config && !access_or_die(repo_config, R_OK)) {
+	if (repo_config && !access_or_die(repo_config, R_OK, 0)) {
 		ret += git_config_from_file(fn, repo_config, data);
 		found += 1;
 	}
diff --git a/dir.c b/dir.c
index 91cfd99..9cb2f3d 100644
--- a/dir.c
+++ b/dir.c
@@ -1637,9 +1637,9 @@ void setup_standard_excludes(struct dir_struct *dir)
 		home_config_paths(NULL, &xdg_path, "ignore");
 		excludes_file = xdg_path;
 	}
-	if (!access_or_warn(path, R_OK))
+	if (!access_or_warn(path, R_OK, 0))
 		add_excludes_from_file(dir, path);
-	if (excludes_file && !access_or_warn(excludes_file, R_OK))
+	if (excludes_file && !access_or_warn(excludes_file, R_OK, 0))
 		add_excludes_from_file(dir, excludes_file);
 }
 
diff --git a/git-compat-util.h b/git-compat-util.h
index cde442f..51a29b8 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -691,8 +691,9 @@ int remove_or_warn(unsigned int mode, const char *path);
  * Call access(2), but warn for any error except "missing file"
  * (ENOENT or ENOTDIR).
  */
-int access_or_warn(const char *path, int mode);
-int access_or_die(const char *path, int mode);
+#define ACCESS_EACCES_OK (1U << 0)
+int access_or_warn(const char *path, int mode, unsigned flag);
+int access_or_die(const char *path, int mode, unsigned flag);
 
 /* Warn on an inaccessible file that ought to be accessible */
 void warn_on_inaccessible(const char *path);
diff --git a/wrapper.c b/wrapper.c
index bac59d2..e860ad1 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -408,18 +408,20 @@ void warn_on_inaccessible(const char *path)
 	warning(_("unable to access '%s': %s"), path, strerror(errno));
 }
 
-int access_or_warn(const char *path, int mode)
+int access_or_warn(const char *path, int mode, unsigned flag)
 {
 	int ret = access(path, mode);
-	if (ret && errno != ENOENT && errno != ENOTDIR)
+	if (ret && errno != ENOENT && errno != ENOTDIR &&
+	    (!(flag & ACCESS_EACCES_OK) || errno != EACCES))
 		warn_on_inaccessible(path);
 	return ret;
 }
 
-int access_or_die(const char *path, int mode)
+int access_or_die(const char *path, int mode, unsigned flag)
 {
 	int ret = access(path, mode);
-	if (ret && errno != ENOENT && errno != ENOTDIR)
+	if (ret && errno != ENOENT && errno != ENOTDIR &&
+	    (!(flag & ACCESS_EACCES_OK) || errno != EACCES))
 		die_errno(_("unable to access '%s'"), path);
 	return ret;
 }
-- 
1.8.2.1

Re: [PATCH] config: allow inaccessible configuration under $HOME

[Jeff King]

On Fri, Apr 12, 2013 at 12:14:33PM -0700, Jonathan Nieder wrote:

> One unexpected downside to the changes v1.7.12.1~2^2~4 (config: warn
> on inaccessible files, 2012-08-21) and v1.8.1.1~22^2~2 (config: treat
> user and xdg config permission problems as errors, 2012-10-13) is that
> they often trip when git is being run as a server.  The appropriate
> daemon (sshd, inetd, git-daemon, etc) starts as "root", creates a
> listening socket, and then drops privileges, meaning that when git
> commands are invoked they cannot access $HOME and die with
> 
>  fatal: unable to access '/root/.config/git/config': Permission denied
> 
> The intent was always to prevent important configuration (think
> "[transfer] fsckobjects") from being ignored when the configuration is
> unintentionally unreadable (for example with ENOMEM due to a DoS
> attack).  But that is not worth breaking these cases of
> drop-privileges-without-resetting-HOME that were working fine before.
> 
> Treat user and xdg configuration that is inaccessible due to
> permissions (EACCES) as though no user configuration was provided at
> all.

I kind of wonder if we are doing anything with the check at this point.
I suppose ENOMEM and EIO are the only interesting things left at this
point. The resulting code would be much nicer if the patch were just:

  -access_or_die(...);
  +access(...);

but I guess those conditions are still worth checking for, especially if
we think an attacker could trigger ENOMEM intentionally. To be honest, I
am not sure how possible that is, but it is not that hard to do so.

> An alternative approach would be to check if $HOME is readable, but
> that would not solve the problem in cases where the user who dropped
> privileges had a globally readable HOME with only .config or
> .gitconfig being private.

Yeah, although I wonder if those are signs of a misconfiguration that
should be brought to the user's attention (~/.gitconfig I feel more
confident about; less so about $HOME/.config, which might be locked down
for reasons unrelated to git).

> > Given how tight the exception is, I almost wonder if we should drop the
> > environment variable completely, and just never complain about this case
> > (in other words, just make it a loosening of 96b9e0e3).
> 
> Yeah, I think that would be better.
> 
> How about this?  (Still needs tests.)

I think it's probably OK. Like all of the proposed solutions, it is a
bit of compromise about what is worth mentioning to the user and what is
not. But we cannot have our cake and eat it, too, so I'd be fine with
this.

I agree a test would be nice for whatever solution we choose (both to
check that we fail when we should, and do not when we should not).

> -	if (xdg_config && !access_or_die(xdg_config, R_OK)) {
> +	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {

I know you are trying to be flexible with the flag, but I wonder if the
resulting code would read better if we just added a new
"access_or_die_lenient" helper, which would embody the wisdom of
ignoring EACCES, or anything else that comes up later. It seems like all
callers would want either the vanilla form or the lenient form.

I do not feel too strongly about it, though.

> -int access_or_warn(const char *path, int mode)
> +int access_or_warn(const char *path, int mode, unsigned flag)
>  {
>  	int ret = access(path, mode);
> -	if (ret && errno != ENOENT && errno != ENOTDIR)
> +	if (ret && errno != ENOENT && errno != ENOTDIR &&
> +	    (!(flag & ACCESS_EACCES_OK) || errno != EACCES))
>  		warn_on_inaccessible(path);
>  	return ret;
>  }
>  
> -int access_or_die(const char *path, int mode)
> +int access_or_die(const char *path, int mode, unsigned flag)
>  {
>  	int ret = access(path, mode);
> -	if (ret && errno != ENOENT && errno != ENOTDIR)
> +	if (ret && errno != ENOENT && errno != ENOTDIR &&
> +	    (!(flag & ACCESS_EACCES_OK) || errno != EACCES))
>  		die_errno(_("unable to access '%s'"), path);
>  	return ret;
>  }

Now that these conditions are getting more complex, we should perhaps
combine them, like:

  static int access_error_is_ok(int err, int flag)
  {
          return err == ENOENT || errno == ENOTDIR ||
                  (flag & ACCESS_EACCES_OK && err == EACCES);
  }

-Peff

[PATCH] fixup! config: allow inaccessible configuration under $HOME

[Jonathan Nieder]

A cleanup from Jeff King.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 wrapper.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Jeff King wrote:

> I kind of wonder if we are doing anything with the check at this point.
> I suppose ENOMEM and EIO are the only interesting things left at this
> point. The resulting code would be much nicer if the patch were just:
>
>   -access_or_die(...);
>   +access(...);
>
> but I guess those conditions are still worth checking for, especially if
> we think an attacker could trigger ENOMEM intentionally. To be honest, I
> am not sure how possible that is, but it is not that hard to do so.

Yeah, I was tempted to go the plain access() route.  The case that
convinced me not to is that I wouldn't want to think about whether
there could have been a blip in $HOME producing EIO when wondering how
some malformed object had been accepted.  The ENOMEM case just seemed
simpler to explain.

I would also be surprised if it is easy to control kernel ENOMEM
carefully enough to exploit (a more likely DoS outcome is crashing
programs or a kernel assertion failure, which are more harmless in
their way).  I've given up on predicting what is too hard or easy
enough for security folks.  I couldn't think of an obviously easier
vulnerability that is already accepted to put my mind at ease.

[...]
> I know you are trying to be flexible with the flag,

I was mostly worried about damage to readability if we end up needing
to go further down this direction.  The opportunity to look at all
call sites was also nice.

[...]
>   static int access_error_is_ok(int err, int flag)
>   {
>           return err == ENOENT || errno == ENOTDIR ||
>                   (flag & ACCESS_EACCES_OK && err == EACCES);
>   }

Nice.  Here's a patch for squashing in.

diff --git a/wrapper.c b/wrapper.c
index e860ad1..29a866b 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -408,11 +408,16 @@ void warn_on_inaccessible(const char *path)
 	warning(_("unable to access '%s': %s"), path, strerror(errno));
 }
 
+static int access_error_is_ok(int err, unsigned flag)
+{
+	return errno == ENOENT || errno == ENOTDIR ||
+		((flag & ACCESS_EACCES_OK) && errno == EACCES);
+}
+
 int access_or_warn(const char *path, int mode, unsigned flag)
 {
 	int ret = access(path, mode);
-	if (ret && errno != ENOENT && errno != ENOTDIR &&
-	    (!(flag & ACCESS_EACCES_OK) || errno != EACCES))
+	if (ret && !access_error_is_ok(errno, flag))
 		warn_on_inaccessible(path);
 	return ret;
 }
@@ -420,8 +425,7 @@ int access_or_warn(const char *path, int mode, unsigned flag)
 int access_or_die(const char *path, int mode, unsigned flag)
 {
 	int ret = access(path, mode);
-	if (ret && errno != ENOENT && errno != ENOTDIR &&
-	    (!(flag & ACCESS_EACCES_OK) || errno != EACCES))
+	if (ret && !access_error_is_ok(errno, flag))
 		die_errno(_("unable to access '%s'"), path);
 	return ret;
 }
-- 
1.8.2.1

[PATCH v2] config: allow inaccessible configuration under $HOME

[Jonathan Nieder]

The changes v1.7.12.1~2^2~4 (config: warn on inaccessible files,
2012-08-21) and v1.8.1.1~22^2~2 (config: treat user and xdg config
permission problems as errors, 2012-10-13) were intended to prevent
important configuration (think "[transfer] fsckobjects") from being
ignored when the configuration is unintentionally unreadable (for
example with EIO on a flaky filesystem, or with ENOMEM due to a DoS
attack).  Usually ~/.gitconfig and ~/.config/git are readable by the
current user, and if they aren't then it would be easy to fix those
permissions, so the damage from adding this check should have been
minimal.

Unfortunately the access() check often trips when git is being run as
a server.  A daemon (such as inetd or git-daemon) starts as "root",
creates a listening socket, and then drops privileges, meaning that
when git commands are invoked they cannot access $HOME and die with

 fatal: unable to access '/root/.config/git/config': Permission denied

Any patch to fix this would have one of three problems:

  1. We annoy sysadmins who need to take an extra step to handle HOME
     when dropping privileges (the current behavior, or any other
     proposal that they have to opt into).

  2. We annoy sysadmins who want to set HOME when dropping privileges,
     either by making what they want to do impossible, or making them
     set an extra variable or option to accomplish what used to work
     (e.g., a patch to git-daemon to set HOME when --user is passed).

  3. We loosen the check, so some cases which might be noteworthy are
     not caught.

This patch is of type (3).

Treat user and xdg configuration that are inaccessible due to
permissions (EACCES) as though no user configuration was provided at
all.

An alternative method would be to check if $HOME is readable, but that
would not help in cases where the user who dropped privileges had a
globally readable HOME with only .config or .gitconfig being private.

This does not change the behavior when /etc/gitconfig or .git/config
is unreadable (since those are more serious configuration errors),
nor when ~/.gitconfig or ~/.config/git is unreadable due to problems
other than permissions.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Improved-by: Jeff King <peff@peff.net>
---
Jonathan Nieder wrote:

> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -408,11 +408,16 @@ void warn_on_inaccessible(const char *path)
>  	warning(_("unable to access '%s': %s"), path, strerror(errno));
>  }
>  
> +static int access_error_is_ok(int err, unsigned flag)
> +{
> +	return errno == ENOENT || errno == ENOTDIR ||

Sigh, I can't spell "err".  Here's a v2 incorporating that change and
with commit message incorporating the latest discussion.

 builtin/config.c  |  4 ++--
 config.c          | 10 +++++-----
 dir.c             |  4 ++--
 git-compat-util.h |  5 +++--
 wrapper.c         | 14 ++++++++++----
 5 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 33c9bf9..19ffcaf 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -379,8 +379,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			 */
 			die("$HOME not set");
 
-		if (access_or_warn(user_config, R_OK) &&
-		    xdg_config && !access_or_warn(xdg_config, R_OK))
+		if (access_or_warn(user_config, R_OK, 0) &&
+		    xdg_config && !access_or_warn(xdg_config, R_OK, 0))
 			given_config_file = xdg_config;
 		else
 			given_config_file = user_config;
diff --git a/config.c b/config.c
index aefd80b..830ee14 100644
--- a/config.c
+++ b/config.c
@@ -58,7 +58,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 		path = buf.buf;
 	}
 
-	if (!access_or_die(path, R_OK)) {
+	if (!access_or_die(path, R_OK, 0)) {
 		if (++inc->depth > MAX_INCLUDE_DEPTH)
 			die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
 			    cf && cf->name ? cf->name : "the command line");
@@ -954,23 +954,23 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 
 	home_config_paths(&user_config, &xdg_config, "config");
 
-	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK)) {
+	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
 					    data);
 		found += 1;
 	}
 
-	if (xdg_config && !access_or_die(xdg_config, R_OK)) {
+	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {
 		ret += git_config_from_file(fn, xdg_config, data);
 		found += 1;
 	}
 
-	if (user_config && !access_or_die(user_config, R_OK)) {
+	if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) {
 		ret += git_config_from_file(fn, user_config, data);
 		found += 1;
 	}
 
-	if (repo_config && !access_or_die(repo_config, R_OK)) {
+	if (repo_config && !access_or_die(repo_config, R_OK, 0)) {
 		ret += git_config_from_file(fn, repo_config, data);
 		found += 1;
 	}
diff --git a/dir.c b/dir.c
index 91cfd99..9cb2f3d 100644
--- a/dir.c
+++ b/dir.c
@@ -1637,9 +1637,9 @@ void setup_standard_excludes(struct dir_struct *dir)
 		home_config_paths(NULL, &xdg_path, "ignore");
 		excludes_file = xdg_path;
 	}
-	if (!access_or_warn(path, R_OK))
+	if (!access_or_warn(path, R_OK, 0))
 		add_excludes_from_file(dir, path);
-	if (excludes_file && !access_or_warn(excludes_file, R_OK))
+	if (excludes_file && !access_or_warn(excludes_file, R_OK, 0))
 		add_excludes_from_file(dir, excludes_file);
 }
 
diff --git a/git-compat-util.h b/git-compat-util.h
index cde442f..51a29b8 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -691,8 +691,9 @@ int remove_or_warn(unsigned int mode, const char *path);
  * Call access(2), but warn for any error except "missing file"
  * (ENOENT or ENOTDIR).
  */
-int access_or_warn(const char *path, int mode);
-int access_or_die(const char *path, int mode);
+#define ACCESS_EACCES_OK (1U << 0)
+int access_or_warn(const char *path, int mode, unsigned flag);
+int access_or_die(const char *path, int mode, unsigned flag);
 
 /* Warn on an inaccessible file that ought to be accessible */
 void warn_on_inaccessible(const char *path);
diff --git a/wrapper.c b/wrapper.c
index bac59d2..dd7ecbb 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -408,18 +408,24 @@ void warn_on_inaccessible(const char *path)
 	warning(_("unable to access '%s': %s"), path, strerror(errno));
 }
 
-int access_or_warn(const char *path, int mode)
+static int access_error_is_ok(int err, unsigned flag)
+{
+	return err == ENOENT || err == ENOTDIR ||
+		((flag & ACCESS_EACCES_OK) && err == EACCES);
+}
+
+int access_or_warn(const char *path, int mode, unsigned flag)
 {
 	int ret = access(path, mode);
-	if (ret && errno != ENOENT && errno != ENOTDIR)
+	if (ret && !access_error_is_ok(errno, flag))
 		warn_on_inaccessible(path);
 	return ret;
 }
 
-int access_or_die(const char *path, int mode)
+int access_or_die(const char *path, int mode, unsigned flag)
 {
 	int ret = access(path, mode);
-	if (ret && errno != ENOENT && errno != ENOTDIR)
+	if (ret && !access_error_is_ok(errno, flag))
 		die_errno(_("unable to access '%s'"), path);
 	return ret;
 }
-- 
1.8.2.1

Re: [PATCH v2] config: allow inaccessible configuration under $HOME

[Mike Galbraith]

Tested, original setup works fine.

On Fri, 2013-04-12 at 14:03 -0700, Jonathan Nieder wrote: 
> The changes v1.7.12.1~2^2~4 (config: warn on inaccessible files,
> 2012-08-21) and v1.8.1.1~22^2~2 (config: treat user and xdg config
> permission problems as errors, 2012-10-13) were intended to prevent
> important configuration (think "[transfer] fsckobjects") from being
> ignored when the configuration is unintentionally unreadable (for
> example with EIO on a flaky filesystem, or with ENOMEM due to a DoS
> attack).  Usually ~/.gitconfig and ~/.config/git are readable by the
> current user, and if they aren't then it would be easy to fix those
> permissions, so the damage from adding this check should have been
> minimal.
> 
> Unfortunately the access() check often trips when git is being run as
> a server.  A daemon (such as inetd or git-daemon) starts as "root",
> creates a listening socket, and then drops privileges, meaning that
> when git commands are invoked they cannot access $HOME and die with
> 
>  fatal: unable to access '/root/.config/git/config': Permission denied
> 
> Any patch to fix this would have one of three problems:
> 
>   1. We annoy sysadmins who need to take an extra step to handle HOME
>      when dropping privileges (the current behavior, or any other
>      proposal that they have to opt into).
> 
>   2. We annoy sysadmins who want to set HOME when dropping privileges,
>      either by making what they want to do impossible, or making them
>      set an extra variable or option to accomplish what used to work
>      (e.g., a patch to git-daemon to set HOME when --user is passed).
> 
>   3. We loosen the check, so some cases which might be noteworthy are
>      not caught.
> 
> This patch is of type (3).
> 
> Treat user and xdg configuration that are inaccessible due to
> permissions (EACCES) as though no user configuration was provided at
> all.
> 
> An alternative method would be to check if $HOME is readable, but that
> would not help in cases where the user who dropped privileges had a
> globally readable HOME with only .config or .gitconfig being private.
> 
> This does not change the behavior when /etc/gitconfig or .git/config
> is unreadable (since those are more serious configuration errors),
> nor when ~/.gitconfig or ~/.config/git is unreadable due to problems
> other than permissions.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Improved-by: Jeff King <peff@peff.net>
> ---
> Jonathan Nieder wrote:
> 
> > --- a/wrapper.c
> > +++ b/wrapper.c
> > @@ -408,11 +408,16 @@ void warn_on_inaccessible(const char *path)
> >  	warning(_("unable to access '%s': %s"), path, strerror(errno));
> >  }
> >  
> > +static int access_error_is_ok(int err, unsigned flag)
> > +{
> > +	return errno == ENOENT || errno == ENOTDIR ||
> 
> Sigh, I can't spell "err".  Here's a v2 incorporating that change and
> with commit message incorporating the latest discussion.
> 
>  builtin/config.c  |  4 ++--
>  config.c          | 10 +++++-----
>  dir.c             |  4 ++--
>  git-compat-util.h |  5 +++--
>  wrapper.c         | 14 ++++++++++----
>  5 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/builtin/config.c b/builtin/config.c
> index 33c9bf9..19ffcaf 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -379,8 +379,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  			 */
>  			die("$HOME not set");
>  
> -		if (access_or_warn(user_config, R_OK) &&
> -		    xdg_config && !access_or_warn(xdg_config, R_OK))
> +		if (access_or_warn(user_config, R_OK, 0) &&
> +		    xdg_config && !access_or_warn(xdg_config, R_OK, 0))
>  			given_config_file = xdg_config;
>  		else
>  			given_config_file = user_config;
> diff --git a/config.c b/config.c
> index aefd80b..830ee14 100644
> --- a/config.c
> +++ b/config.c
> @@ -58,7 +58,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc
>  		path = buf.buf;
>  	}
>  
> -	if (!access_or_die(path, R_OK)) {
> +	if (!access_or_die(path, R_OK, 0)) {
>  		if (++inc->depth > MAX_INCLUDE_DEPTH)
>  			die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
>  			    cf && cf->name ? cf->name : "the command line");
> @@ -954,23 +954,23 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
>  
>  	home_config_paths(&user_config, &xdg_config, "config");
>  
> -	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK)) {
> +	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
>  		ret += git_config_from_file(fn, git_etc_gitconfig(),
>  					    data);
>  		found += 1;
>  	}
>  
> -	if (xdg_config && !access_or_die(xdg_config, R_OK)) {
> +	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {
>  		ret += git_config_from_file(fn, xdg_config, data);
>  		found += 1;
>  	}
>  
> -	if (user_config && !access_or_die(user_config, R_OK)) {
> +	if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) {
>  		ret += git_config_from_file(fn, user_config, data);
>  		found += 1;
>  	}
>  
> -	if (repo_config && !access_or_die(repo_config, R_OK)) {
> +	if (repo_config && !access_or_die(repo_config, R_OK, 0)) {
>  		ret += git_config_from_file(fn, repo_config, data);
>  		found += 1;
>  	}
> diff --git a/dir.c b/dir.c
> index 91cfd99..9cb2f3d 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1637,9 +1637,9 @@ void setup_standard_excludes(struct dir_struct *dir)
>  		home_config_paths(NULL, &xdg_path, "ignore");
>  		excludes_file = xdg_path;
>  	}
> -	if (!access_or_warn(path, R_OK))
> +	if (!access_or_warn(path, R_OK, 0))
>  		add_excludes_from_file(dir, path);
> -	if (excludes_file && !access_or_warn(excludes_file, R_OK))
> +	if (excludes_file && !access_or_warn(excludes_file, R_OK, 0))
>  		add_excludes_from_file(dir, excludes_file);
>  }
>  
> diff --git a/git-compat-util.h b/git-compat-util.h
> index cde442f..51a29b8 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -691,8 +691,9 @@ int remove_or_warn(unsigned int mode, const char *path);
>   * Call access(2), but warn for any error except "missing file"
>   * (ENOENT or ENOTDIR).
>   */
> -int access_or_warn(const char *path, int mode);
> -int access_or_die(const char *path, int mode);
> +#define ACCESS_EACCES_OK (1U << 0)
> +int access_or_warn(const char *path, int mode, unsigned flag);
> +int access_or_die(const char *path, int mode, unsigned flag);
>  
>  /* Warn on an inaccessible file that ought to be accessible */
>  void warn_on_inaccessible(const char *path);
> diff --git a/wrapper.c b/wrapper.c
> index bac59d2..dd7ecbb 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -408,18 +408,24 @@ void warn_on_inaccessible(const char *path)
>  	warning(_("unable to access '%s': %s"), path, strerror(errno));
>  }
>  
> -int access_or_warn(const char *path, int mode)
> +static int access_error_is_ok(int err, unsigned flag)
> +{
> +	return err == ENOENT || err == ENOTDIR ||
> +		((flag & ACCESS_EACCES_OK) && err == EACCES);
> +}
> +
> +int access_or_warn(const char *path, int mode, unsigned flag)
>  {
>  	int ret = access(path, mode);
> -	if (ret && errno != ENOENT && errno != ENOTDIR)
> +	if (ret && !access_error_is_ok(errno, flag))
>  		warn_on_inaccessible(path);
>  	return ret;
>  }
>  
> -int access_or_die(const char *path, int mode)
> +int access_or_die(const char *path, int mode, unsigned flag)
>  {
>  	int ret = access(path, mode);
> -	if (ret && errno != ENOENT && errno != ENOTDIR)
> +	if (ret && !access_error_is_ok(errno, flag))
>  		die_errno(_("unable to access '%s'"), path);
>  	return ret;
>  }

Re: [PATCH v2] config: allow inaccessible configuration under $HOME

[Jason A. Donenfeld]

Jonathan's patch would indeed be nice. In cgit, we are forced to do
ugly things like this:

        /* Do not look in /etc/ for gitconfig and gitattributes. */
        setenv("GIT_CONFIG_NOSYSTEM", "1", 1);
        setenv("GIT_ATTR_NOSYSTEM", "1", 1);

        /* We unset HOME and XDG_CONFIG_HOME before calling the git
setup function
         * so that we don't make unneccessary filesystem accesses. */
        user_home = getenv("HOME");
        xdg_home = getenv("XDG_CONFIG_HOME");
        unsetenv("HOME");
        unsetenv("XDG_CONFIG_HOME");

        /* Setup the git directory and initialize the notes system.
Both of these
         * load local configuration from the git repository, so we do
them both while
         * the HOME variables are unset. */
        setup_git_directory_gently(&nongit);
        init_display_notes(NULL);

        /* We restore the unset variables afterward. */
        if (user_home)
                setenv("HOME", user_home, 1);
        if (xdg_home)
                setenv("XDG_CONFIG_HOME", xdg_home, 1);

A big nasty guard around the git setup function, so that we don't error out.

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Apr 12, 2013 at 09:08:31AM -0700, Junio C Hamano wrote:
>
>> OK, then...
>
>> -- >8 --
>> Subject: [PATCH] doc: clarify that "git daemon --user=<user>" option does not export HOME=~user
>
> I'd add this motiviation to the body of the commit message:
>
>   The fact that we don't set $HOME may confuse admins who
>   expect $HOME/.gitconfig to be respected. And worse, since
>   96b9e0e3, a git-daemon started by root is likely to fail
>   to run at all, as the user we switch to generally cannot
>   read ~root.
>
> This still feels ugly, like we are documenting some gotcha
> that is going to hit most admins, when we could be helping
> them in the code.

I agree that it feels a bit wrong to sound as if we are blaming the
messanger (the one that notices a possible misconfiguration), but
you are correct that we should make a note on why we think it is a
good idea to add this piece of extra documentation in the history.

Will add the above before queuing.

> One option we have not explored is an environment variable
> to loosen git's requirement. I'm thinking something like
> GIT_INACCESSIBLE_HOMEDIR_OK, which could be set by default
> when git-daemon uses --user.
>
> That would leave all existing setups working, but would
> still enable the extra protections for people not running
> git-daemon (and people who use git via sudo could choose to
> set it, too, if they would prefer that to setting up HOME).

Perhaps.

Right now, the only case people noticed was that we complain when
the effective user cannot even tell if config file(s) exists or not.
Labelling this option as "Treat unreable as missing" is fine, but
"an inaccessible homedir is OK" is vastly different.  Imagine a new
version where we start _requiring_ something to exist (and we read
from it) and imagine further that the expected place of that thing
is somewhere inside $HOME. We cannot keep the promise to those who
set "an inaccessible homedir is OK" option when that happens, as we
may need that piece of information we wanted to read from there in
order to properly operate.

In any case, I think the loosening is an independent issue.

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Mike Galbraith]

On Fri, 2013-04-12 at 09:08 -0700, Junio C Hamano wrote: 
> Jeff King <peff@peff.net> writes:
> 
> >> How about "and make sure any Git configuration files", since there
> >> might not be any Git configuration files.
> >
> > Yeah, that is better. Thanks.
> 
> OK, then...
> 
> -- >8 --
> Subject: [PATCH] doc: clarify that "git daemon --user=<user>" option does not export HOME=~user
> 
> Signed-off-by: Jeff King <peff@peff.net>
> Helped-by: W. Trevor King <wking@tremily.us>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/git-daemon.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
> index 7e5098a..2ac07ba 100644
> --- a/Documentation/git-daemon.txt
> +++ b/Documentation/git-daemon.txt
> @@ -147,6 +147,13 @@ OPTIONS
>  Giving these options is an error when used with `--inetd`; use
>  the facility of inet daemon to achieve the same before spawning
>  'git daemon' if needed.
> ++
> +Like many programs that switch user id, the daemon does not reset
> +environment variables such as `$HOME` when it runs git programs,
> +e.g. `upload-pack` and `receive-pack`. When using this option, you
> +may also want to set and export `HOME` to point at the home
> +directory of `<user>` before starting the daemon, and make sure any
> +Git configuration files in that directory are readable by `<user>`.

The "you may want to.." is perhaps a little understated given it will
fail -EGOAWAY if git-daemon is started via init scripts if you don't.
(but otoh, that's enough of a hint to anyone setting the thing up, no
need to write paragraphs of legal-beagle boiler-plate for dinky bug;)

-Mike 

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Thu, Apr 11, 2013 at 12:54:34PM -0700, Junio C Hamano wrote:

> > I could go either way. I think 96b9e0e is the right thing to do
> > conceptually, but I kind of doubt it was affecting all that many people.
> > And though it's _possible_ for it to be a security problem, I find it
> > much more likely that the site admin tries to set some config, gets
> > annoyed when it doesn't work, and debugs it. So from a practical
> > perspective, 96b9e0e may be doing more harm than good, even though it's
> > the right thing.
> 
> Recent reports in this thread make us think so, I guess.
> 
> But reverting 96b9e0e alone would not help these people very much
> though.  They will have reams of warning messages in their server
> logs, and the way to "fix" it would be the same as the way to work
> around the access_or_die(), namely, to set $HOME to point at a more
> appropriate place before running "git daemon".

Yeah, if we revert 96b9e0e, it would only make sense to revert the
warnings, too. Going halfway does not help anyone.

> I also have a suspicion that your patch makes things worse for
> people who are more adept at these issues around running daemons
> than the people who introduced this problem in the first place (eh,
> that's "us").  It is plausible that they may run multiple instances
> of "initially root but setuid() to an unprivileged user" daemons,
> giving each of them a separate play area by setting $HOME to
> different values, just for management's ease not necessarily for
> security (hence sharing the same unprivileged user), which will be
> broken by the patch that unconditionally overrides $HOME.

Yes, we would definitely be breaking them with this patch. I don't know
how common that is. As you noted, it is a bad idea security-wise (if
everything runs as "nobody", then the services are not insulated from
each other), but I can perhaps see a case where all git repos are owned
by the "git" user, but they may be accessed by different config
profiles, which are managed by $HOME.

You could still accomplish the same thing with git by setting
XDG_CONFIG_HOME, though that of course requires effort from the admin.
Sub-programs may not necessarily respect $XDG_CONFIG_HOME, though (e.g.,
anything run from a post-receive hook). On the other hand, people do not
generally push through git-daemon. But that feels like a weak argument.

-Peff

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Evan Priestley]

Jonathan Nieder <jrnie...@gmail.com> wrote:

> I'm not sure whether to keep 96b9e0e (config: treat user and xdg
> config permission problem as errors) in the long run, BTW.
> 
> Insights welcome.

For what it's worth, here's an anecdote about this:

I work on some open source software which includes a web-based repository browser for Git, somewhat similar to gitweb. We implement this partially by executing `git` commands from the webserver (usually Apache). For example, we run `git cat-file …` to retrieve file content to show to the user.

After this change, a number of users who manage installs of the software are hitting "fatal: unable to access '/root/.config/git/config': Permission denied" while browsing repositories, because their Apache runs as some unprivileged user (like "apache" or "www-data") but with HOME=/root. We've seen about half a dozen reports of this now, so I believe this sort of setup is at least somewhat common and not just a bizarre one-off user with a broken environment. Users generally have difficulty resolving this error on their own, as it's not obvious that this boils down to an Apache environmental issue.

We'll likely resolve this by running `HOME=/ git ...` instead of `git ...` when we execute commands (or some more finessed version of that, but basically pointing HOME at some dummy readable directory). From cursory investigation, it appears we can't avoid this fatal with more surgical settings like GIT_CONFIG or XDG_CONFIG_HOME, since git still ends up looking in HOME and fataling anyway. This fix is a bit clunky, but not really a big deal.

I imagine our use case is fairly unusual, but I wanted to relate it in case it's helpful in balancing concerns here. If I've missed a more reasonable approach to solving this than redirecting HOME, please let me know, but it looks like that's more or less what the git-daemon patch is doing too.

Thanks,
Evan Priestley