http: fix allocation bug introduced in #4211. by htuch · Pull Request #4245 · envoyproxy/envoy

htuch · 2018-08-24T15:32:13Z

There were some non-local invariants that header_map_impl_fuzz_test surfaced around minimum dynamic
buffer size. This PR improves comments and documentation of invariants and fixes the allocation issue
to maintain them.

Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10038.

Risk level: Low. It's recommended to bump to this for potential security reasons if you are already post
#4211.
Testing: Unit test and corpus entry added.

Signed-off-by: Harvey Tuch htuch@google.com

There were some non-local invariants that header_map_impl_fuzz_test surfaced around minimum dynamic buffer size. This PR improves comments and documentation of invariants and fixes the allocation issue to maintain them. Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10038. Risk level: Low. It's recommended to bump to this for potential security reasons if you are already post envoyproxy#4211. Testing: Unit test and corpus entry added. Signed-off-by: Harvey Tuch <htuch@google.com>

ggreenway · 2018-08-24T16:25:18Z

@htuch from ipv6 tests:

==================== Test output for //test/integration:ads_integration_test:
WARNING: Perftools heap leak checker is active -- Performance may suffer
[==========] Running 40 tests from 3 test cases.
[----------] Global test environment set-up.
[----------] 32 tests from IpVersionsClientType/AdsIntegrationTest
[ RUN      ] IpVersionsClientType/AdsIntegrationTest.Basic/0
[2018-08-24 15:59:54.943][4912][critical][assert] source/common/http/header_map_impl.cc:199] assert failure: dynamic_capacity_ >= MaxIntegerLength.

dnoe · 2018-08-24T17:04:41Z

source/common/http/header_map_impl.cc

    type_ = Type::Dynamic;
-    dynamic_capacity_ = (string_length_ + size) * 2;
+    dynamic_capacity_ =
+        std::max(MinDynamicCapacity, static_cast<size_t>((string_length_ + size) * 2));


You use size_t here, but below the same calculation below uses static_cast<uint64_t>. They should be the same, but it might be good to be consistent here, probably standardizing on size_t.

dnoe · 2018-08-24T17:10:32Z

source/common/http/header_map_impl.cc

    // Whether dynamic or inline the buffer is guaranteed to be large enough.
+    ASSERT(dynamic_capacity_ >= MaxIntegerLength);
+    // It's safe to use buffer.dynamic_, since buffer.ref_ is union aliased.
+    ASSERT(&buffer_.dynamic_ == &buffer_.ref_);


When would this ever not be true? Seems like we are testing the compiler here, but maybe I'm missing something.

If you made a change to the code to change a union to struct, for example :)

It would be nice if that could be a static_assert, then. Obviously not possible in its current form. Can you take advantage of something like typeid ?

dnoe · 2018-08-24T17:12:13Z

source/common/http/header_map_impl.cc

+    ASSERT(buffer_.dynamic_ == inline_buffer_);
  case Type::Dynamic: {
    // Whether dynamic or inline the buffer is guaranteed to be large enough.
+    ASSERT(dynamic_capacity_ >= MaxIntegerLength);


I am wondering if both of the above ASSERTs should be RELEASE_ASSERTs given the potential impact.

I think the min length ones are all internal invariants and can be validated via review. I think we should trigger them in a relatively obvious way if changes are made in the future, but I might be wrong.

dnoe · 2018-08-24T17:14:40Z

test/common/http/header_map_impl_test.cc

+  {
+    const std::string empty;
+    HeaderString value4(empty);
+    HeaderMapImpl::appendToHeader(value4, " ");


This doesn't match the comment, since " " isn't an empty string.

True; the original fuzz case looked like an empty string, but I'm thinking it might have been strange Unicode.

Oh, that's terrifying.

Signed-off-by: Harvey Tuch <htuch@google.com>

dnoe · 2018-08-24T18:27:28Z

source/common/http/header_map_impl.cc

    // We can get here either because we didn't fit in inline or we are already dynamic.
    if (type_ == Type::Inline) {
      const uint64_t new_capacity = (static_cast<uint64_t>(string_length_) + size) * 2;
-      // If the resizing will cause buffer overflow due to hitting uint32_t::max, an OOM is likely


Do you want to change new_capacity above this to be size_t? Other than that, LGTM.

Signed-off-by: Harvey Tuch <htuch@google.com>

ggreenway · 2018-08-24T18:49:32Z

source/common/http/header_map_impl.cc

-      // If the resizing will cause buffer overflow due to hitting uint32_t::max, an OOM is likely
-      // imminent. Fast-fail rather than allow a buffer overflow attack (issue #1421)
-      RELEASE_ASSERT(new_capacity <= std::numeric_limits<uint32_t>::max(), "");
+      const size_t new_capacity = (static_cast<uint64_t>(string_length_) + size) * 2;


cast to uint64_t, then assign to size_t; this seems wrong

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2018-08-24T20:16:24Z

@dnoe @ggreenway can we merge? Thanks.

alyssawilk · 2018-08-27T19:15:59Z

source/common/http/header_map_impl.cc

-      // If the resizing will cause buffer overflow due to hitting uint32_t::max, an OOM is likely
-      // imminent. Fast-fail rather than allow a buffer overflow attack (issue #1421)
-      RELEASE_ASSERT(new_capacity <= std::numeric_limits<uint32_t>::max(), "");
+      const size_t new_capacity = (string_length_ + size) * 2;


Wow, thanks for doing the validation - sorry I missed this in #4211

Unfortunately I don't think this size_t does what we want it to do - I think we need to static_cast string_length to size_t to not do this as a unit32_t. Also on 122 we're definitely setting to a unit32_t before checking (which is totally a preexisting bug but as long as you're making our code safer I get to suggest we make it safer-er =P). Honestly I can't do this stuff by eye either, so I cheated and wrote some death test to sanity check things and validate our validation :-)

Pulling the following changes from github.com/envoyproxy/envoy: f936fc6 ssl: serialize accesses to SSL socket factory contexts (envoyproxy#4345) e34dcd6 Fix crash in tcp_proxy (envoyproxy#4323) ae6a252 router: fix matching when all domains have wildcards (envoyproxy#4326) aa06142 test: Stop fake_upstream methods from accidentally succeeding (envoyproxy#4232) 5d73187 rbac: update the authenticated.user to a StringMatcher. (envoyproxy#4250) c6bfc7d time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers (envoyproxy#4257) 752483e Fixing the fix (envoyproxy#4333) 83487f6 tls: update BoringSSL to ab36a84b (3497). (envoyproxy#4338) 7bc210e test: fixing interactions between waitFor and ignore_spurious_events (envoyproxy#4309) 69474b3 admin: order stats in clusters json admin (envoyproxy#4306) 2d155f9 ppc64le build (envoyproxy#4183) 07efc6d fix static initialization fiasco problem (envoyproxy#4314) 0b7e3b5 test: Remove declared but undefined class methods (envoyproxy#4297) 1485a13 lua: make sure resetting dynamic metadata wrapper when request info is marked dead d243cd6 test: set to zero when start_time exceeds limit (envoyproxy#4328) 0a1e92a test: fix heap use-after-free in ~IntegrationTestServer. (envoyproxy#4319) cddc732 CONTRIBUTING: Document 'kick-ci' trick. (envoyproxy#4335) f13ef24 docs: remove reference to deprecated value field (envoyproxy#4322) e947a27 router: minor doc fixes in stream idle timeout (envoyproxy#4329) 0c2e998 tcp-proxy: fixing a TCP proxy bug where we attempted to readDisable a closed connection (envoyproxy#4296) 00ffe44 utility: fix strftime overflow handling. (envoyproxy#4321) af1183c Re-enable TcpProxySslIntegrationTest and make the tests pass again. (envoyproxy#4318) 3553461 fuzz: fix H2 codec fuzzer post envoyproxy#4262. (envoyproxy#4311) 42f6048 Proto string issue fix (envoyproxy#4320) 9c492a0 Support Envoy to fetch secrets using SDS service. (envoyproxy#4256) a857219 ratelimit: revert `revert rate limit failure mode config` and add tests (envoyproxy#4303) 1d34172 dns: fix exception unsafe behavior in c-ares callbacks. (envoyproxy#4307) 1212423 alts: add gRPC TSI socket (envoyproxy#4153) f0363ae fuzz: detect client-side resets in H2 codec fuzzer. (envoyproxy#4300) 01aa3f8 test: hopefully deflaking echo integration test (envoyproxy#4304) 1fc0f4b ratelimit: link legacy proto when message is being used (envoyproxy#4308) aa4481e fix rare List::remove(&target) segfault (envoyproxy#4244) 89e0f23 headers: fixing fast fail of size-validation (envoyproxy#4269) 97eba59 build: bump googletest version. (envoyproxy#4293) 0057e22 fuzz: avoid false positives in HCM fuzzer. (envoyproxy#4262) 9d094e5 Revert ac0bd74 (envoyproxy#4295) ddb28a4 Add validation context provider (envoyproxy#4264) 3b47cba added histogram latency information to Hystrix dashboard stream (envoyproxy#3986) cf87d50 docs: update SNI FAQ. (envoyproxy#4285) f952033 config: fix update empty stat for eds (envoyproxy#4276) 329e591 router: Add ability of custom headers to rely on per-request data (envoyproxy#4219) 68d20b4 thrift: refactor build files and imports (envoyproxy#4271) 5fa8192 access_log: log requested_server_name in tcp proxy (envoyproxy#4144) fa45bb4 fuzz: libc++ clocks don't like nanos. (envoyproxy#4282) 53f8944 stats: add symbol table for future stat name encoding (envoyproxy#3927) c987b42 test infra: Remove timeSource() from the ClusterManager api (envoyproxy#4247) cd171d9 websocket: tunneling websockets (and upgrades in general) over H2 (envoyproxy#4188) b9dc5d9 router: disallow :path/host rewriting in request_headers_to_add. (envoyproxy#4220) 0c91011 network: skip socket options and source address for UDS client connections (envoyproxy#4252) da1857d build: fixing a downstream compile error by noting explicit fallthrough (envoyproxy#4265) 9857cfe fuzz: cleanup per-test environment after each fuzz case. (envoyproxy#4253) 52beb06 test: Wrap proto string in std::string before comparison (envoyproxy#4238) f5e219e extensions/thrift_proxy: Add header matching to thrift router (envoyproxy#4239) c9ce5d2 fuzz: track read_disable_count bidirectionally in codec_impl_fuzz_test. (envoyproxy#4260) 35103b3 fuzz: use nanoseconds for SystemTime in RequestInfo. (envoyproxy#4255) ba6ba98 fuzz: make runtime root hermetic in server_fuzz_test. (envoyproxy#4258) b0a9014 time: Add 'format' test to ensure no one directly instantiates Prod*Time from source. (envoyproxy#4248) 8567460 access_log: support beginning of epoch in START_TIME. (envoyproxy#4254) 28d5f41 proto: unify envoy_proto_library/api_proto_library. (envoyproxy#4233) f7d3cb6 http: fix allocation bug introduced in envoyproxy#4211. (envoyproxy#4245) Fixes istio/istio#8310 (once pulled into istio/istio). Signed-off-by: Piotr Sikora <piotrsikora@google.com>

htuch requested review from dnoe and ggreenway August 24, 2018 15:32

htuch assigned dnoe and ggreenway Aug 24, 2018

dnoe reviewed Aug 24, 2018

View reviewed changes

htuch added 2 commits August 24, 2018 13:22

Fix ASSERT fails in tests.

28d232d

Signed-off-by: Harvey Tuch <htuch@google.com>

Review feedback and more belt-and-braces.

7c7c122

Signed-off-by: Harvey Tuch <htuch@google.com>

dnoe reviewed Aug 24, 2018

View reviewed changes

dnoe previously approved these changes Aug 24, 2018

View reviewed changes

htuch added 2 commits August 24, 2018 14:28

static_assert for union alias.

36af343

Signed-off-by: Harvey Tuch <htuch@google.com>

uint64_t -> size_t.

8e12de5

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch dismissed dnoe’s stale review via 8e12de5 August 24, 2018 18:29

ggreenway reviewed Aug 24, 2018

View reviewed changes

Fewer type gymnastics.

cd531fa

Signed-off-by: Harvey Tuch <htuch@google.com>

ggreenway approved these changes Aug 24, 2018

View reviewed changes

htuch merged commit f7d3cb6 into envoyproxy:master Aug 24, 2018

htuch deleted the header-map-itoa-fix branch August 24, 2018 20:33

htuch mentioned this pull request Aug 27, 2018

Use ClusterFuzz corpus in CI #4263

Open

alyssawilk mentioned this pull request Aug 27, 2018

build: fixing a downstream compile error by noting explicit fallthrough #4265

Merged

alyssawilk reviewed Aug 27, 2018

View reviewed changes

alyssawilk mentioned this pull request Aug 27, 2018

unvalidated user defined headers dangerous #4268

Closed

Conversation

htuch commented Aug 24, 2018

Uh oh!

ggreenway commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants