From 83c89f3d217d473ecb000b68c910c0f183c3a355 Mon Sep 17 00:00:00 2001 From: Brian Neradt 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 #include #include +#include #include #include @@ -205,6 +206,24 @@ ink_zero(T &t) memset(static_cast(&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 +inline constexpr bool +can_safely_shift_left(T value, int num_places) +{ + constexpr auto max_value = std::numeric_limits::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 +#include +#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