This is an automated email from the ASF dual-hosted git repository.
bneradt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 83c89f3 Detect and handle chunk header size truncation (#8452)
83c89f3 is described below
commit 83c89f3d217d473ecb000b68c910c0f183c3a355
Author: Brian Neradt <[email protected]>
AuthorDate: Wed Oct 27 11:30:54 2021 -0500
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 +++++++++++++++++++++
.../chunked_encoding/bad_chunked_encoding.test.py | 86 ++++++++++++-
.../replays/malformed_chunked_header.replay.yaml | 109 ++++++++++++++++
6 files changed, 364 insertions(+), 3 deletions(-)
diff --git a/include/tscore/ink_memory.h b/include/tscore/ink_memory.h
index fdaaa27..7fd8de1 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 08837c6..564adc2 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 43750b2..c0ca76c 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 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();
+}
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 cdfc0bf..38e4206 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 unsupported values for chunked_encoding
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 @@ tr.Processes.Default.Command = 'curl -H "host: example.com"
-H "transfer-encodin
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.ContainsExpression("501 Field not imp
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 0000000..c6091ab
--- /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
