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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html