Re: [PATCH] daemon: return "access denied" if a service is not allowed

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 14, 2011, 21:02 UTC
Message-ID: <20111014210251.GD16371@elie.hsd1.il.comcast.net>
In-Reply-To: <20111014192326.GA7713@sigill.intra.peff.net>

Jeff King wrote:

Show 5 quoted lines

> When the git-daemon is asked about an inaccessible
> repository, it simply hangs up the connection without saying
> anything further. This makes it hard to distinguish between
> a repository we cannot access (e.g., due to typo), and a
> service or network outage.

*nod*

> Instead, let's print an "ERR" line, which git clients
> understand since v1.6.1 (2008-12-24).

Just to be clear, "git archive --remote" does not understand ERR lines in the 'master' branch (though 908aaceb makes it understand them in 'next'). But I consider even distinguishing

 a. fatal: git archive: protocol error
 b. fatal: git archive: expected ACK/NAK, got EOF

[(a) is how an ERR response is reported, and (b) a remote hangup] to be progress, so it's not so important. :)

> Because there is a risk of leaking information about
> non-exported repositories, by default all errors simply say
> "access denied". Open sites can pass a flag to turn on more
> specific messages.

I'm not sure what an "open site" is. :) But having this flag for sites to declare whether they consider whether a repository exists to be privileged information seems reasonable to me.

Note that this really would be privileged information in some not-too-weird cases. For example, if many users have a repository at ~/.git, ~/.config/.git, or ~/src/linux/.git, then someone might try to access

	/home/alice/.git
	/home/alice/.config/.git
	/home/alice/src/linux/.git
	/home/bob/.git
	...

in turn to find a valid username, as reconnaisance for a later attack not involving git.

Luckily, this can be avoided with

	git daemon --user-path=public_git --base-path=/pub/git

which only allows access to subdirectories of /pub/git and public_git in home directories. With

	git daemon --base-path=/pub/git

there is still no problem, since access to home directories is not allowed at all in that case. I suppose the documentation should mention that --informative-errors is best not used unless --base-path is also in use. (Almost everyone is already using --base-path, so this isn't a very serious problem.)

> --- a/daemon.c
> +++ b/daemon.c
> @@ -20,6 +20,7 @@

[...]

Show 13 quoted lines

> @@ -1167,6 +1176,14 @@ int main(int argc, char **argv)
>  			make_service_overridable(arg + 18, 0);
>  			continue;
>  		}
> +		if (!prefixcmp(arg, "--informative-errors")) {
> +			informative_errors = 1;
> +			continue;
> +		}
> +		else if (!prefixcmp(arg, "--no-informative-errors")) {
> +			informative_errors = 0;
> +			continue;
> +		}
>  		if (!strcmp(arg, "--")) {

Micronit: uncuddled "else".  The style of the surrounding code is to
just not include the "else" at all and rely on "continue" to
short-circuit things.

Anyway, except for the documentation nits mentioned above (and Junio's nit, too),

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

Thanks a lot for this.