AW: [PATCH] Fix http-backend reading till EOF, ignoring CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-backend.exe as iis cgi

[Florian Manschwetus <manschwetus@cs-software-gmbh.de>]

> -----Ursprüngliche Nachricht-----
> Von: Jeff King [mailto:peff@peff.net]
> Gesendet: Dienstag, 29. März 2016 22:14
> An: Florian Manschwetus
> Cc: Chris Packham; Konstantin Khomoutov; git@vger.kernel.org
> Betreff: Re: [PATCH] Fix http-backend reading till EOF, ignoring
> CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-
> backend.exe as iis cgi
> 
> On Tue, Mar 29, 2016 at 10:38:23AM +0000, Florian Manschwetus wrote:
> 
> > > | A request-body is supplied with the request if the CONTENT_LENGTH
> > > | is not NULL.  The server MUST make at least that many bytes
> > > | available for the script to read.  The server MAY signal an
> > > | end-of-file condition after CONTENT_LENGTH bytes have been read or
> > > | it MAY supply extension data.  Therefore, the script MUST NOT
> > > | attempt to read more than CONTENT_LENGTH bytes, even if more data
> > > | is available.  However, it is not obliged to read any of the data.
> > >
> > > So yes, if Git currently reads until EOF, it's an error.
> > > The correct way would be:
> > >
> > > 1) Check to see if the CONTENT_LENGTH variable is available in the
> > >    environment.  If no, read nothing.
> > >
> > > 2) Otherwise read as many bytes it specifies, and no more.
> > >
> > > 1. https://www.ietf.org/rfc/rfc3875
> 
> I don't think the second part of (1) will work very well if the client sends a
> chunked transfer-encoding (which git will do if the input is large). In such a
> case the server would either have to buffer the entire input to find its length,
> or stream the data to the CGI without setting $CONTENT_LENGTH. At least
> some servers choose the latter (including Apache).
> 
> > diff --git a/http-backend.c b/http-backend.c index 8870a26..94976df
> > 100644
> > --- a/http-backend.c
> > +++ b/http-backend.c
> > @@ -277,16 +277,32 @@ static struct rpc_service *select_service(const
> char *name)
> >   */
> >  static ssize_t read_request(int fd, unsigned char **out)  {
> > -	size_t len = 0, alloc = 8192;
> > -	unsigned char *buf = xmalloc(alloc);
> > +	unsigned char *buf = null;
> > ...
> 
> git-am complained that your patch did not apply, but after writing something
> similar locally, I found that t5551.25 hangs indefinitely.
> Which is not surprising. Most tests are doing very limited ref negotiation, so
> the content that hits read_request() here is small, and we send it in a single
> write with a content-length header. But t5551.25 uses a much bigger
> workload, which causes the client to use a chunked transfer-encoding, and
> this code to refuse to read anything (and then the protocol stalls, as we are
> waiting for the client to say something).
> 
> So I think you'd want to take a missing CONTENT_LENGTH as a hint to read
> until EOF.
> 
> That also raises another issue: what happens in the paths that don't hit
> read_request()? We may also process input via:
> 
>   - inflate_request(), if the client gzipped it; for well-formed input,
>     I think we'll stop reading when the gzip stream tells us there is no
>     more data, but a malformed one would have us reading until EOF,
>     regardless of what $CONTENT_LENGTH says.
> 
>   - for input which we expect to be large (like incoming packfiles for a
>     push), buffer_input will be unset, and we will pass the descriptor
>     directly to a sub-program like git-index-pack. Again, for
>     well-formed input it would read just the packfile, but it may
>     actually continue to EOF.
> 
> So I don't think your patch is covering all cases.
> 
> -Peff

After additional analysis it turned out, that in the case you mentioned, at least IIS, sets CONTENT_LENGTH to -1 resulting in the current behavior of git-http-backend being sufficient in this situation.
Therefore I refactored the code again a bit, to match up the behavior I currently fake by using some bash magic...

From ccd6c88e39a850b253979b785463719cdc0fa1e2 Mon Sep 17 00:00:00 2001
From: manschwetus <manschwetus@cs-software-gmbh.de>
Date: Tue, 29 Mar 2016 12:16:21 +0200
Subject: [PATCH 1/2] Fix http-backend reading till EOF, ignoring
 CONTENT_LENGTH, violating rfc3875

Signed-off-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
---
 http-backend.c | 48 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index 8870a26..94976df 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -277,16 +277,32 @@ static struct rpc_service *select_service(const char *name)
  */
 static ssize_t read_request(int fd, unsigned char **out)
 {
-	size_t len = 0, alloc = 8192;
-	unsigned char *buf = xmalloc(alloc);
+	unsigned char *buf = null;
+	size_t len = 0;
+	/* get request size */
+	size_t req_len = git_env_ulong("CONTENT_LENGTH",
+					   0);
+
+	/* check request size */
+	if (max_request_buffer < req_len) {
+		die("request was larger than our maximum size (%lu);"
+			    " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+			    max_request_buffer);
+	}
+
+	if (req_len <= 0) {
+		*out = null;
+		return 0;
+	}
+
+	/* allocate buffer */
+	buf = xmalloc(req_len)
 
-	if (max_request_buffer < alloc)
-		max_request_buffer = alloc;
 
 	while (1) {
 		ssize_t cnt;
 
-		cnt = read_in_full(fd, buf + len, alloc - len);
+		cnt = read_in_full(fd, buf + len, req_len - len);
 		if (cnt < 0) {
 			free(buf);
 			return -1;
@@ -294,21 +310,18 @@ static ssize_t read_request(int fd, unsigned char **out)
 
 		/* partial read from read_in_full means we hit EOF */
 		len += cnt;
-		if (len < alloc) {
+		if (len < req_len) {
+			/* TODO request incomplete?? */
+			/* maybe just remove this block and condition along with the loop, */
+			/* if read_in_full is prooven reliable */
 			*out = buf;
 			return len;
+		} else {
+			/* request complete */
+			*out = buf;
+			return len;
+			
 		}
-
-		/* otherwise, grow and try again (if we can) */
-		if (alloc == max_request_buffer)
-			die("request was larger than our maximum size (%lu);"
-			    " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
-			    max_request_buffer);
-
-		alloc = alloc_nr(alloc);
-		if (alloc > max_request_buffer)
-			alloc = max_request_buffer;
-		REALLOC_ARRAY(buf, alloc);
 	}
 }
 
-- 
2.7.2.windows.1


From 4b2aac3dfd4954098190745a9e4fa17f254cd6a1 Mon Sep 17 00:00:00 2001
From: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
Date: Wed, 30 Mar 2016 10:54:21 +0200
Subject: [PATCH 2/2] restored old behavior as read_request_eof(...) and moved
 new variant to read_request_fix_len(...) and introduced read_request(...) as
 wrapper, which decides based on value retrieved from CONTENT_LENGTH which
 variant to use

Signed-off-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
---
 http-backend.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 63 insertions(+), 4 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index 94976df..3aa0446 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -275,13 +275,52 @@ static struct rpc_service *select_service(const char *name)
  * hit max_request_buffer we die (we'd rather reject a
  * maliciously large request than chew up infinite memory).
  */
-static ssize_t read_request(int fd, unsigned char **out)
+static ssize_t read_request_eof(int fd, unsigned char **out)
+{
+        size_t len = 0, alloc = 8192;
+        unsigned char *buf = xmalloc(alloc);
+
+        if (max_request_buffer < alloc)
+                max_request_buffer = alloc;
+
+        while (1) {
+                ssize_t cnt;
+
+                cnt = read_in_full(fd, buf + len, alloc - len);
+                if (cnt < 0) {
+                        free(buf);
+                        return -1;
+                }
+
+                /* partial read from read_in_full means we hit EOF */
+                len += cnt;
+                if (len < alloc) {
+                        *out = buf;
+                        return len;
+                }
+
+                /* otherwise, grow and try again (if we can) */
+                if (alloc == max_request_buffer)
+                        die("request was larger than our maximum size (%lu);"
+                            " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+                            max_request_buffer);
+
+                alloc = alloc_nr(alloc);
+                if (alloc > max_request_buffer)
+                        alloc = max_request_buffer;
+                REALLOC_ARRAY(buf, alloc);
+        }
+}
+
+/*
+ * replacement for original read_request, now renamed to read_request_eof,
+ * honoring given content_length (req_len),
+ * provided by new wrapper function read_request
+ */
+static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char **out)
 {
 	unsigned char *buf = null;
 	size_t len = 0;
-	/* get request size */
-	size_t req_len = git_env_ulong("CONTENT_LENGTH",
-					   0);
 
 	/* check request size */
 	if (max_request_buffer < req_len) {
@@ -325,6 +364,26 @@ static ssize_t read_request(int fd, unsigned char **out)
 	}
 }
 
+/**
+ * wrapper function, whcih determines based on CONTENT_LENGTH value,
+ * to
+ * - use old behaviour of read_request, to read until EOF
+ * => read_request_eof(...)
+ * - just read CONTENT_LENGTH-bytes, when provided
+ * => read_request_fix_len(...)
+ */
+static ssize_t read_request(int fd, unsigned char **out)
+{
+        /* get request size */
+        size_t req_len = git_env_ulong("CONTENT_LENGTH",
+                                           -1);
+        if (req_len < 0){
+          read_request_eof(fd, out);
+        } else {
+          read_request_fix_len(fd, req_len, out);
+        }
+}
+
 static void inflate_request(const char *prog_name, int out, int buffer_input)
 {
 	git_zstream stream;
-- 
2.7.2.windows.1


Mit freundlichen Grüßen / With kind regards
Florian Manschwetus
 
CS Software Concepts and Solutions GmbH
Geschäftsführer / Managing director: Dr. Werner Alexi 
Amtsgericht Wiesbaden HRB 10004 (Commercial registry)
Schiersteiner Straße 31
D-65187 Wiesbaden
Germany