Fix use-after-free error for remote .wasm file accesses. (envoyproxy#427

)

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

source/extensions/common/wasm/wasm.cc

@@ -51,10 +51,6 @@ namespace Extensions {
 namespace Common {
 namespace Wasm {
 
-namespace {
-
-std::atomic<int64_t> active_wasm_;
-
 std::string Sha256(absl::string_view data) {
   std::vector<uint8_t> digest(SHA256_DIGEST_LENGTH);
   EVP_MD_CTX* ctx(EVP_MD_CTX_new());
@@ -68,6 +64,10 @@ std::string Sha256(absl::string_view data) {
   return std::string(reinterpret_cast<const char*>(&digest[0]), digest.size());
 }
 
+namespace {
+
+std::atomic<int64_t> active_wasm_;
+
 std::string Xor(absl::string_view a, absl::string_view b) {
   ASSERT(a.size() == b.size());
   std::string result;

source/extensions/common/wasm/wasm.h

@@ -286,6 +286,8 @@ class WasmHandle : public Envoy::Server::Wasm,
 };
 using WasmHandleSharedPtr = std::shared_ptr<WasmHandle>;
 
+std::string Sha256(absl::string_view data);
+
 using CreateWasmCallback = std::function<void(WasmHandleSharedPtr)>;
 
 // Create a high level Wasm VM with Envoy API support. Note: 'id' may be empty if this VM will not

source/extensions/filters/http/wasm/wasm_filter.cc

@@ -22,8 +22,8 @@ FilterConfig::FilterConfig(const envoy::extensions::filters::http::wasm::v3::Was
       context.direction(), context.localInfo(), &context.listenerMetadata());
 
   auto plugin = plugin_;
-  auto callback = [&config, plugin, this](Common::Wasm::WasmHandleSharedPtr base_wasm) {
-    auto configuration = std::make_shared<std::string>(config.config().configuration());
+  auto configuration = std::make_shared<std::string>(config.config().configuration());
+  auto callback = [configuration, plugin, this](Common::Wasm::WasmHandleSharedPtr base_wasm) {
     // NB: the Slot set() call doesn't complete inline, so all arguments must outlive this call.
     tls_slot_->set([base_wasm, plugin, configuration](Event::Dispatcher& dispatcher) {
       return std::static_pointer_cast<ThreadLocal::ThreadLocalObject>(

source/extensions/filters/network/wasm/wasm_filter.cc

@@ -17,8 +17,8 @@ FilterConfig::FilterConfig(const envoy::extensions::filters::network::wasm::v3::
       context.direction(), context.localInfo(), &context.listenerMetadata());
 
   auto plugin = plugin_;
-  auto callback = [&config, plugin, this](Common::Wasm::WasmHandleSharedPtr base_wasm) {
-    auto configuration = std::make_shared<std::string>(config.config().configuration());
+  auto configuration = std::make_shared<std::string>(config.config().configuration());
+  auto callback = [configuration, plugin, this](Common::Wasm::WasmHandleSharedPtr base_wasm) {
     // NB: the Slot set() call doesn't complete inline, so all arguments must outlive this call.
     tls_slot_->set([base_wasm, plugin, configuration](Event::Dispatcher& dispatcher) {
       return std::static_pointer_cast<ThreadLocal::ThreadLocalObject>(

source/extensions/wasm/config.cc

@@ -23,19 +23,21 @@ void WasmFactory::createWasm(const envoy::extensions::wasm::v3::WasmService& con
       config.config().name(), config.config().root_id(), config.config().vm_config().vm_id(),
       envoy::config::core::v3::TrafficDirection::UNSPECIFIED, context.localInfo(), nullptr);
 
-  auto callback = [&context, &config, plugin, cb](Common::Wasm::WasmHandleSharedPtr base_wasm) {
-    if (config.singleton()) {
+  auto configuration = std::make_shared<std::string>(config.config().configuration());
+  bool singleton = config.singleton();
+  auto callback = [&context, configuration, singleton, plugin,
+                   cb](Common::Wasm::WasmHandleSharedPtr base_wasm) {
+    if (singleton) {
       // Return the WASM VM which will be stored as a singleton by the Server.
       auto root_context = base_wasm->wasm()->start(plugin);
-      if (!base_wasm->wasm()->configure(root_context, plugin, config.config().configuration())) {
+      if (!base_wasm->wasm()->configure(root_context, plugin, *configuration)) {
         cb(nullptr);
       }
       return cb(base_wasm);
     }
     // Per-thread WASM VM.
     // NB: the Slot set() call doesn't complete inline, so all arguments must outlive this call.
     // NB: no need to keep the resulting slot as Wasm is cached on each thread.
-    auto configuration = std::make_shared<std::string>(config.config().configuration());
     context.threadLocal().allocateSlot()->set([base_wasm, plugin,
                                                configuration](Event::Dispatcher& dispatcher) {
       return std::static_pointer_cast<ThreadLocal::ThreadLocalObject>(

test/extensions/common/wasm/BUILD

@@ -29,9 +29,12 @@ envoy_cc_test(
     ],
     external_deps = ["abseil_optional"],
     deps = [
+        "//source/common/common:hex_lib",
+        "//source/common/crypto:utility_lib",
         "//source/common/event:dispatcher_lib",
         "//source/common/stats:isolated_store_lib",
         "//source/common/stats:stats_lib",
+        "//source/extensions/common/crypto:utility_lib",
         "//source/extensions/common/wasm:wasm_lib",
         "//test/extensions/common/wasm/test_data:test_cpp_plugin",
         "//test/mocks/server:server_mocks",

test/extensions/common/wasm/wasm_test.cc

@@ -1,5 +1,6 @@
 #include <stdio.h>
 
+#include "common/common/hex.h"
 #include "common/event/dispatcher_impl.h"
 #include "common/stats/isolated_store_impl.h"
 
@@ -15,6 +16,7 @@
 #include "gtest/gtest.h"
 
 using testing::Eq;
+using testing::Return;
 
 namespace Envoy {
 namespace Extensions {
@@ -414,6 +416,84 @@ TEST_P(WasmCommonTest, VmCache) {
   dispatcher->clearDeferredDeleteList();
 }
 
+TEST_P(WasmCommonTest, RemoteCode) {
+  if (GetParam() == "null") {
+    return;
+  }
+  Stats::IsolatedStoreImpl stats_store;
+  Api::ApiPtr api = Api::createApiForTest(stats_store);
+  NiceMock<Upstream::MockClusterManager> cluster_manager;
+  NiceMock<Init::MockManager> init_manager;
+  Init::ExpectableWatcherImpl init_watcher;
+  Event::DispatcherPtr dispatcher(api->allocateDispatcher());
+  Config::DataSource::RemoteAsyncDataProviderPtr remote_data_provider;
+  auto scope = Stats::ScopeSharedPtr(stats_store.createScope("wasm."));
+  NiceMock<LocalInfo::MockLocalInfo> local_info;
+  auto name = "";
+  auto root_id = "";
+  auto vm_id = "";
+  auto vm_configuration = "vm_cache";
+  auto plugin = std::make_shared<Extensions::Common::Wasm::Plugin>(
+      name, root_id, vm_id, envoy::config::core::v3::TrafficDirection::UNSPECIFIED, local_info,
+      nullptr);
+
+  std::string code = TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(
+      absl::StrCat("{{ test_rundir }}/test/extensions/common/wasm/test_data/test_cpp.wasm")));
+
+  VmConfig vm_config;
+  vm_config.set_runtime(absl::StrCat("envoy.wasm.runtime.", GetParam()));
+  vm_config.set_configuration(vm_configuration);
+  std::string sha256 = Extensions::Common::Wasm::Sha256(code);
+  std::string sha256Hex =
+      Hex::encode(reinterpret_cast<const uint8_t*>(&*sha256.begin()), sha256.size());
+  vm_config.mutable_code()->mutable_remote()->set_sha256(sha256Hex);
+  vm_config.mutable_code()->mutable_remote()->mutable_http_uri()->set_uri(
+      "http://example.com/test.wasm");
+  vm_config.mutable_code()->mutable_remote()->mutable_http_uri()->set_cluster("example_com");
+  vm_config.mutable_code()->mutable_remote()->mutable_http_uri()->mutable_timeout()->set_seconds(5);
+  WasmHandleSharedPtr wasm_handle;
+  auto root_context = new Extensions::Common::Wasm::TestContext();
+
+  EXPECT_CALL(*root_context, scriptLog_(spdlog::level::info, Eq("on_vm_start vm_cache")));
+  EXPECT_CALL(*root_context, scriptLog_(spdlog::level::info, Eq("on_done logging")));
+  EXPECT_CALL(*root_context, scriptLog_(spdlog::level::info, Eq("on_delete logging")));
+
+  EXPECT_CALL(cluster_manager, httpAsyncClientForCluster("example_com"))
+      .WillOnce(ReturnRef(cluster_manager.async_client_));
+  EXPECT_CALL(cluster_manager.async_client_, send_(_, _, _))
+      .WillOnce(
+          Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks,
+                     const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* {
+            Http::ResponseMessagePtr response(
+                new Http::ResponseMessageImpl(Http::ResponseHeaderMapPtr{
+                    new Http::TestResponseHeaderMapImpl{{":status", "200"}}}));
+            response->body() = std::make_unique<Buffer::OwnedImpl>(code);
+            callbacks.onSuccess(std::move(response));
+            return nullptr;
+          }));
+
+  Init::TargetHandlePtr init_target_handle;
+  EXPECT_CALL(init_manager, add(_)).WillOnce(Invoke([&](const Init::Target& target) {
+    init_target_handle = target.createHandle("test");
+  }));
+  createWasmForTesting(vm_config, plugin, scope, cluster_manager, init_manager, *dispatcher, *api,
+                       std::unique_ptr<Context>(root_context), remote_data_provider,
+                       [&wasm_handle](WasmHandleSharedPtr w) { wasm_handle = w; });
+
+  EXPECT_CALL(init_watcher, ready());
+  init_target_handle->initialize(init_watcher);
+
+  EXPECT_NE(wasm_handle, nullptr);
+
+  plugin.reset();
+  auto wasm = wasm_handle->wasm().get();
+  wasm_handle.reset();
+  dispatcher->run(Event::Dispatcher::RunType::NonBlock);
+  wasm->configure(root_context, plugin, "done");
+  dispatcher->run(Event::Dispatcher::RunType::NonBlock);
+  dispatcher->clearDeferredDeleteList();
+}
+
 } // namespace Wasm
 } // namespace Common
 } // namespace Extensions