Fix: proxy buffer may be insufficient after PCRE rewrite

src/pcre.c

@@ -265,6 +265,9 @@ static FILTER_ACTION pcre_filter_client(void *fo, struct clientparam * param, vo
 	return (res)? CONTINUE:PASS;
 }
 
+/* What a rewritten buffer keeps free for its caller to append to. */
+#define PCRE_HEADROOM 1024
+
 static FILTER_ACTION pcre_filter_buffer(void *fc, struct clientparam *param, unsigned char ** buf_p, int * bufsize_p, int offset, int * length_p){
 	PCRE2_SIZE *ovector;
 	int count = 0;
@@ -324,12 +327,17 @@ static FILTER_ACTION pcre_filter_buffer(void *fc, struct clientparam *param, uns
 			else if(*replace == '$' && isnumber(*(replace+1))){
 				replace ++;
 				num = atoi(replace);
+				/* Past the digits first, and only then decide whether
+				   the group is one to copy: the pass which measured
+				   this string did it in that order, and a reference it
+				   counted as nothing must not be written out as its
+				   own digits here. */
+				while(isnumber(*replace)) replace++;
 				if(num > (count - 1)) continue;
 				if(ovector[(num<<1)] == PCRE2_UNSET) continue;
 				if(ovector[(num<<1) + 1] > (PCRE2_SIZE)*length_p || ovector[(num<<1)] > ovector[(num<<1) + 1]) continue;
 				memcpy(target, *buf_p + ovector[(num<<1)], ovector[(num<<1) + 1] - ovector[(num<<1)]);
 				target += (ovector[(num<<1) + 1] - ovector[(num<<1)]);
-				while(isnumber(*replace)) replace++;
 			}
 			else {
 				*target++ = *replace++;
@@ -338,7 +346,13 @@ static FILTER_ACTION pcre_filter_buffer(void *fc, struct clientparam *param, uns
 		repsz = (int)(target - tmpbuf);
 		memcpy(target, *buf_p + ovector[1], *length_p - ovector[1]);
 		if((ovector[0] + replen + 1) > *bufsize_p){
-			newbuf = pl->mallocfunc(ovector[0] + replen + 1);
+			/* Room beyond what was produced: whoever asked for the
+			   filtering usually has something of its own to add, and a
+			   buffer sized to the last byte written leaves nowhere to
+			   put it. The size reported is the size allocated. */
+			int newsize = ovector[0] + replen + 1 + PCRE_HEADROOM;
+
+			newbuf = pl->mallocfunc(newsize);
 			if(!newbuf){
 				pl->freefunc(tmpbuf);
 				return CONTINUE;
@@ -346,7 +360,7 @@ static FILTER_ACTION pcre_filter_buffer(void *fc, struct clientparam *param, uns
 			memcpy(newbuf, *buf_p, ovector[0]);
 			pl->freefunc(*buf_p);
 			*buf_p = (unsigned char *)newbuf;
-			*bufsize_p = ovector[0] + replen + 1;
+			*bufsize_p = newsize;
 		}
 		memcpy(*buf_p + ovector[0], tmpbuf, replen);
 		pl->freefunc(tmpbuf);

src/proxy.c

@@ -132,6 +132,12 @@ char * proxy_stringtable[] = {
 };
 
 #define LINESIZE 32768
+/* "Content-Length: " plus 20 digits plus CRLF and a NUL, rounded up */
+#define CLHDRSIZE 48
+/* what the headers this proxy adds of its own can come to: a Forwarded or
+   Via with a host name in it, a Connection, a Proxy-support and a
+   Proxy-Authorization carrying an encoded user and password */
+#define HDRRESERVE 2048
 #define BUFSIZE (LINESIZE*2)
 #define FTPBUFSIZE 1536
 
@@ -151,6 +157,20 @@ static int send_st(struct clientparam *param, int idx){
 	return socksend(param, param->clisock, (unsigned char *)proxy_stringtable[idx], pst_len(idx), conf.timeouts[STRING_S]);
 }
 
+/* Makes room in a buffer whose size is tracked. A filter may hand back one
+   holding exactly what it produced, so nothing may be added to it without
+   asking for the room first. Returns 1 when the room cannot be had. */
+static int growbuf(unsigned char **buf, int *bufsize, int need){
+	unsigned char *newbuf;
+
+	if(need <= *bufsize) return 0;
+	need += BUFSIZE;		/* for what follows too, not just this */
+	if(!(newbuf = realloc(*buf, need))) return 1;
+	*buf = newbuf;
+	*bufsize = need;
+	return 0;
+}
+
 static void freeptr(void *p){
 	void **pp = (void **)p;
 	if(*pp) { free(*pp); *pp = NULL; }
@@ -648,6 +668,10 @@ for(;;){
 	RETURN(0);
  }
  if(action != PASS) RETURN(517);
+ /* A filter may have returned a buffer sized to exactly what it produced.
+    The headers this proxy adds of its own go in after it, so the room for
+    them is taken back before anything is written. */
+ if(growbuf(&buf, &bufsize, inbuf + HDRRESERVE)) RETURN(21);
  param->nolongdatfilter = 0;
 
 #endif
@@ -681,6 +705,7 @@ for(;;){
 	contentlength64 = param->cliinbuf;
 	param->nolongdatfilter = 1;
   }
+  if(growbuf(&buf, &bufsize, (int)strlen((char *)buf) + CLHDRSIZE)) RETURN(21);
   sprintf((char*)buf+strlen((char *)buf), "Content-Length: %"PRIu64"\r\n", contentlength64);
  }
 
@@ -1158,6 +1183,7 @@ for(;;){
 	RETURN(0);
  }
  if(action != PASS) RETURN(517);
+ if(growbuf(&buf, &bufsize, inbuf + HDRRESERVE)) RETURN(21);
 
  param->nolongdatfilter = 0;
 
@@ -1181,6 +1207,7 @@ for(;;){
 	}
 	if(action != PASS) RETURN(517);
 	contentlength64 = param->srvinbuf;
+	if(growbuf(&buf, &bufsize, (int)strlen((char *)buf) + CLHDRSIZE)) RETURN(21);
 	sprintf((char*)buf+strlen((char *)buf), "Content-Length: %"PRIu64"\r\n", contentlength64);
 	hascontent = 1;
   }

tests/cases/pcre.py

@@ -193,3 +193,33 @@ def run(t):
     r = t.http(url + "/echo/old", proxy=p)
     t.eq(200, r.status, "a rewritten request through a parent arrives")
     t.contains(r, "path=/echo/new", "the origin sees the rewritten path through a parent")
+
+    # --- a rewrite which grows the headers ----------------------------------
+    # GHSA-h845-prxq-ww3q: a rewrite that doubles the client headers used to
+    # leave a buffer holding exactly what it produced, and the Content-Length
+    # the data filter regenerates was then written past the end of it.
+    p = proxy_with("rewrite_grow",
+                   'pcre_rewrite cliheader dunno "(?s).*" "$0$0"',
+                   'pcre clidata dunno *')
+    big = "".join("X-%d: %s\r\n" % (i, chr(65 + i) * 20000) for i in range(5))
+    reply = t.raw_proxy_request(p, url + "/echo", extra=big, body="z")
+    t.contains(reply, "200", "a doubled header block with a body is answered")
+    t.contains(t.http(url + "/echo", proxy=p), "path=/echo",
+               "and the proxy is still there afterwards")
+
+    # A reference to a group the pattern does not have is dropped, and dropped
+    # by both the pass which measures the result and the pass which writes it.
+    p = proxy_with("rewrite_nogroup",
+                   'pcre_rewrite cliheader dunno "(?s)Host:" "$9$9$9$9$9$9$9$9"')
+    r = t.http(url + "/echo", proxy=p, headers={"X-Pad": "P" * 2000})
+    t.eq(200, r.status, "a reference to a group which did not match is left out")
+    t.contains(t.http(url + "/echo", proxy=p), "path=/echo",
+               "and that proxy is still there too")
+
+    # an optional group which took part on one request and not on the next
+    p = proxy_with("rewrite_optgroup",
+                   'pcre_rewrite cliheader dunno "X-Mark: (a)?(b)" "[$1][$2]"')
+    t.eq(200, t.http(url + "/echo", proxy=p, headers={"X-Mark": "ab"}).status,
+         "a group which matched is put in")
+    t.eq(200, t.http(url + "/echo", proxy=p, headers={"X-Mark": "b"}).status,
+         "and one which did not is left out")

tests/harness.py

@@ -358,6 +358,23 @@ class Tester:
             return f"<no reply: {exc}>", True
         return b"".join(chunks).decode("utf-8", "replace"), closed
 
+    def raw_proxy_request(self, proxy, url, extra="", body="", method=None):
+        """Send one absolute-URI request through a proxy, headers and all.
+
+        For the requests a client library will not send: an oversized header
+        block, or one whose exact bytes matter.
+        """
+        phost, pport = self._hostport(proxy)
+        host, port, path = self._split(url)
+        method = method or ("POST" if body else "GET")
+        request = (f"{method} http://{host}:{port}{path} HTTP/1.1\r\n"
+                   f"Host: {host}:{port}\r\n" + extra)
+        if body:
+            request += f"Content-Length: {len(body)}\r\n"
+        request += "\r\n" + body
+        text, _ = self.raw_session(pport, request, host=phost, quiet=2)
+        return text
+
     def raw_server(self, port, reply, close_after=True, host="127.0.0.1"):
         """Answer every connection with fixed bytes. Returns a stop function.