Fix overflow with optimized WASM code by ensuring that globals are in… by jplevyak · Pull Request #25 · istio/envoy

jplevyak · 2019-02-20T22:15:31Z

…itialized.

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

…itialized.

istio-testing · 2019-02-20T22:15:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers:

If they are not already assigned, you can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

PiotrSikora

Sweet, thanks!

lambdai · 2019-02-20T22:36:11Z

test/extensions/filters/http/wasm/test_data/Makefile

-# with getFunctionWavm() templates and/or their usage.
-#CXXFLAGS=--std=c++14 -O3 -g3
-CXXFLAGS=--std=c++14 -g3
+CXXFLAGS=--std=c++14 -O3 -g3


-g3: TIL. Thanks!

lambdai · 2019-02-20T22:56:55Z

source/extensions/common/wasm/wavm/wavm.cc

  auto f = getStartFunction(moduleInstance_);
-  if (!f)
+  if (f) {
+    CALL_WITH_CONTEXT(invokeFunctionChecked(context_, f, {}), context);


Is there a early return? It's possible that f is reassigned and invoked at line 287

So, we want to call the generic start function before the global variables are initialized. I am not sure why, but that is what WAVM does and this is WAVM specific code. The "start" function is special and according to the docs should be called (if available) before any exported functions.

Thank you for the explanation!

lambdai · 2019-02-20T22:59:00Z

source/extensions/common/wasm/wasm.cc

 // Calls into the WASM code.
 //
 void Context::onStart() {
+  wasm_->wasmVm()->start(this);


Any test to cover the behavior?

ensuring that globals are initialized. I get it. Thanks!

This adds -O3 to a WASM file for a test that used to cause a crash with -O3, so that is the test.

PiotrSikora · 2019-02-20T23:04:04Z

source/extensions/common/wasm/wavm/wavm.cc

+  if (emscriptenInstance_) {
+    Emscripten::initializeGlobals(context_, irModule_, moduleInstance_);
+  }
+  f = asFunctionNullable(getInstanceExport(moduleInstance_, "main"));


Actually, on a second thought, what if main() is exported as the start function? Could we guard against that and not call it twice?

AFAICT the "start" function is special and according to the docs should be called (if available) before any exported functions e.g. "main" or "_main". In this case it is also called before the Emscripten module globals are initialized because that is what wavm-run does and this is WAVM specific code.

Yeah, my point was that main() function might be marked as the "start" function (at the WASM level), in which case we'd call it twice... but maybe it wouldn't be exported then, though.

Anyway, we can deal with it if that becomes an issue with other languages.

jplevyak · 2019-02-21T00:43:18Z

Can I get an /lgtm when you guys are good?

PiotrSikora · 2019-02-21T00:55:33Z

/lgtm (I already approved it using GitHub's review changes, btw)

lambdai · 2019-02-21T00:58:10Z

/lgtm

istio-testing · 2019-02-21T00:58:15Z

@silentdai: changing LGTM is restricted to assignees, and assigning you to the PR failed.

Details

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lambdai · 2019-02-21T01:25:18Z

@silentdai: changing LGTM is restricted to assignees, and assigning you to the PR failed.

CC @jplevyak @PiotrSikora I am blamed :(

PiotrSikora · 2019-02-23T08:50:45Z

source/extensions/common/wasm/wavm/wavm.cc

@@ -276,8 +276,16 @@ bool Wavm::initialize(const std::string& code, absl::string_view name, bool allo

 void Wavm::start(Context* context) {


@jplevyak unless I'm mistaken, this is never called in program, only in tests, which means that we neither initialize globals, nor call the start function (or main()), and starting program with optimized WASM module leads to a crash.

I think we've lost the call to start() during one of the refactors. Could you look into this? Thanks!

Fixes CVE-2019-9516. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

This test sends a large number of metadata frames in order to trigger a disconnect. However, it was possible for the disconnect to happen and the connection to be torn down before all the metadata frames had been sent. If that happened, ASAN detected a UAF: ``` ==95==ERROR: AddressSanitizer: heap-use-after-free on address 0x60700037e5a0 at pc 0x000004811f9e bp 0x7ffc903af990 sp 0x7ffc903af988 READ of size 8 at 0x60700037e5a0 thread T0 #0 0x4811f9d in Envoy::IntegrationCodecClient::sendMetadata(Envoy::Http::RequestEncoder&, Envoy::Http::MetadataMap) /proc/self/cwd/test/integration/http_integration.cc:168:3 #1 0x46ed711 in Envoy::Http2FloodMitigationTest_RequestMetadata_Test::TestBody() /proc/self/cwd/test/integration/http2_flood_integration_test.cc:1486:20 #2 0xd380e64 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2580:10 #3 0xd348dc2 in testing::Test::Run() /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2655:5 #4 0xd34a927 in testing::TestInfo::Run() /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2832:11 #5 0xd34ccc4 in testing::TestSuite::Run() /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2986:28 #6 0xd36f07a in testing::internal::UnitTestImpl::RunAllTests() /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:5697:44 #7 0xd384e63 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /proc/self/cwd/external/com_google_googletest/googletest/src/ gtest.cc:2580:10 #8 0xd36dd86 in testing::UnitTest::Run() /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:5280:10 #9 0xa0e53a4 in Envoy::TestRunner::RunTests(int, char**) /proc/self/cwd/external/com_google_googletest/googletest/include/gtest/gtest.h:2485:46 #10 0xa0e0af7 in main /proc/self/cwd/test/main.cc:34:10 #11 0x7f442ef69082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee) #12 0x45ed36d in _start (/mnt/ssd/cas/work/1/exec/bazel-out/k8-dbg/bin/test/integration/http2_flood_integration_test.runfiles/envoy/test/integration/http2_flood_integration_test+0x45ed36d) 0x60700037e5a0 is located 48 bytes inside of 80-byte region [0x60700037e570,0x60700037e5c0) freed by thread T0 here: #0 0x466f7d2 in free /local/mnt/workspace/bcain_clang_hu-bcain-lv_22036/final/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:52:3 #1 0x831dde8 in Envoy::Http::CodecClient::ActiveRequest::~ActiveRequest() /proc/self/cwd/./source/common/http/codec_client.h:220:10 #2 0x5aa33f9 in std::__1::unique_ptr<Envoy::Event::DeferredDeletable, std::__1::default_delete<Envoy::Event::DeferredDeletable> >::reset(Envoy::Event::DeferredDeletable*) /opt/llvm/bin/../include/c++/v1/__memory/unique_ptr.h:54:5 #3 0xa3218e8 in Envoy::Event::DispatcherImpl::clearDeferredDeleteList() /proc/self/cwd/source/common/event/dispatcher_impl.cc:142:21 #4 0xa3348df in void std::__1::__invoke_void_return_wrapper<void, true>::__call<Envoy::Event::DispatcherImpl::DispatcherImpl(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, Envoy::Thread::ThreadFactory&, Envoy::TimeSource& , Envoy::Random::RandomGenerator&, Envoy::Filesystem::Instance&, Envoy::Event::TimeSystem&, std::__1::function<std::__1::unique_ptr<Envoy::Event::ScaledRangeTimerManager, std::__1::default_delete<Envoy::Event::ScaledRangeTimerManager> > (Envoy::Event::Dispatcher&)> const& , std::__1::shared_ptr<Envoy::Buffer::WatermarkFactory> const&)::$_2&>(Envoy::Event::DispatcherImpl::DispatcherImpl(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, Envoy::Thread::ThreadFactory&, Envoy::TimeSource&, Envoy::Rand om::RandomGenerator&, Envoy::Filesystem::Instance&, Envoy::Event::TimeSystem&, std::__1::function<std::__1::unique_ptr<Envoy::Event::ScaledRangeTimerManager, std::__1::default_delete<Envoy::Event::ScaledRangeTimerManager> > (Envoy::Event::Dispatcher&)> const&, std::__1::s hared_ptr<Envoy::Buffer::WatermarkFactory> const&)::$_2&) /proc/self/cwd/source/common/event/dispatcher_impl.cc:79:30 #5 0xa334603 in std::__1::__function::__func<Envoy::Event::DispatcherImpl::DispatcherImpl(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, Envoy::Thread::ThreadFactory&, Envoy::TimeSource&, Envoy::Random::RandomGenerator&, Envoy::Filesystem::Instance&, Envoy::Event::TimeSystem&, std::__1::function<std::__1::unique_ptr<Envoy::Event::ScaledRangeTimerManager, std::__1::default_delete<Envoy::Event::ScaledRangeTimerManager> > (Envoy::Event::Dispatcher&)> const&, std::__1::shared_ptr<Envoy::Buffe r::WatermarkFactory> const&)::$_2, std::__1::allocator<Envoy::Event::DispatcherImpl::DispatcherImpl(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, Envoy::Thread::ThreadFactory&, Envoy::TimeSource&, Envoy::Random::RandomGenera tor&, Envoy::Filesystem::Instance&, Envoy::Event::TimeSystem&, std::__1::function<std::__1::unique_ptr<Envoy::Event::ScaledRangeTimerManager, std::__1::default_delete<Envoy::Event::ScaledRangeTimerManager> > (Envoy::Event::Dispatcher&)> const&, std::__1::shared_ptr<Envoy: :Buffer::WatermarkFactory> const&)::$_2>, void ()>::operator()() /opt/llvm/bin/../include/c++/v1/__functional/function.h:180:16 #6 0x4897039 in std::__1::__function::__value_func<void ()>::operator()() const /opt/llvm/bin/../include/c++/v1/__functional/function.h:507:16 #7 0xa8e6aa4 in Envoy::Event::SchedulableCallbackImpl::SchedulableCallbackImpl(Envoy::CSmartPtr<event_base, &(event_base_free)>&, std::__1::function<void ()>)::$_0::__invoke(int, short, void*) /opt/llvm/bin/../include/c++/v1/__functional/function.h:1184:12 #8 0xb557c5e in event_process_active_single_queue /mnt/ssd/cas/work/2/exec/external/com_github_libevent_libevent/event.c:1713:4 #9 0xb539252 in event_process_active /mnt/ssd/cas/work/2/exec/external/com_github_libevent_libevent/event.c #10 0xb539252 in event_base_loop /mnt/ssd/cas/work/2/exec/external/com_github_libevent_libevent/event.c:2047:12 #11 0xa8e1e3c in Envoy::Event::LibeventScheduler::run(Envoy::Event::Dispatcher::RunType) /proc/self/cwd/source/common/event/libevent_scheduler.cc:60:3 #12 0xa32bd94 in Envoy::Event::DispatcherImpl::run(Envoy::Event::Dispatcher::RunType) /proc/self/cwd/source/common/event/dispatcher_impl.cc:299:19 #13 0x480faad in Envoy::IntegrationCodecClient::flushWrite() /proc/self/cwd/test/integration/http_integration.cc:100:29 #14 0x4811e94 in Envoy::IntegrationCodecClient::sendMetadata(Envoy::Http::RequestEncoder&, Envoy::Http::MetadataMap) /proc/self/cwd/test/integration/http_integration.cc:169:3 #15 0x46ed711 in Envoy::Http2FloodMitigationTest_RequestMetadata_Test::TestBody() /proc/self/cwd/test/integration/http2_flood_integration_test.cc:1486:20 #16 0xd380e64 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2580:10 #17 0xd348dc2 in testing::Test::Run() /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2655:5 #18 0xd34a927 in testing::TestInfo::Run() /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2832:11 #19 0xd34ccc4 in testing::TestSuite::Run() /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2986:28 #20 0xd36f07a in testing::internal::UnitTestImpl::RunAllTests() /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:5697:44 #21 0xd384e63 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2580:10 #22 0xd36dd86 in testing::UnitTest::Run() /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:5280:10 #23 0xa0e53a4 in Envoy::TestRunner::RunTests(int, char**) /proc/self/cwd/external/com_google_googletest/googletest/include/gtest/gtest.h:2485:46 #24 0xa0e0af7 in main /proc/self/cwd/test/main.cc:34:10 #25 0x7f442ef69082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee) ``` To fix that, write all metadata frames at once. Signed-off-by: Benjamin Peterson <benjamin@engflow.com>

Fix overflow with optimized WASM code by ensuring that globals are in…

e137d66

…itialized.

jplevyak requested a review from PiotrSikora February 20, 2019 22:15

PiotrSikora approved these changes Feb 20, 2019

View reviewed changes

lambdai reviewed Feb 20, 2019

View reviewed changes

PiotrSikora reviewed Feb 20, 2019

View reviewed changes

jplevyak merged commit d3a53ef into istio:wasm Feb 21, 2019

jplevyak deleted the wasm-opt-fix branch February 21, 2019 01:45

PiotrSikora reviewed Feb 23, 2019

View reviewed changes

jplevyak pushed a commit to jplevyak/envoy that referenced this pull request Aug 13, 2019

http2: enable strict validation of HTTP/2 headers. (istio#25)

84dabbf

Fixes CVE-2019-9516. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

		@@ -276,8 +276,16 @@ bool Wavm::initialize(const std::string& code, absl::string_view name, bool allo

		void Wavm::start(Context* context) {

Conversation

jplevyak commented Feb 20, 2019

Uh oh!

istio-testing commented Feb 20, 2019

Uh oh!

PiotrSikora left a comment

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!

jplevyak commented Feb 21, 2019

Uh oh!

PiotrSikora commented Feb 21, 2019

Uh oh!

lambdai commented Feb 21, 2019

Uh oh!

istio-testing commented Feb 21, 2019

Uh oh!

lambdai commented Feb 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants