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:


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

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

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to