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

-----Ursprüngliche Nachricht-----
Von: Chris Packham [mailto:judge.packham@gmail.com] 
Gesendet: Dienstag, 29. März 2016 11:28
An: Florian Manschwetus
Cc: Konstantin Khomoutov; <redacted>
Betreff: Re: Problem with git-http-backend.exe as iis cgi

Hi Florian

On Tue, Mar 29, 2016 at 7:01 PM, Florian Manschwetus [off-list ref] wrote:

Hi,
I put together a first patch for the issue.

Mit freundlichen Grüßen / With kind regards Florian Manschwetus

E-Mail: manschwetus@cs-software-gmbh.de
Tel.: +49-(0)611-8908534

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
Tel.: 0611/8908555


-----Ursprüngliche Nachricht-----
Von: Konstantin Khomoutov [mailto:kostix+git@007spb.ru]
Gesendet: Donnerstag, 10. März 2016 13:55
An: Florian Manschwetus
Cc: <redacted>
Betreff: Re: Problem with git-http-backend.exe as iis cgi

On Thu, 10 Mar 2016 07:28:50 +0000
Florian Manschwetus [off-list ref] wrote:

quoted

I tried to setup git-http-backend with iis, as iis provides proper 
impersonation for cgi under windows, which leads to have the 
filesystem access performed with the logon user, therefore the 
webserver doesn't need generic access to the files. I stumbled across 
a problem, ending up with post requests hanging forever. After some 
investigation I managed to get it work by wrapping the http-backend 
into a bash script, giving a lot of control about the environmental 
things, I was unable to solve within IIS configuration. The 
workaround, I use currently, is to use "/bin/head -c 
${CONTENT_LENGTH}
| ./git-http-backend.exe", which directly shows the issue. Git
http-backend should check if CONTENT_LENGTH is set to something 
reasonable (e.g. >0) and should in this case read only CONTENT_LENGTH 
bytes from stdin, instead of reading till EOF what I suspect it is 
doing currently.

The rfc [1] states in its section 4.2:

| 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

Your patch description seems well thought out but if you want someone to notice it you should have a read of https://git.kernel.org/cgit/git/git.git/tree/Documentation/SubmittingPatches

Moin,
I have cloned git and created a more clean patch...

Signed-off-by: Florian Manschwetus <redacted>
---
 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);
 	}
 }
 

@@ -701,3 +714,4 @@ int main(int argc, char **argv)
 	cmd->imp(cmd_arg);
 	return 0;
 }
+

-- 
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