CVE-2020-10739: NULL Pointer Dereference (CWE-476)
value: Istio 1.4.x before 1.4.9 and Istio 1.5.x before 1.5.4 contain the following vulnerability when telemetry v2 is enabled: by sending a specially crafted packet, an attacker could trigger a Null Pointer Exception resulting in a Denial of Service. This could be sent to the ingress gateway or a sidecar, triggering a null pointer exception which results in a denial of service. This also affects servicemesh-proxy where a null pointer exception flaw was found in servicemesh-proxy. When running Telemetry v2 (not on by default in version 1.4.x), an attacker could send a specially crafted packet to the ingress gateway or proxy sidecar, triggering a denial of service.
CWE description: The product dereferences a pointer that it expects to be valid but is NULL.
CVSS v3 base score: 7.5
Published: 2020-06-02T13:15Z
Repository: https://github.com/istio/envoy
Language: C++
Files changed: source/extensions/common/wasm/wasm.cc, source/extensions/common/wasm/wasm.h, test/extensions/wasm/wasm_test.cc
Commit date: 2020-05-08 19:05:05 -0700
Commit message: 1.4 - Do not call into the VM unless the VM Context has been created. (#24)

* Ensure that the in VM Context is created before onDone is called.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

* Update as per offline discussion.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

* Set in_vm_context_created_ in onNetworkNewConnection.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

* Add guards to other network calls.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

* Fix common/wasm tests.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

* Patch tests.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

* Remove unecessary file from cherry-pick.

Signed-off-by: John Plevyak <jplevyak@gmail.com>
Diff: diff --git a/source/extensions/common/wasm/wasm.cc b/source/extensions/common/wasm/wasm.cc
index cb98612f8..5e71dd819 100644
--- a/source/extensions/common/wasm/wasm.cc
+++ b/source/extensions/common/wasm/wasm.cc
@@ -1697,10 +1697,11 @@ void Context::onStart(absl::string_view root_id, absl::string_view vm_configurat
   if (wasm_->onStart_) {
     auto root_id_addr = wasm_->copyString(root_id);
     auto config_addr = wasm_->copyString(vm_configuration);
     wasm_->onStart_(this, id_, root_id_addr, root_id.size(), config_addr, vm_configuration.size());
   }
+  in_vm_context_created_ = true;
 }
 
 bool Context::validateConfiguration(absl::string_view configuration) {
   if (!wasm_->validateConfiguration_) {
     return true;
@@ -1723,47 +1724,48 @@ void Context::onCreate(uint32_t root_context_id) {
   }
 }
 
 Network::FilterStatus Context::onNetworkNewConnection() {
   onCreate(root_context_id_);
+  in_vm_context_created_ = true;
   if (!wasm_->onNewConnection_) {
     return Network::FilterStatus::Continue;
   }
   if (wasm_->onNewConnection_(this, id_).u64_ == 0) {
     return Network::FilterStatus::Continue;
   }
   return Network::FilterStatus::StopIteration;
 }
 
 Network::FilterStatus Context::onDownstreamData(int data_length, bool end_of_stream) {
-  if (!wasm_->onDownstreamData_) {
+  if (!in_vm_context_created_ || !wasm_->onDownstreamData_) {
     return Network::FilterStatus::Continue;
   }
   auto result = wasm_->onDownstreamData_(this, id_, static_cast<uint32_t>(data_length),
                                          static_cast<uint32_t>(end_of_stream));
   // TODO(PiotrSikora): pull Proxy-WASM's FilterStatus values.
   return result.u64_ == 0 ? Network::FilterStatus::Continue : Network::FilterStatus::StopIteration;
 }
 
 Network::FilterStatus Context::onUpstreamData(int data_length, bool end_of_stream) {
-  if (!wasm_->onUpstreamData_) {
+  if (!in_vm_context_created_ || !wasm_->onUpstreamData_) {
     return Network::FilterStatus::Continue;
   }
   auto result = wasm_->onUpstreamData_(this, id_, static_cast<uint32_t>(data_length),
                                        static_cast<uint32_t>(end_of_stream));
   // TODO(PiotrSikora): pull Proxy-WASM's FilterStatus values.
   return result.u64_ == 0 ? Network::FilterStatus::Continue : Network::FilterStatus::StopIteration;
 }
 
 void Context::onDownstreamConnectionClose(PeerType peer_type) {
-  if (wasm_->onDownstreamConnectionClose_) {
+  if (in_vm_context_created_ && wasm_->onDownstreamConnectionClose_) {
     wasm_->onDownstreamConnectionClose_(this, id_, static_cast<uint32_t>(peer_type));
   }
 }
 
 void Context::onUpstreamConnectionClose(PeerType peer_type) {
-  if (wasm_->onUpstreamConnectionClose_) {
+  if (in_vm_context_created_ && wasm_->onUpstreamConnectionClose_) {
     wasm_->onUpstreamConnectionClose_(this, id_, static_cast<uint32_t>(peer_type));
   }
 }
 
 Http::FilterHeadersStatus Context::onRequestHeaders() {
@@ -1783,11 +1785,11 @@ Http::FilterHeadersStatus Context::onRequestHeaders() {
   }
   return Http::FilterHeadersStatus::StopIteration;
 }
 
 Http::FilterDataStatus Context::onRequestBody(int body_buffer_length, bool end_of_stream) {
-  if (!wasm_->onRequestBody_) {
+  if (!in_vm_context_created_ || !wasm_->onRequestBody_) {
     return Http::FilterDataStatus::Continue;
   }
   switch (wasm_
               ->onRequestBody_(this, id_, static_cast<uint32_t>(body_buffer_length),
                                static_cast<uint32_t>(end_of_stream))
@@ -1802,21 +1804,21 @@ Http::FilterDataStatus Context::onRequestBody(int body_buffer_length, bool end_o
     return Http::FilterDataStatus::StopIterationNoBuffer;
   }
 }
 
 Http::FilterTrailersStatus Context::onRequestTrailers() {
-  if (!wasm_->onRequestTrailers_) {
+  if (!in_vm_context_created_ || !wasm_->onRequestTrailers_) {
     return Http::FilterTrailersStatus::Continue;
   }
   if (wasm_->onRequestTrailers_(this, id_).u64_ == 0) {
     return Http::FilterTrailersStatus::Continue;
   }
   return Http::FilterTrailersStatus::StopIteration;
 }
 
 Http::FilterMetadataStatus Context::onRequestMetadata() {
-  if (!wasm_->onRequestMetadata_) {
+  if (!in_vm_context_created_ || !wasm_->onRequestMetadata_) {
     return Http::FilterMetadataStatus::Continue;
   }
   if (wasm_->onRequestMetadata_(this, id_).u64_ == 0) {
     return Http::FilterMetadataStatus::Continue;
   }
@@ -1840,11 +1842,11 @@ Http::FilterHeadersStatus Context::onResponseHeaders() {
   }
   return Http::FilterHeadersStatus::StopIteration;
 }
 
 Http::FilterDataStatus Context::onResponseBody(int body_buffer_length, bool end_of_stream) {
-  if (!wasm_->onResponseBody_) {
+  if (!in_vm_context_created_ || !wasm_->onResponseBody_) {
     return Http::FilterDataStatus::Continue;
   }
   switch (wasm_
               ->onResponseBody_(this, id_, static_cast<uint32_t>(body_buffer_length),
                                 static_cast<uint32_t>(end_of_stream))
@@ -1859,21 +1861,21 @@ Http::FilterDataStatus Context::onResponseBody(int body_buffer_length, bool end_
     return Http::FilterDataStatus::StopIterationNoBuffer;
   }
 }
 
 Http::FilterTrailersStatus Context::onResponseTrailers() {
-  if (!wasm_->onResponseTrailers_) {
+  if (!in_vm_context_created_ || !wasm_->onResponseTrailers_) {
     return Http::FilterTrailersStatus::Continue;
   }
   if (wasm_->onResponseTrailers_(this, id_).u64_ == 0) {
     return Http::FilterTrailersStatus::Continue;
   }
   return Http::FilterTrailersStatus::StopIteration;
 }
 
 Http::FilterMetadataStatus Context::onResponseMetadata() {
-  if (!wasm_->onResponseMetadata_) {
+  if (!in_vm_context_created_ || !wasm_->onResponseMetadata_) {
     return Http::FilterMetadataStatus::Continue;
   }
   if (wasm_->onResponseMetadata_(this, id_).u64_ == 0) {
     return Http::FilterMetadataStatus::Continue;
   }
@@ -2443,23 +2445,23 @@ void Context::onDestroy() {
   destroyed_ = true;
   onDone();
 }
 
 void Context::onDone() {
-  if (wasm_->onDone_) {
+  if (in_vm_context_created_ && wasm_->onDone_) {
     wasm_->onDone_(this, id_);
   }
 }
 
 void Context::onLog() {
-  if (wasm_->onLog_) {
+  if (in_vm_context_created_ && wasm_->onLog_) {
     wasm_->onLog_(this, id_);
   }
 }
 
 void Context::onDelete() {
-  if (wasm_->onDelete_) {
+  if (in_vm_context_created_ && wasm_->onDelete_) {
     wasm_->onDelete_(this, id_);
   }
 }
 
 Http::FilterHeadersStatus Context::decodeHeaders(Http::HeaderMap& headers, bool end_stream) {
diff --git a/source/extensions/common/wasm/wasm.h b/source/extensions/common/wasm/wasm.h
index 4986e4bfa..7ecac9947 100644
--- a/source/extensions/common/wasm/wasm.h
+++ b/source/extensions/common/wasm/wasm.h
@@ -411,10 +411,12 @@ public:
                               bool end_stream); // stream only
 
   // Connection
   virtual bool isSsl();
 
+  void setInVmContextCreatedForTesting() { in_vm_context_created_ = true; }
+
 protected:
   friend class Wasm;
   friend struct AsyncClientHandler;
   friend struct GrpcCallClientHandler;
   friend struct GrpcStreamClientHandler;
diff --git a/test/extensions/wasm/wasm_test.cc b/test/extensions/wasm/wasm_test.cc
index 34fe5f4d4..38d71035f 100644
--- a/test/extensions/wasm/wasm_test.cc
+++ b/test/extensions/wasm/wasm_test.cc
@@ -182,11 +182,11 @@ TEST_P(WasmTest, DivByZero) {
       "{{ test_rundir }}/test/extensions/wasm/test_data/segv_cpp.wasm"));
   EXPECT_FALSE(code.empty());
   auto context = std::make_unique<TestContext>(wasm.get());
   EXPECT_CALL(*context, scriptLog_(spdlog::level::err, Eq("before div by zero")));
   EXPECT_TRUE(wasm->initialize(code, false));
-  wasm->setContext(context.get());
+  context->setInVmContextCreatedForTesting();
 
   if (GetParam() == "v8") {
     EXPECT_THROW_WITH_MESSAGE(
         context->onLog(), Extensions::Common::Wasm::WasmException,
         "Function: proxy_onLog failed: Uncaught RuntimeError: divide by zero");
@@ -386,10 +386,11 @@ TEST_P(WasmTest, StatsHighLevel) {
   EXPECT_NE(wasm, nullptr);
   const auto code = TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(
       "{{ test_rundir }}/test/extensions/wasm/test_data/stats_cpp.wasm"));
   EXPECT_FALSE(code.empty());
   auto context = std::make_unique<TestContext>(wasm.get());
+  context->setInVmContextCreatedForTesting();
 
   EXPECT_CALL(*context, scriptLog_(spdlog::level::trace, Eq("get counter = 1")));
   EXPECT_CALL(*context, scriptLog_(spdlog::level::debug, Eq("get counter = 2")));
   // recordMetric on a Counter is the same as increment.
   EXPECT_CALL(*context, scriptLog_(spdlog::level::info, Eq("get counter = 5")));

Vulnerable code:   if (!wasm_->onDownstreamData_) {
  if (!wasm_->onUpstreamData_) {
  if (wasm_->onDownstreamConnectionClose_) {
  if (wasm_->onUpstreamConnectionClose_) {
  if (!wasm_->onRequestBody_) {
  if (!wasm_->onRequestTrailers_) {
  if (!wasm_->onRequestMetadata_) {
  if (!wasm_->onResponseBody_) {
  if (!wasm_->onResponseTrailers_) {
  if (!wasm_->onResponseMetadata_) {
  if (wasm_->onDone_) {
  if (wasm_->onLog_) {
  if (wasm_->onDelete_) {
  wasm->setContext(context.get());
Fixed code:   in_vm_context_created_ = true;
  in_vm_context_created_ = true;
  if (!in_vm_context_created_ || !wasm_->onDownstreamData_) {
  if (!in_vm_context_created_ || !wasm_->onUpstreamData_) {
  if (in_vm_context_created_ && wasm_->onDownstreamConnectionClose_) {
  if (in_vm_context_created_ && wasm_->onUpstreamConnectionClose_) {
  if (!in_vm_context_created_ || !wasm_->onRequestBody_) {
  if (!in_vm_context_created_ || !wasm_->onRequestTrailers_) {
  if (!in_vm_context_created_ || !wasm_->onRequestMetadata_) {
  if (!in_vm_context_created_ || !wasm_->onResponseBody_) {
  if (!in_vm_context_created_ || !wasm_->onResponseTrailers_) {
  if (!in_vm_context_created_ || !wasm_->onResponseMetadata_) {
  if (in_vm_context_created_ && wasm_->onDone_) {
  if (in_vm_context_created_ && wasm_->onLog_) {
  if (in_vm_context_created_ && wasm_->onDelete_) {
  void setInVmContextCreatedForTesting() { in_vm_context_created_ = true; }

  context->setInVmContextCreatedForTesting();
  context->setInVmContextCreatedForTesting();