Thread (1 message) 1 message, 1 author, 2016-06-15

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:52:10

Nguyễn Thái Ngọc Duy  [off-list ref] writes:
quoted hunk
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 <redacted>
---
 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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help