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

From: Jeff King <peff@peff.net>
Date: Apr 12, 2013, 19:37 UTC
Message-ID: <20130412193755.GA5329@sigill.intra.peff.net>
In-Reply-To: <20130412191433.GR27070@google.com>

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

Show 19 quoted lines

> 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).

Show 7 quoted lines

> > 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.

Show 21 quoted lines

> -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