From f5f2256c00abbfd02c22fbae3937da1c7bd8a34f Mon Sep 17 00:00:00 2001
From: Masakazu Kitajo <maskit@apache.org>
Date: Fri, 3 Jan 2025 14:39:10 -0700
Subject: [PATCH] Do not allow extra CRs in chunks (#11936)

* Do not allow extra CRs in chunks

* Renumber test uuid

* Add test cases and fix an oversight

* Use prefix increment
---
 src/proxy/http/HttpTunnel.cc                  | 12 +++++
 .../bad_chunked_encoding.test.py              |  6 +--
 .../malformed_chunked_header.replay.yaml      | 49 +++++++++++++++++--
 3 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/src/proxy/http/HttpTunnel.cc b/src/proxy/http/HttpTunnel.cc
index a72ad36fc43..631ded94fd1 100644
--- a/src/proxy/http/HttpTunnel.cc
+++ b/src/proxy/http/HttpTunnel.cc
@@ -148,6 +148,7 @@ ChunkedHandler::read_size()
 {
   int64_t bytes_used;
   bool    done = false;
+  int     cr   = 0;
 
   while (chunked_reader->read_avail() > 0 && !done) {
     const char *tmp       = chunked_reader->start();
@@ -186,6 +187,9 @@ ChunkedHandler::read_size()
             done  = true;
             break;
           } else {
+            if (ParseRules::is_cr(*tmp)) {
+              ++cr;
+            }
             state = CHUNK_READ_SIZE_CRLF; // now look for CRLF
           }
         }
@@ -195,7 +199,15 @@ ChunkedHandler::read_size()
           cur_chunk_bytes_left = (cur_chunk_size = running_sum);
           state                = (running_sum == 0) ? CHUNK_READ_TRAILER_BLANK : CHUNK_READ_CHUNK;
           done                 = true;
+          cr                   = 0;
           break;
+        } else if (ParseRules::is_cr(*tmp)) {
+          if (cr != 0) {
+            state = CHUNK_READ_ERROR;
+            done  = true;
+            break;
+          }
+          ++cr;
         }
       } else if (state == CHUNK_READ_SIZE_START) {
         if (ParseRules::is_cr(*tmp)) {
diff --git a/tests/gold_tests/chunked_encoding/bad_chunked_encoding.test.py b/tests/gold_tests/chunked_encoding/bad_chunked_encoding.test.py
index 547ac399daf..c3ded527950 100644
--- a/tests/gold_tests/chunked_encoding/bad_chunked_encoding.test.py
+++ b/tests/gold_tests/chunked_encoding/bad_chunked_encoding.test.py
@@ -172,13 +172,13 @@ def runChunkedTraffic(self):
         # code from the verifier client.
         tr.Processes.Default.ReturnCode = 1
         tr.Processes.Default.Streams.stdout += Testers.ContainsExpression(
-            r"(Unexpected chunked content for key 4: too small|Failed HTTP/1 transaction with key: 4)",
+            r"(Unexpected chunked content for key 101: too small|Failed HTTP/1 transaction with key: 101)",
             "Verify that ATS closed the forth transaction.")
         tr.Processes.Default.Streams.stdout += Testers.ContainsExpression(
-            r"(Unexpected chunked content for key 5: too small|Failed HTTP/1 transaction with key: 5)",
+            r"(Unexpected chunked content for key 102: too small|Failed HTTP/1 transaction with key: 102)",
             "Verify that ATS closed the fifth transaction.")
         tr.Processes.Default.Streams.stdout += Testers.ContainsExpression(
-            r"(Unexpected chunked content for key 6: too small|Failed HTTP/1 transaction with key: 6)",
+            r"(Unexpected chunked content for key 103: too small|Failed HTTP/1 transaction with key: 103)",
             "Verify that ATS closed the sixth transaction.")
 
         # ATS should close the connection before any body gets through. "def"
diff --git a/tests/gold_tests/chunked_encoding/replays/malformed_chunked_header.replay.yaml b/tests/gold_tests/chunked_encoding/replays/malformed_chunked_header.replay.yaml
index f12995c234a..b428141aca9 100644
--- a/tests/gold_tests/chunked_encoding/replays/malformed_chunked_header.replay.yaml
+++ b/tests/gold_tests/chunked_encoding/replays/malformed_chunked_header.replay.yaml
@@ -78,6 +78,26 @@ sessions:
     server-response:
       status: 200
 
+- transactions:
+  - client-request:
+      method: "POST"
+      version: "1.1"
+      url: /malformed/chunk/header3
+      headers:
+        fields:
+        - [ Host, example.com ]
+        - [ Transfer-Encoding, chunked ]
+        - [ uuid, 4 ]
+      content:
+        transfer: plain
+        encoding: uri
+        # BWS cannot have CR
+        data: 3%0D%0D%0Aabc%0D%0A0%0D%0A%0D%0A
+
+    # The connection will be dropped and this response will not go out.
+    server-response:
+      status: 200
+
   #
   # Now repeat the above two malformed chunk header tests, but on the server
   # side.
@@ -90,7 +110,7 @@ sessions:
       headers:
         fields:
         - [ Host, example.com ]
-        - [ uuid, 4 ]
+        - [ uuid, 101 ]
 
     # The connection will be dropped and this response will not go out.
     server-response:
@@ -113,7 +133,7 @@ sessions:
       headers:
         fields:
         - [ Host, example.com ]
-        - [ uuid, 5 ]
+        - [ uuid, 102 ]
 
     # The connection will be dropped and this response will not go out.
     server-response:
@@ -136,7 +156,7 @@ sessions:
       headers:
         fields:
         - [ Host, example.com ]
-        - [ uuid, 6 ]
+        - [ uuid, 103 ]
 
     # The connection will be dropped and this response will not go out.
     server-response:
@@ -150,3 +170,26 @@ sessions:
         encoding: uri
         # Super large chunk header, larger than will fit in an int.
         data: 111111113%0D%0Adef%0D%0A0%0D%0A%0D%0A
+
+- transactions:
+  - client-request:
+      method: "GET"
+      version: "1.1"
+      url: /response/malformed/chunk/size2
+      headers:
+        fields:
+        - [ Host, example.com ]
+        - [ uuid, 104 ]
+
+    # The connection will be dropped and this response will not go out.
+    server-response:
+      status: 200
+      reason: OK
+      headers:
+        fields:
+        - [ Transfer-Encoding, chunked ]
+      content:
+        transfer: plain
+        encoding: uri
+        # BWS cannot have CR
+        data: 3%0D%0D%0Aabc%0D%0A0%0D%0A%0D%0A