Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> The message is chosen to avoid leaking information, yet let users know
> that they are deliberately not allowed to use the service, not a fault
> in service configuration or the service itself.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> ---
> OK let's try again. I don't send ERR when faults happen in
> service->fn() (eventually run_service_command) because
>
> - if it's start_command(), it's likely due to service configuration
> fault (wrong --exec-path..)
>
> - if it's finish_command(), the service may have run and sent
> something back to users. We may break the protocol by sending ERR
>
> daemon.c | 12 ++++++++----
> 1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index 4c8346d..f0cae24 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -257,11 +257,11 @@ static int run_service(char *dir, struct daemon_service *service)
> if (!enabled && !service->overridable) {
> logerror("'%s': service not enabled.", service->name);
> errno = EACCES;
> - return -1;
> + goto failed;
> }
>
> if (!(path = path_ok(dir)))
> - return -1;
> + goto failed;
>
> /*
> * Security on the cheap.
> @@ -277,7 +277,7 @@ static int run_service(char *dir, struct daemon_service *service)
> if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
> logerror("'%s': repository not exported.", path);
> errno = EACCES;
> - return -1;
> + goto failed;
> }
>
> if (service->overridable) {
> @@ -291,7 +291,7 @@ static int run_service(char *dir, struct daemon_service *service)
> logerror("'%s': service not enabled for '%s'",
> service->name, path);
> errno = EACCES;
> - return -1;
> + goto failed;
> }
>
> /*
> @@ -301,6 +301,10 @@ static int run_service(char *dir, struct daemon_service *service)
> signal(SIGTERM, SIG_IGN);
>
> return service->fn();
> +
> +failed:
> + packet_write(1, "ERR %s: access denied", dir);
> + return -1;
> }
This looks better.
I think telling "dir" back to the user is probably safe (it is not
affected by what path_ok() does).
I briefly wondered if we also want to say which service failed, but "dir"
is much more likely to be typoed and deserves to be parroted back to help
the user realize mistakes, while the service name is not something the
user usually types, so the balance the patch strikes is probably the
optimal.
Thanks.
--
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