From: Nils Goroll <nils.goroll@uplex.de>

Until now, we read the (CR)?LF at the end of a chunk as part of the
next chunk header (see: /* Skip leading whitespace */).

For a follow up commit, we are going to want to know if the next chunk
header is available for read, so we now consume the chunk end as part
of the chunk itself.

This also fixes a corner case: We previously accepted chunks with a
missing end-of-chunk (see fix of r01729.vtc).

Ref: https://datatracker.ietf.org/doc/html/rfc7230#section-4.1
---
 bin/varnishd/http1/cache_http1_vfp.c | 36 ++++++++++++++++++++--------
 bin/varnishtest/tests/r01184.vtc     |  2 ++
 bin/varnishtest/tests/r01506.vtc     | 16 ++++++-------
 bin/varnishtest/tests/r01729.vtc     |  6 ++---
 4 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/bin/varnishd/http1/cache_http1_vfp.c b/bin/varnishd/http1/cache_http1_vfp.c
index 20f349d1c..aceb5a628 100644
--- a/bin/varnishd/http1/cache_http1_vfp.c
+++ b/bin/varnishd/http1/cache_http1_vfp.c
@@ -89,6 +89,24 @@ v1f_read(const struct vfp_ctx *vc, struct http_conn *htc, void *d, ssize_t len)
 }
 
 
+/*--------------------------------------------------------------------
+ * read (CR)?LF at the end of a chunk
+ */
+static enum vfp_status
+v1f_chunk_end(struct vfp_ctx *vc, struct http_conn *htc)
+{
+	char c;
+
+	if (v1f_read(vc, htc, &c, 1) <= 0)
+		return (VFP_Error(vc, "chunked read err"));
+	if (c == '\r' && v1f_read(vc, htc, &c, 1) <= 0)
+		return (VFP_Error(vc, "chunked read err"));
+	if (c != '\n')
+		return (VFP_Error(vc, "chunked tail no NL"));
+	return (VFP_OK);
+}
+
+
 /*--------------------------------------------------------------------
  * Read a chunked HTTP object.
  *
@@ -99,6 +117,7 @@ static enum vfp_status v_matchproto_(vfp_pull_f)
 v1f_chunked_pull(struct vfp_ctx *vc, struct vfp_entry *vfe, void *ptr,
     ssize_t *lp)
 {
+	static enum vfp_status vfps;
 	struct http_conn *htc;
 	char buf[20];		/* XXX: 20 is arbitrary */
 	char *q;
@@ -168,18 +187,15 @@ v1f_chunked_pull(struct vfp_ctx *vc, struct vfp_entry *vfe, void *ptr,
 			return (VFP_Error(vc, "chunked insufficient bytes"));
 		*lp = lr;
 		vfe->priv2 -= lr;
-		if (vfe->priv2 == 0)
-			vfe->priv2 = -1;
-		return (VFP_OK);
+		if (vfe->priv2 != 0)
+			return (VFP_OK);
+
+		vfe->priv2 = -1;
+		return (v1f_chunk_end(vc, htc));
 	}
 	AZ(vfe->priv2);
-	if (v1f_read(vc, htc, buf, 1) <= 0)
-		return (VFP_Error(vc, "chunked read err"));
-	if (buf[0] == '\r' && v1f_read(vc, htc, buf, 1) <= 0)
-		return (VFP_Error(vc, "chunked read err"));
-	if (buf[0] != '\n')
-		return (VFP_Error(vc, "chunked tail no NL"));
-	return (VFP_END);
+	vfps = v1f_chunk_end(vc, htc);
+	return (vfps == VFP_OK ? VFP_END : vfps);
 }
 
 static const struct vfp v1f_chunked = {
diff --git a/bin/varnishtest/tests/r01184.vtc b/bin/varnishtest/tests/r01184.vtc
index 0988e65a3..94ecd3c23 100644
--- a/bin/varnishtest/tests/r01184.vtc
+++ b/bin/varnishtest/tests/r01184.vtc
@@ -62,6 +62,7 @@ server s1 {
 		sendhex " 10 45 f3 a9 83 b8 18 1c  7b c2 30 55 04 17 13 c4"
 		sendhex " 0f 07 5f 7a 38 f4 8e 50  b3 37 d4 3a 32 4a 34 07"
 		sendhex " FF FF FF FF FF FF FF FF  72 ea 06 5f b3 1c fa dd"
+	send "\n"
 	expect_close
 } -start
 
@@ -93,6 +94,7 @@ server s1 {
 		sendhex " 10 45 f3 a9 83 b8 18 1c  7b c2 30 55 04 17 13 c4"
 		sendhex " 0f 07 5f 7a 38 f4 8e 50  b3 37 d4 3a 32 4a 34 07"
 		sendhex " FF FF FF FF FF FF FF FF  72 ea 06 5f b3 1c fa dd"
+	send "\n"
 	expect_close
 } -start
 
diff --git a/bin/varnishtest/tests/r01506.vtc b/bin/varnishtest/tests/r01506.vtc
index 96b7b54c9..f7f89a716 100644
--- a/bin/varnishtest/tests/r01506.vtc
+++ b/bin/varnishtest/tests/r01506.vtc
@@ -7,15 +7,15 @@ server s0 {
 	txresp -nolen \
 	    -hdr "Transfer-Encoding: chunked" \
 	    -hdr "Connection: close"
-	send "11\r\n0_23456789abcdef\n"
-	send "11\r\n1_23456789abcdef\n"
-	send "11\r\n2_23456789abcdef\n"
-	send "11\r\n3_23456789abcdef\n"
+	send "11\r\n0_23456789abcdef\n\n"
+	send "11\r\n1_23456789abcdef\n\n"
+	send "11\r\n2_23456789abcdef\n\n"
+	send "11\r\n3_23456789abcdef\n\n"
 	barrier b1 sync
-	send "11\r\n4_23456789abcdef\n"
-	send "11\r\n5_23456789abcdef\n"
-	send "11\r\n6_23456789abcdef\n"
-	send "11\r\n7_23456789abcdef\n"
+	send "11\r\n4_23456789abcdef\n\n"
+	send "11\r\n5_23456789abcdef\n\n"
+	send "11\r\n6_23456789abcdef\n\n"
+	send "11\r\n7_23456789abcdef\n\n"
 	chunkedlen 0
 
 } -dispatch
diff --git a/bin/varnishtest/tests/r01729.vtc b/bin/varnishtest/tests/r01729.vtc
index 883a60cc6..f6a01e976 100644
--- a/bin/varnishtest/tests/r01729.vtc
+++ b/bin/varnishtest/tests/r01729.vtc
@@ -11,7 +11,7 @@ server s1 {
 	send "\r\n"
 	send "14\r\n"
 	send "0123456789"
-	send "0123456789"
+	send "0123456789\n"
 	send "0\r\n"
 	send "\r\n"
 
@@ -29,7 +29,7 @@ client c1 {
 	send "\r\n"
 	send "14\r\n"
 	send "0123456789"
-	send "0123456789"
+	send "0123456789\n"
 	send "0\r\n"
 	send "\r\n"
 
@@ -45,7 +45,7 @@ client c1 {
 	send "\r\n"
 	send "14\r\n"
 	send "0123456789"
-	send "0123456789"
+	send "0123456789\n"
 	send "0\r\n"
 	send "\r\n"
 
-- 
2.39.5
From: Nils Goroll <nils.goroll@uplex.de>

... which we are going to need in a follow up commit.

No functional changes, diff best viewed with -b
---
 bin/varnishd/http1/cache_http1_vfp.c | 122 ++++++++++++++++-----------
 1 file changed, 72 insertions(+), 50 deletions(-)

diff --git a/bin/varnishd/http1/cache_http1_vfp.c b/bin/varnishd/http1/cache_http1_vfp.c
index aceb5a628..d684f1043 100644
--- a/bin/varnishd/http1/cache_http1_vfp.c
+++ b/bin/varnishd/http1/cache_http1_vfp.c
@@ -108,76 +108,98 @@ v1f_chunk_end(struct vfp_ctx *vc, struct http_conn *htc)
 
 
 /*--------------------------------------------------------------------
- * Read a chunked HTTP object.
+ * Parse a chunk header and, for VFP_OK, return size in a pointer
  *
  * XXX: Reading one byte at a time is pretty pessimal.
  */
 
-static enum vfp_status v_matchproto_(vfp_pull_f)
-v1f_chunked_pull(struct vfp_ctx *vc, struct vfp_entry *vfe, void *ptr,
-    ssize_t *lp)
+static enum vfp_status
+v1f_chunked_hdr(struct vfp_ctx *vc, struct http_conn *htc, ssize_t *szp)
 {
-	static enum vfp_status vfps;
-	struct http_conn *htc;
 	char buf[20];		/* XXX: 20 is arbitrary */
-	char *q;
 	unsigned u;
 	uintmax_t cll;
-	ssize_t cl, l, lr;
+	ssize_t cl, lr;
+	char *q;
 
 	CHECK_OBJ_NOTNULL(vc, VFP_CTX_MAGIC);
-	CHECK_OBJ_NOTNULL(vfe, VFP_ENTRY_MAGIC);
-	CAST_OBJ_NOTNULL(htc, vfe->priv1, HTTP_CONN_MAGIC);
-	AN(ptr);
-	AN(lp);
+	CHECK_OBJ_NOTNULL(htc, HTTP_CONN_MAGIC);
+	AN(szp);
+	assert(*szp == -1);
 
-	l = *lp;
-	*lp = 0;
-	if (vfe->priv2 == -1) {
-		/* Skip leading whitespace */
-		do {
-			lr = v1f_read(vc, htc, buf, 1);
-			if (lr <= 0)
-				return (VFP_Error(vc, "chunked read err"));
-		} while (vct_islws(buf[0]));
-
-		if (!vct_ishex(buf[0]))
-			 return (VFP_Error(vc, "chunked header non-hex"));
-
-		/* Collect hex digits, skipping leading zeros */
-		for (u = 1; u < sizeof buf; u++) {
-			do {
-				lr = v1f_read(vc, htc, buf + u, 1);
-				if (lr <= 0)
-					return (VFP_Error(vc, "chunked read err"));
-			} while (u == 1 && buf[0] == '0' && buf[u] == '0');
-			if (!vct_ishex(buf[u]))
-				break;
-		}
+	/* Skip leading whitespace */
+	do {
+		lr = v1f_read(vc, htc, buf, 1);
+		if (lr <= 0)
+			return (VFP_Error(vc, "chunked read err"));
+	} while (vct_islws(buf[0]));
 
-		if (u >= sizeof buf)
-			return (VFP_Error(vc, "chunked header too long"));
+	if (!vct_ishex(buf[0]))
+		return (VFP_Error(vc, "chunked header non-hex"));
 
-		/* Skip trailing white space */
-		while (vct_islws(buf[u]) && buf[u] != '\n') {
+	/* Collect hex digits, skipping leading zeros */
+	for (u = 1; u < sizeof buf; u++) {
+		do {
 			lr = v1f_read(vc, htc, buf + u, 1);
 			if (lr <= 0)
 				return (VFP_Error(vc, "chunked read err"));
-		}
+		} while (u == 1 && buf[0] == '0' && buf[u] == '0');
+		if (!vct_ishex(buf[u]))
+			break;
+	}
 
-		if (buf[u] != '\n')
-			return (VFP_Error(vc, "chunked header no NL"));
+	if (u >= sizeof buf)
+		return (VFP_Error(vc, "chunked header too long"));
+
+	/* Skip trailing white space */
+	while (vct_islws(buf[u]) && buf[u] != '\n') {
+		lr = v1f_read(vc, htc, buf + u, 1);
+		if (lr <= 0)
+			return (VFP_Error(vc, "chunked read err"));
+	}
 
-		buf[u] = '\0';
+	if (buf[u] != '\n')
+		return (VFP_Error(vc, "chunked header no NL"));
 
-		cll = strtoumax(buf, &q, 16);
-		if (q == NULL || *q != '\0')
-			return (VFP_Error(vc, "chunked header number syntax"));
-		cl = (ssize_t)cll;
-		if (cl < 0 || (uintmax_t)cl != cll)
-			return (VFP_Error(vc, "bogusly large chunk size"));
+	buf[u] = '\0';
 
-		vfe->priv2 = cl;
+	cll = strtoumax(buf, &q, 16);
+	if (q == NULL || *q != '\0')
+		return (VFP_Error(vc, "chunked header number syntax"));
+	cl = (ssize_t)cll;
+	if (cl < 0 || (uintmax_t)cl != cll)
+		return (VFP_Error(vc, "bogusly large chunk size"));
+
+	*szp = cl;
+	return (VFP_OK);
+}
+
+
+/*--------------------------------------------------------------------
+ * Read a chunked HTTP object.
+ *
+ */
+
+static enum vfp_status v_matchproto_(vfp_pull_f)
+v1f_chunked_pull(struct vfp_ctx *vc, struct vfp_entry *vfe, void *ptr,
+    ssize_t *lp)
+{
+	static enum vfp_status vfps;
+	struct http_conn *htc;
+	ssize_t l, lr;
+
+	CHECK_OBJ_NOTNULL(vc, VFP_CTX_MAGIC);
+	CHECK_OBJ_NOTNULL(vfe, VFP_ENTRY_MAGIC);
+	CAST_OBJ_NOTNULL(htc, vfe->priv1, HTTP_CONN_MAGIC);
+	AN(ptr);
+	AN(lp);
+
+	l = *lp;
+	*lp = 0;
+	if (vfe->priv2 == -1) {
+		vfps = v1f_chunked_hdr(vc, htc, &vfe->priv2);
+		if (vfps != VFP_OK)
+			return (vfps);
 	}
 	if (vfe->priv2 > 0) {
 		if (vfe->priv2 < l)
-- 
2.39.5
From: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>

It's a little bit harder to follow the CRLF logic when it is intertwined
with the skipped surrounding white space.
---
 bin/varnishd/http1/cache_http1_vfp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/bin/varnishd/http1/cache_http1_vfp.c b/bin/varnishd/http1/cache_http1_vfp.c
index d684f1043..fbc3dcb8b 100644
--- a/bin/varnishd/http1/cache_http1_vfp.c
+++ b/bin/varnishd/http1/cache_http1_vfp.c
@@ -132,7 +132,7 @@ v1f_chunked_hdr(struct vfp_ctx *vc, struct http_conn *htc, ssize_t *szp)
 		lr = v1f_read(vc, htc, buf, 1);
 		if (lr <= 0)
 			return (VFP_Error(vc, "chunked read err"));
-	} while (vct_islws(buf[0]));
+	} while (vct_isows(buf[0]));
 
 	if (!vct_ishex(buf[0]))
 		return (VFP_Error(vc, "chunked header non-hex"));
@@ -152,12 +152,14 @@ v1f_chunked_hdr(struct vfp_ctx *vc, struct http_conn *htc, ssize_t *szp)
 		return (VFP_Error(vc, "chunked header too long"));
 
 	/* Skip trailing white space */
-	while (vct_islws(buf[u]) && buf[u] != '\n') {
+	while (vct_isows(buf[u])) {
 		lr = v1f_read(vc, htc, buf + u, 1);
 		if (lr <= 0)
 			return (VFP_Error(vc, "chunked read err"));
 	}
 
+	if (buf[u] == '\r' && v1f_read(vc, htc, buf + u, 1) <= 0)
+		return (VFP_Error(vc, "chunked read err"));
 	if (buf[u] != '\n')
 		return (VFP_Error(vc, "chunked header no NL"));
 
-- 
2.39.5
From: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>

---
 bin/varnishtest/tests/f00017.vtc | 69 ++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 bin/varnishtest/tests/f00017.vtc

diff --git a/bin/varnishtest/tests/f00017.vtc b/bin/varnishtest/tests/f00017.vtc
new file mode 100644
index 000000000..a38b8b1ef
--- /dev/null
+++ b/bin/varnishtest/tests/f00017.vtc
@@ -0,0 +1,69 @@
+varnishtest "Do not tolerate anything else than CRLF as chunked ending"
+
+server s0 {
+	rxreq
+	expect_close
+} -dispatch
+
+varnish v1 -vcl+backend {} -start
+
+logexpect l1 -v v1 {
+	expect * 1001 FetchError "chunked tail no NL"
+	expect * 1004 FetchError "chunked tail no NL"
+	expect * 1007 FetchError "chunked header non-hex"
+	expect * 1010 FetchError "chunked header non-hex"
+} -start
+
+client c1 {
+	non_fatal
+	txreq -req POST -hdr "Transfer-encoding: chunked"
+	send "1\r\n"
+	send "This is more than one byte of data\r\n"
+	send "0\r\n"
+	send "\r\n"
+	fatal
+	rxresp
+	expect resp.status == 503
+	expect_close
+} -run
+
+client c2 {
+	non_fatal
+	txreq -req POST -hdr "Transfer-encoding: chunked"
+	send "1\r\n"
+	send "Z  2\r\n"
+	send "3d\r\n"
+	send "0\r\n\r\nPOST /evil HTTP/1.1\r\nHost: whatever\r\nContent-Length: 5\r\n\r\n"
+	send "0\r\n"
+	send "\r\n"
+	fatal
+	rxresp
+	expect resp.status == 503
+	expect_close
+} -run
+
+client c3 {
+	non_fatal
+	txreq -req POST -hdr "Transfer-encoding: chunked"
+	send "d\r\n"
+	send "Spurious CRLF\r\n\r\n"
+	send "0\r\n"
+	send "\r\n"
+	fatal
+	rxresp
+	expect resp.status == 503
+	expect_close
+} -run
+
+client c4 {
+	non_fatal
+	txreq -req POST -hdr "Transfer-encoding: chunked"
+	send "\n0\r\n"
+	send "\r\n"
+	fatal
+	rxresp
+	expect resp.status == 503
+	expect_close
+} -run
+
+logexpect l1 -wait
-- 
2.39.5