You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by bn...@apache.org on 2021/10/27 16:30:16 UTC
[trafficserver] branch 8.1.x updated: Detect and handle chunk
header size truncation (#8458)
This is an automated email from the ASF dual-hosted git repository.
bneradt pushed a commit to branch 8.1.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/8.1.x by this push:
new 2addc8c Detect and handle chunk header size truncation (#8458)
2addc8c is described below
commit 2addc8ca71449ceac0d5b80172460ee09c938f5e
Author: Brian Neradt <br...@gmail.com>
AuthorDate: Wed Oct 27 11:30:07 2021 -0500
Detect and handle chunk header size truncation (#8458)
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 +++++++++++++++++++++++++++++++
4 files changed, 171 insertions(+), 1 deletion(-)
diff --git a/include/tscore/ink_memory.h b/include/tscore/ink_memory.h
index cf850c0..d304522 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>
@@ -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 1fc50ab..94e0044 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";
@@ -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 a82aec2..f81d6ef 100644
--- 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
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();
+}