From 83c89f3d217d473ecb000b68c910c0f183c3a355 Mon Sep 17 00:00:00 2001
From: Brian Neradt <brian.neradt@gmail.com>
Date: Wed, 27 Oct 2021 11:30:54 -0500
Subject: [PATCH] Detect and handle chunk header size truncation (#8452)

This detects if a chunk header size is too large and, if so, closes the
connection.
---
 include/tscore/ink_memory.h                   |  19 +++
 proxy/http/HttpTunnel.cc                      |  11 +-
 src/tscore/Makefile.am                        |   1 +
 src/tscore/unit_tests/test_ink_memory.cc      | 141 ++++++++++++++++++
 .../bad_chunked_encoding.test.py              |  86 ++++++++++-
 .../malformed_chunked_header.replay.yaml      | 109 ++++++++++++++
 6 files changed, 364 insertions(+), 3 deletions(-)
 create mode 100644 src/tscore/unit_tests/test_ink_memory.cc
 create mode 100644 tests/gold_tests/chunked_encoding/replays/malformed_chunked_header.replay.yaml

diff --git a/include/tscore/ink_memory.h b/include/tscore/ink_memory.h
index fdaaa27c21b..7fd8de1f347 100644
--- a/include/tscore/ink_memory.h
+++ b/include/tscore/ink_memory.h
@@ -26,6 +26,7 @@
 #include <cstring>
 #include <strings.h>
 #include <cinttypes>
+#include <limits>
 #include <string>
 #include <string_view>
 
@@ -205,6 +206,24 @@ ink_zero(T &t)
   memset(static_cast<void *>(&t), 0, sizeof(t));
 }
 
+/** Verify that we can safely shift value num_places places left.
+ *
+ * This checks that the shift will not cause the variable to overflow and that
+ * the value will not become negative.
+ *
+ * @param[in] value The value against which to check whether the shift is safe.
+ *
+ * @param[in] num_places The number of places to check that shifting left is safe.
+ *
+ */
+template <typename T>
+inline constexpr bool
+can_safely_shift_left(T value, int num_places)
+{
+  constexpr auto max_value = std::numeric_limits<T>::max();
+  return value >= 0 && value <= (max_value >> num_places);
+}
+
 /** Scoped resources.
 
     An instance of this class is used to hold a contingent resource. When this object goes out of scope
diff --git a/proxy/http/HttpTunnel.cc b/proxy/http/HttpTunnel.cc
index 08837c6290e..564adc23bf6 100644
--- a/proxy/http/HttpTunnel.cc
+++ b/proxy/http/HttpTunnel.cc
@@ -36,6 +36,7 @@
 #include "HttpSM.h"
 #include "HttpDebugNames.h"
 #include "tscore/ParseRules.h"
+#include "tscore/ink_memory.h"
 
 static const int min_block_transfer_bytes = 256;
 static const char *const CHUNK_HEADER_FMT = "%" PRIx64 "\r\n";
@@ -134,8 +135,16 @@ ChunkedHandler::read_size()
       if (state == CHUNK_READ_SIZE) {
         // The http spec says the chunked size is always in hex
         if (ParseRules::is_hex(*tmp)) {
+          // Make sure we will not overflow running_sum with our shift.
+          if (!can_safely_shift_left(running_sum, 4)) {
+            // We have no more space in our variable for the shift.
+            state = CHUNK_READ_ERROR;
+            done  = true;
+            break;
+          }
           num_digits++;
-          running_sum *= 16;
+          // Shift over one hex value.
+          running_sum <<= 4;
 
           if (ParseRules::is_digit(*tmp)) {
             running_sum += *tmp - '0';
diff --git a/src/tscore/Makefile.am b/src/tscore/Makefile.am
index 43750b2a739..c0ca76c670e 100644
--- a/src/tscore/Makefile.am
+++ b/src/tscore/Makefile.am
@@ -174,6 +174,7 @@ test_tscore_SOURCES = \
 	unit_tests/test_Extendible.cc \
 	unit_tests/test_History.cc \
 	unit_tests/test_ink_inet.cc \
+	unit_tests/test_ink_memory.cc \
 	unit_tests/test_IntrusiveHashMap.cc \
 	unit_tests/test_IntrusivePtr.cc \
 	unit_tests/test_IpMap.cc \
diff --git a/src/tscore/unit_tests/test_ink_memory.cc b/src/tscore/unit_tests/test_ink_memory.cc
new file mode 100644
index 00000000000..fa6725b84ca
--- /dev/null
+++ b/src/tscore/unit_tests/test_ink_memory.cc
@@ -0,0 +1,141 @@
+/** @file
+
+    ink_memory unit tests.
+
+    @section license License
+
+    Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+    Unless required by applicable law or agreed to in writing, software
+    distributed under the License is distributed on an "AS IS" BASIS,
+    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+    See the License for the specific language governing permissions and
+    limitations under the License.
+*/
+
+#include <catch.hpp>
+#include <cstdint>
+#include "tscore/ink_memory.h"
+
+constexpr void
+test_can_safely_shift_int8_t()
+{
+  constexpr int8_t a = 0;
+  static_assert(can_safely_shift_left(a, 0) == true, "shifting 0 is safe");
+  static_assert(can_safely_shift_left(a, 4) == true, "shifting 0 is safe");
+  static_assert(can_safely_shift_left(a, 8) == true, "shifting 0 is safe");
+
+  constexpr int8_t b = 1;
+  static_assert(can_safely_shift_left(b, 0) == true, "shifting int8_t 1 0 places is safe");
+  static_assert(can_safely_shift_left(b, 1) == true, "shifting int8_t 1 1 places is safe");
+  static_assert(can_safely_shift_left(b, 6) == true, "shifting int8_t 1 6 places is safe");
+  static_assert(can_safely_shift_left(b, 7) == false, "shifting int8_t 1 7 places becomes negative");
+  static_assert(can_safely_shift_left(b, 8) == false, "shifting int8_t 1 8 places overflows");
+
+  constexpr int8_t c = 0xff;
+  static_assert(can_safely_shift_left(c, 0) == false, "int8_t 0xff is already negative");
+  static_assert(can_safely_shift_left(c, 1) == false, "shifting int8_t 0xff 1 place overflows");
+}
+
+constexpr void
+test_can_safely_shift_uint8_t()
+{
+  constexpr uint8_t a = 0;
+  static_assert(can_safely_shift_left(a, 0) == true, "shifting 0 is safe");
+  static_assert(can_safely_shift_left(a, 4) == true, "shifting 0 is safe");
+  static_assert(can_safely_shift_left(a, 8) == true, "shifting 0 is safe");
+
+  constexpr uint8_t b = 1;
+  static_assert(can_safely_shift_left(b, 0) == true, "shifting uint8_t 1 0 places is safe");
+  static_assert(can_safely_shift_left(b, 1) == true, "shifting uint8_t 1 1 places is safe");
+  static_assert(can_safely_shift_left(b, 6) == true, "shifting uint8_t 1 6 places is safe");
+  static_assert(can_safely_shift_left(b, 7) == true, "shifting uint8_t 1 7 is safe");
+  static_assert(can_safely_shift_left(b, 8) == false, "shifting uint8_t 1 8 places overflows");
+
+  constexpr uint8_t c = 0xff;
+  static_assert(can_safely_shift_left(c, 0) == true, "shifting int8_t 0xff 0 places is safe");
+  static_assert(can_safely_shift_left(c, 1) == false, "shifting int8_t 0xff 1 place overflows");
+}
+
+constexpr void
+test_can_safely_shift_int32_t()
+{
+  constexpr int32_t a = 0;
+  static_assert(can_safely_shift_left(a, 4) == true, "shifting 0 is safe");
+
+  constexpr int32_t b = 1;
+  static_assert(can_safely_shift_left(b, 4) == true, "shifting 1 is safe");
+
+  constexpr int32_t c = 0x00ff'ffff;
+  static_assert(can_safely_shift_left(c, 4) == true, "shifting 0x00ff'ffff is safe");
+
+  constexpr int32_t d = 0x07ff'ffff;
+  static_assert(can_safely_shift_left(d, 4) == true, "shifting 0x07ff'ffff is safe");
+
+  constexpr int32_t e = -1;
+  static_assert(can_safely_shift_left(e, 4) == false, "shifting -1 will result in truncation");
+
+  constexpr int32_t f = 0x0800'0000;
+  static_assert(can_safely_shift_left(f, 4) == false, "shifting 0x0801'0000 will become negative");
+
+  constexpr int32_t g = 0x0fff'ffff;
+  static_assert(can_safely_shift_left(g, 4) == false, "shifting 0x0fff'ffff will become negative");
+
+  constexpr int32_t h = 0x1000'0000;
+  static_assert(can_safely_shift_left(h, 4) == false, "shifting 0x1000'0000 will overflow");
+
+  constexpr int32_t i = 0xf000'0000;
+  static_assert(can_safely_shift_left(i, 4) == false, "shifting 0xf000'0000 will overflow");
+
+  constexpr int32_t j = 0xf800'0000;
+  static_assert(can_safely_shift_left(j, 4) == false, "shifting 0xf800'0000 will become negative");
+}
+
+constexpr void
+test_can_safely_shift_uint32_t()
+{
+  constexpr uint32_t a = 0;
+  static_assert(can_safely_shift_left(a, 4) == true, "shifting 0 is safe");
+
+  constexpr uint32_t b = 1;
+  static_assert(can_safely_shift_left(b, 4) == true, "shifting 1 is safe");
+
+  constexpr uint32_t c = 0x00ff'ffff;
+  static_assert(can_safely_shift_left(c, 4) == true, "shifting 0x00ff'ffff is safe");
+
+  constexpr uint32_t d = 0x07ff'ffff;
+  static_assert(can_safely_shift_left(d, 4) == true, "shifting 0x07ff'ffff is safe");
+
+  constexpr uint32_t e = 0x0800'0000;
+  static_assert(can_safely_shift_left(e, 4) == true, "shifting unisgned 0x0800'0000 is safe");
+
+  constexpr uint32_t f = 0x0fff'ffff;
+  static_assert(can_safely_shift_left(f, 4) == true, "shifting unsigned 0x0fff'ffff is safe");
+
+  constexpr uint32_t g = 0x1000'0000;
+  static_assert(can_safely_shift_left(g, 4) == false, "shifting 0x1000'0000 will overflow");
+
+  constexpr uint32_t h = 0xf000'0000;
+  static_assert(can_safely_shift_left(h, 4) == false, "shifting 0xf000'0000 will overflow");
+
+  constexpr uint32_t i = 0xf800'0000;
+  static_assert(can_safely_shift_left(i, 4) == false, "shifting 0xf800'0000 will become negative");
+}
+
+TEST_CASE("can_safely_shift", "[libts][ink_inet][memory]")
+{
+  // can_safely_shift_left is a constexpr function, therefore all these checks are
+  // done at compile time and REQUIRES calls are not necessary.
+  test_can_safely_shift_int8_t();
+  test_can_safely_shift_uint8_t();
+  test_can_safely_shift_int32_t();
+  test_can_safely_shift_uint32_t();
+}
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 cdfc0bf9cd2..38e4206687b 100644
--- a/tests/gold_tests/chunked_encoding/bad_chunked_encoding.test.py
+++ b/tests/gold_tests/chunked_encoding/bad_chunked_encoding.test.py
@@ -25,7 +25,7 @@
 Test.ContinueOnFail = True
 
 # Define default ATS
-ts = Test.MakeATSProcess("ts", select_ports=True, enable_tls=False)
+ts = Test.MakeATSProcess("ts1", select_ports=True, enable_tls=False)
 server = Test.MakeOriginServer("server")
 
 testName = ""
@@ -53,7 +53,7 @@
     ts.Variables.port)
 tr.Processes.Default.ReturnCode = 0
 tr.Processes.Default.StartBefore(server)
-tr.Processes.Default.StartBefore(Test.Processes.ts)
+tr.Processes.Default.StartBefore(ts)
 tr.Processes.Default.Streams.All = Testers.ContainsExpression("501 Field not implemented", "Should fail")
 tr.Processes.Default.Streams.All = Testers.ExcludesExpression("200 OK", "Should not succeed")
 tr.StillRunningAfter = server
@@ -69,3 +69,85 @@
 tr.Processes.Default.Streams.All = Testers.ExcludesExpression("200 OK", "Should not succeed")
 tr.StillRunningAfter = server
 tr.StillRunningAfter = ts
+
+
+class MalformedChunkHeaderTest:
+    chunkedReplayFile = "replays/malformed_chunked_header.replay.yaml"
+
+    def __init__(self):
+        self.setupOriginServer()
+        self.setupTS()
+
+    def setupOriginServer(self):
+        self.server = Test.MakeVerifierServerProcess("verifier-server", self.chunkedReplayFile)
+
+        # The server's responses will fail the first two transactions
+        # because ATS will close the connection due to the malformed
+        # chunk headers.
+        self.server.Streams.stdout += Testers.ContainsExpression(
+            "Header write for key 1 failed",
+            "Verify that writing the first response failed.")
+        self.server.Streams.stdout += Testers.ContainsExpression(
+            "Header write for key 2 failed",
+            "Verify that writing the second response failed.")
+
+        # ATS should close the connection before any body gets through.
+        # "abc" is the body sent for each of these chunked cases.
+        self.server.Streams.stdout += Testers.ExcludesExpression(
+            "abc",
+            "Verify that the body never got through.")
+
+    def setupTS(self):
+        self.ts = Test.MakeATSProcess("ts2", enable_tls=True, enable_cache=False)
+        self.ts.addDefaultSSLFiles()
+        self.ts.Disk.records_config.update({
+            "proxy.config.diags.debug.enabled": 1,
+            "proxy.config.diags.debug.tags": "http",
+            "proxy.config.ssl.server.cert.path": f'{self.ts.Variables.SSLDir}',
+            "proxy.config.ssl.server.private_key.path": f'{self.ts.Variables.SSLDir}',
+            "proxy.config.ssl.client.verify.server.policy": 'PERMISSIVE',
+        })
+        self.ts.Disk.ssl_multicert_config.AddLine(
+            'dest_ip=* ssl_cert_name=server.pem ssl_key_name=server.key'
+        )
+        self.ts.Disk.remap_config.AddLine(
+            f"map / http://127.0.0.1:{self.server.Variables.http_port}/",
+        )
+        self.ts.Streams.stderr += Testers.ContainsExpression(
+            "user agent post chunk decoding error",
+            "Verify that ATS detected a problem parsing a chunk.")
+
+    def runChunkedTraffic(self):
+        tr = Test.AddTestRun()
+        tr.AddVerifierClientProcess(
+            "client",
+            self.chunkedReplayFile,
+            http_ports=[self.ts.Variables.port],
+            https_ports=[self.ts.Variables.ssl_port],
+            other_args='--thread-limit 1')
+        tr.Processes.Default.StartBefore(self.server)
+        tr.Processes.Default.StartBefore(self.ts)
+        tr.StillRunningAfter = self.server
+        tr.StillRunningAfter = self.ts
+
+        # The aborted connections will result in errors and a non-zero return
+        # code from the verifier client.
+        tr.Processes.Default.ReturnCode = 1
+        tr.Processes.Default.Streams.stdout += Testers.ContainsExpression(
+            "Failed HTTP/1 transaction with key=3",
+            "Verify that ATS closed the third transaction.")
+        tr.Processes.Default.Streams.stdout += Testers.ContainsExpression(
+            "Failed HTTP/1 transaction with key=4",
+            "Verify that ATS closed the fourth transaction.")
+
+        # ATS should close the connection before any body gets through.
+        # "abc" is the body sent for each of these chunked cases.
+        tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression(
+            "abc",
+            "Verify that the body never got through.")
+
+    def run(self):
+        self.runChunkedTraffic()
+
+
+MalformedChunkHeaderTest().run()
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
new file mode 100644
index 00000000000..c6091ab4ee1
--- /dev/null
+++ b/tests/gold_tests/chunked_encoding/replays/malformed_chunked_header.replay.yaml
@@ -0,0 +1,109 @@
+#  Licensed to the Apache Software Foundation (ASF) under one
+#  or more contributor license agreements.  See the NOTICE file
+#  distributed with this work for additional information
+#  regarding copyright ownership.  The ASF licenses this file
+#  to you under the Apache License, Version 2.0 (the
+#  "License"); you may not use this file except in compliance
+#  with the License.  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+meta:
+  version: "1.0"
+
+sessions:
+- transactions:
+  - client-request:
+      method: "POST"
+      version: "1.1"
+      url: /malformed/chunk/header
+      headers:
+        fields:
+        - [ Host, example.com ]
+        - [ Transfer-Encoding, chunked ]
+        - [ uuid, 1 ]
+      content:
+        transfer: plain
+        encoding: uri
+        # Chunk header sizes are in hex, so a size of `z` is malformed.
+        data: z%0D%0Aabc%0D%0A0%0D%0A%0D%0A
+
+    # The connection will be dropped and this response will not go out.
+    server-response:
+      status: 200
+
+- transactions:
+  - client-request:
+      method: "POST"
+      version: "1.1"
+      url: /large/chunk/size/header
+      headers:
+        fields:
+        - [ Host, example.com ]
+        - [ Transfer-Encoding, chunked ]
+        - [ uuid, 2 ]
+      content:
+        transfer: plain
+        encoding: uri
+        # Super large chunk header, larger than will fit in an int.
+        data: 111111113%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.
+  #
+- transactions:
+  - client-request:
+      method: "GET"
+      version: "1.1"
+      url: /response/malformed/chunk/size
+      headers:
+        fields:
+        - [ Host, example.com ]
+        - [ uuid, 3 ]
+
+    # 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
+        # Chunk header sizes are in hex, so a size of `z` is malformed.
+        data: z%0D%0Aabc%0D%0A0%0D%0A%0D%0A
+
+- transactions:
+  - client-request:
+      method: "GET"
+      version: "1.1"
+      url: /response/large/chunk/size
+      headers:
+        fields:
+        - [ Host, example.com ]
+        - [ uuid, 4 ]
+
+    # 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
+        # Super large chunk header, larger than will fit in an int.
+        data: 111111113%0D%0Aabc%0D%0A0%0D%0A%0D%0A
