Commit Diff

Commit:
3fdc457c8db0550a6143ab626bfefe3351ab0b93
From:
Omar Polo <op@omarpolo.com>
Date:
Message:
swap try_client_by_id with client_by_id i.e. allow client_by_id to fail and return NULL. Initially I thought it was a good idea to shut down a server process if we receive an invalid client id as reply from one of our requests to the executor process. This turned out not to be correct since a client can (read: will) disconnect in the delay beteewn we acknowledge their request and the cgi script execution. The fastcgi and proxy handler already handled this situation, so they're unaffected. This allows an attacker to make gmid unresponsible by just making enough requests until they hit the right timing.
Actions:
Patch | Tree
commit - 409a2599b30159207a7d4da6a7fd7aede4a4327f
commit + 3fdc457c8db0550a6143ab626bfefe3351ab0b93
blob - a6741bf4fd67f104871baa9f9210d8b9d2eee535
blob + 6dd1932f3d033e363dc7f9fc893dcab62f562998
--- gmid.h
+++ gmid.h
@@ -373,7 +373,7 @@ void		 mark_nonblock(int);
 void		 client_write(struct bufferevent *, void *);
 void		 start_reply(struct client*, int, const char*);
 void		 client_close(struct client *);
-struct client	*try_client_by_id(int);
+struct client	*client_by_id(int);
 void		 loop(struct tls*, int, int, struct imsgbuf*);
 
 int		 client_tree_cmp(struct client *, struct client *);
blob - fa8da5decd780633d30948e510a96774c1839848
blob + a66e4ea3d4ac1b607b67d653e648ad75eab918e5
--- server.c
+++ server.c
@@ -70,7 +70,6 @@ static void	 cgi_write(struct bufferevent *, void *);
 static void	 cgi_error(struct bufferevent *, short, void *);
 
 static void	 do_accept(int, short, void*);
-static struct client *client_by_id(int);
 
 static void	 handle_imsg_cgi_res(struct imsgbuf*, struct imsg*, size_t);
 static void	 handle_imsg_fcgi_fd(struct imsgbuf*, struct imsg*, size_t);
@@ -1421,18 +1420,8 @@ do_accept(int sock, short et, void *d)
 	connected_clients++;
 }
 
-static struct client *
-client_by_id(int id)
-{
-	struct client *c;
-
-	if ((c = try_client_by_id(id)) == NULL)
-		fatal("in client_by_id: invalid id %d", id);
-	return c;
-}
-
 struct client *
-try_client_by_id(int id)
+client_by_id(int id)
 {
 	struct client find;
 
@@ -1445,7 +1434,11 @@ handle_imsg_cgi_res(struct imsgbuf *ibuf, struct imsg 
 {
 	struct client *c;
 
-	c = client_by_id(imsg->hdr.peerid);
+	if ((c = client_by_id(imsg->hdr.peerid)) == NULL) {
+		if (imsg->fd != -1)
+			close(imsg->fd);
+		return;
+	}
 
 	if ((c->pfd = imsg->fd) == -1) {
 		start_reply(c, TEMP_FAILURE, "internal server error");
@@ -1468,7 +1461,7 @@ handle_imsg_fcgi_fd(struct imsgbuf *ibuf, struct imsg 
 
 	id = imsg->hdr.peerid;
 
-	if ((c = try_client_by_id(id)) == NULL) {
+	if ((c = client_by_id(id)) == NULL) {
 		if (imsg->fd != -1)
 			close(imsg->fd);
 		return;
@@ -1499,7 +1492,7 @@ handle_imsg_conn_fd(struct imsgbuf *ibuf, struct imsg 
 	int		 id;
 
 	id = imsg->hdr.peerid;
-	if ((c = try_client_by_id(id)) == NULL) {
+	if ((c = client_by_id(id)) == NULL) {
 		if (imsg->fd != -1)
 			close(imsg->fd);
 		return;