Subject: Re: [PATCH v2 2/2] daemon: report permission denied error to clients

From: Junio C Hamano <[email-masked]>
Date: Fri, 21 Oct 2011 12:25:17 -0700

Clemens Buchacher <[email-masked]> writes:

> diff --git a/daemon.c b/daemon.c
> index 72fb53a..2f7f84e 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -120,12 +120,14 @@ static char *path_ok(char *directory)
>  
>  	if (daemon_avoid_alias(dir)) {
>  		logerror("'%s': aliased", dir);
> +		errno = 0;
>  		return NULL;
>  	}
>  
>  	if (*dir == '~') {
>  		if (!user_path) {
>  			logerror("'%s': User-path not allowed", dir);
> +			errno = EACCES;
>  			return NULL;
>  		}

Isn't the first one inconsistent from all the others?

A request cames "../some/path" and it is not allowed by a daemon policy
and it gets errno==0 which is turned into "no such repo" later, while
another request to "~drizzed/another/path" is also rejected by a daemon
policy and gets errno==EACCESS which is turned into "permission denied".

Indeed everything else says EACCESS in this patch, except for the check
done by enter_repo() which can additionally say ENAMETOOLONG (which would
not be very useful in practice) or whatever error coming from failure to
go there with chdir(), which is not likely to be EACCESS because it has
already been checked with a separate access() that is done before the
actual chdir() call.

> +	if (!(path = path_ok(dir))) {
> +		if (errno == EACCES)
> +			return daemon_error(dir, "permission denied");
> +		else
> +			return daemon_error(dir, "no such repository");
> +	}

If errno is set to EACCESS in cases (1) we are not even going to tell you
if a repository exists there or not--you are not authorized to know and
(2) there is a repository but you do not have authorization to access it,
then this "leaking a bit more information" part is acceptable for site
with "--informative-errors", I would think. A repository that is invalid
from the daemon's point of view (e.g. validate_headref("HEAD") fails
because it points at an object that does not exist) but that the owner
intended to make it valid by correcting such mistakes would be reported as
"no such repository" with such a logic, so I am not sure if the distinction
between these two cases really matters in practice, though.