Detect and handle chunk header size truncation

Origin: upstream
Applied-Upstream: https://github.com/apache/trafficserver/commit/2addc8ca71449ceac0d5b80172460ee09c938f5e
Reviewed-by: Jean Baptiste Favre <debian@jbfavre.org>
Last-Update: 2022-05-21

This detects if a chunk header size is too large and, if so, closes the
connection.
Last-Update: 2022-05-21
Gbp-Pq: Name 0019-CVE_2021_37149.patch

diff --git a/include/tscore/ink_memory.h b/include/tscore/ink_memory.h
index cf850c0b9be5ff0eb1127986221ce098ee610bb3..d30452263ce4805bd6b7fb885a0668d2b9896f44 100644 (file)
--- 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>
 
@@ -204,6 +205,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 48aebb8d6835e06daed7a8804d4a8c3e4715ecd4..a5ab2910a7852560ebb8ca7e2d91d207580b752a 100644 (file)
--- 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";
@@ -153,8 +154,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 a82aec2429121577e0cca01d0de3efb9ccaa6bbb..f81d6ef1f8de92f4a4623463a59539f16b3118cc 100644 (file)
--- a/src/tscore/Makefile.am
+++ b/src/tscore/Makefile.am
@@ -258,6 +258,7 @@ test_tscore_SOURCES = \
        unit_tests/test_BufferWriter.cc \
        unit_tests/test_BufferWriterFormat.cc \
        unit_tests/test_ink_inet.cc \
+       unit_tests/test_ink_memory.cc \
        unit_tests/test_IntrusivePtr.cc \
        unit_tests/test_IpMap.cc \
        unit_tests/test_layout.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 (file)
index 0000000..fa6725b
--- /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();
+}