Fix: null pointer dereference in ftppr / smtpp

op_rewrite checked what it built with targetunsafe(), which describes a
path on this machine: on Windows it must name a drive or a share, so a
rewritten request path was refused and every rewrite rule failed there.
A rewrite produces a request path and is checked as one.

The header loop stopped at HTTPSRV_MAXHDR and answered anyway, leaving
the rest of the request in the stream for the next one to be read out
of. It refuses the request instead. A header longer than the buffer
arrives as several lines, so the count bounds what is read rather than
what a client may send in one header.

The test for the buffer growth after a PCRE rewrite sent its request
through to an httpsrv origin, which stops reading at that same cap; it
uses an origin which reads whatever it is sent, since what is under test
is the proxy in the middle.

src/ftppr.c

@@ -64,6 +64,13 @@ void * ftpprchild(struct clientparam* param) {
 
 	}
 	else if (!strncasecmp((char *)buf, "PASS ", 5)){
+		/* The user name carries the server to log in to, and it arrives
+		   with USER. Without it there is nothing to log in to, and what
+		   follows would read the name and the host as if there were. */
+		if(!param->hostname || !param->extusername){
+			socksend(param, param->ctrlsock, (unsigned char *)"503 Login with USER first\r\n", 27, conf.timeouts[STRING_S]);
+			RETURN(805);
+		}
 		param->extpassword = (unsigned char *)strdup((char *)buf+5);
 		inbuf = BUFSIZE;
 		res = ftplogin(param, (char *)buf, &inbuf);

src/httpsrv.c

@@ -937,7 +937,11 @@ static int op_rewrite(struct httpreq *r, const unsigned char *params)
 	if(!params || !*params) return op_forbidden(r);
 	if(expand(path, sizeof(path), params, r->path, r->caps, r->ncaps))
 		return op_forbidden(r);
-	if(targetunsafe(path) || path[0] != '/') return op_forbidden(r);
+	/* What comes out is a request path and is checked as one. A path on this
+	   machine is a different thing, and asking a rewrite to look like one
+	   would leave nothing writable on Windows, where such a path names a
+	   drive or a share. */
+	if(path[0] != '/' || pathunsafe(path)) return op_forbidden(r);
 	strcpy(r->path, path);
 	return HTTPSRV_REWRITTEN;
 }
@@ -1327,9 +1331,12 @@ static int httpsrv_request(struct clientparam *param, struct httpreq *r)
 		strcpy(r->path, decoded);
 	}
 
-	while(hdrs++ < HTTPSRV_MAXHDR &&
+	/* A header longer than the buffer arrives as several lines, so the count
+	   bounds what is read and not what a client may send in one header. */
+	while(hdrs < HTTPSRV_MAXHDR &&
 	      (i = sockgetlinebuf(param, CLIENT, (unsigned char *)buf, sizeof(buf) - 1,
 			'\n', conf.timeouts[STRING_S])) > 2){
+		hdrs++;
 		if(rawkeep(r, buf, i)) RETURN(710);
 		buf[i] = 0;
 		if(!strncasecmp(buf, "host:", 5) && !r->proxy){
@@ -1384,6 +1391,11 @@ static int httpsrv_request(struct clientparam *param, struct httpreq *r)
 		}
 	}
 
+	/* The headers ran past what this server reads, so the rest of the request
+	   is still in the stream and there is no answering it: what follows would
+	   be read as the request after this one. */
+	if(hdrs >= HTTPSRV_MAXHDR) RETURN(712);
+
 	/* The next request begins where this body ends, so a body which cannot
 	   be read to its end - one this server does not frame, or one longer
 	   than it is willing to read - closes the connection instead.

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

src/smtpp.c

@@ -159,7 +159,10 @@ void * smtppchild(struct clientparam* param) {
 	i = de64(buf,username,255);
 	if(i < 1) {RETURN(664);}
 	username[i] = 0;
-	parseconnusername((char *)username, param, 0, 587);
+	/* The name has to carry the host to connect to, and the answer says
+	   whether it did: without one there is nowhere to go, and what follows
+	   reads the name as if there were. */
+	if(parseconnusername((char *)username, param, 0, 587)) {RETURN(669);}
 	socksend(param, param->clisock, (unsigned char *)"334 UGFzc3dvcmQ6\r\n", 18,conf.timeouts[STRING_S]);	
 	i = sockgetlinebuf(param, CLIENT, buf, sizeof(buf) - 10, '\n', conf.timeouts[STRING_S]);
 	if(i < 2) {RETURN(665);}
@@ -184,7 +187,7 @@ void * smtppchild(struct clientparam* param) {
 	}
 	if(i < 3 || *username) {RETURN(668);}
 	username[i] = 0;
-	parseconnusername((char *)username+1, param, 0, 587);
+	if(parseconnusername((char *)username+1, param, 0, 587)) {RETURN(670);}
 	res = (int)strlen((char *)username+1) + 2;
 	if(res < i){
 		if(param->extpassword) free(param->extpassword);

tests/cases/pcre.py

@@ -193,3 +193,43 @@ 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.
+    # The origin here reads whatever it is sent and answers the same way every
+    # time: what is being tested is the proxy in the middle, not what a server
+    # is willing to accept in one request.
+    grown = t.free_port()
+    stop = t.raw_server(grown, b"HTTP/1.1 200 OK\r\nContent-Length: 2\r\n\r\nok",
+                        drain=True)
+    try:
+        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, f"http://127.0.0.1:{grown}/x",
+                                    extra=big, body="z")
+        t.contains(reply, "200", "a doubled header block with a body is answered")
+        t.contains(t.raw_proxy_request(p, f"http://127.0.0.1:{grown}/x"), "200",
+                   "and the proxy is still there afterwards")
+    finally:
+        stop()
+
+    # 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

@@ -22,6 +22,7 @@ configurations it needs, starts them, and states what it expects:
 import base64
 import http.client
 import os
+import random
 import shutil
 import socket
 import ssl
@@ -33,6 +34,11 @@ import threading
 import time
 
 
+# Ports handed out in this run: a service which has finished may still be
+# in TIME_WAIT, and another case binding the same port would fail for it.
+_PORTS_TAKEN = set()
+
+
 class Response:
     """A reply, or the reason there wasn't one."""
 
@@ -146,14 +152,30 @@ class Tester:
             sock.close()
 
     def free_port(self):
-        """A port nothing is listening on. Closed again before it is used,
+        """A port nothing is listening on, and nothing is likely to take.
-        which is racy in principle and reliable enough in practice."""
+
-        s = socket.socket()
+        Asking the system for an ephemeral port hands back one out of the
+        range it also draws outgoing connections from - 32768 up on Linux,
+        49152 up on Windows - so between the check here and the bind in the
+        service, a connection somewhere else in the suite can take it. That
+        shows up as a service which never listens, or a bind() error deep in
+        a case which has nothing to do with ports. Ports are taken from below
+        both ranges instead, and none is handed out twice in a run.
+        """
+        for _ in range(200):
+            port = random.randint(10000, 19999)
+            if port in _PORTS_TAKEN:
+                continue
+            sock = socket.socket()
             try:
-            s.bind(("127.0.0.1", 0))
+                sock.bind(("127.0.0.1", port))
-            return s.getsockname()[1]
+            except OSError:
+                continue
             finally:
-            s.close()
+                sock.close()
+            _PORTS_TAKEN.add(port)
+            return port
+        raise RuntimeError("no free port in the range the suite uses")
 
     def write_config(self, name, config):
         path = os.path.join(self.tmpdir, name + ".cfg")
@@ -358,12 +380,31 @@ class Tester:
             return f"<no reply: {exc}>", True
         return b"".join(chunks).decode("utf-8", "replace"), closed
 
-    def raw_server(self, port, reply, close_after=True, host="127.0.0.1"):
+    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",
+                   drain=False):
         """Answer every connection with fixed bytes. Returns a stop function.
 
         For the shapes a real server would have to be talked into: an answer
         whose body is delimited by the close, or one which promises to stay
-        and does not.
+        and does not. drain reads the whole request first, however large,
+        which is what a test of the sending side needs.
         """
         sock = socket.socket()
         sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
@@ -378,8 +419,13 @@ class Tester:
                 except OSError:
                     break
                 try:
-                    conn.settimeout(self.timeout)
+                    conn.settimeout(0.5 if drain else self.timeout)
-                    conn.recv(65536)
+                    while True:
+                        try:
+                            if not conn.recv(65536) or not drain:
+                                break
+                        except socket.timeout:
+                            break       # it has stopped sending
                     conn.sendall(reply)
                     if close_after:
                         conn.close()