Cors: Fix for potential use-after-free

Signed-off-by: Kevin Baichoo <kbaichoo@google.com> Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com> Signed-off-by: Ryan Northey <ryan@synca.io> Signed-off-by: Boteng Yao <boteng@google.com>
envoyproxy · Jul 25, 2023 · 806bc47 · 806bc47
1 parent d8138c3
commit 806bc47
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 6 deletions.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -8,6 +8,10 @@ minor_behavior_changes:
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*
+- area: cors
+  change: |
+    Fix a use-after-free bug that occurs in the CORS filter if the ``origin`` header is removed between
+    request header decoding and response header encoding.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

diff --git a/source/extensions/filters/http/cors/cors_filter.cc b/source/extensions/filters/http/cors/cors_filter.cc
@@ -62,12 +62,14 @@ Http::FilterHeadersStatus CorsFilter::decodeHeaders(Http::RequestHeaderMap& head
     return Http::FilterHeadersStatus::Continue;
   }
 
-  origin_ = headers.getInline(origin_handle.handle());
-  if (origin_ == nullptr || origin_->value().empty()) {
+  const Http::HeaderEntry* origin = headers.getInline(origin_handle.handle());
+  if (origin == nullptr || origin->value().empty()) {
     return Http::FilterHeadersStatus::Continue;
   }
 
-  if (!isOriginAllowed(origin_->value())) {
+  latched_origin_ = std::string(origin->value().getStringView());
+
+  if (!isOriginAllowed(origin->value())) {
     config_->stats().origin_invalid_.inc();
     return Http::FilterHeadersStatus::Continue;
   }
@@ -92,7 +94,7 @@ Http::FilterHeadersStatus CorsFilter::decodeHeaders(Http::RequestHeaderMap& head
       {{Http::Headers::get().Status, std::to_string(enumToInt(Http::Code::OK))}})};
 
   response_headers->setInline(access_control_allow_origin_handle.handle(),
-                              origin_->value().getStringView());
+                              origin->value().getStringView());
 
   if (allowCredentials()) {
     response_headers->setReferenceInline(access_control_allow_credentials_handle.handle(),
@@ -138,7 +140,7 @@ Http::FilterHeadersStatus CorsFilter::encodeHeaders(Http::ResponseHeaderMap& hea
     return Http::FilterHeadersStatus::Continue;
   }
 
-  headers.setInline(access_control_allow_origin_handle.handle(), origin_->value().getStringView());
+  headers.setInline(access_control_allow_origin_handle.handle(), latched_origin_);
   if (allowCredentials()) {
     headers.setReferenceInline(access_control_allow_credentials_handle.handle(),
                                Http::CustomHeaders::get().CORSValues.True);

diff --git a/source/extensions/filters/http/cors/cors_filter.h b/source/extensions/filters/http/cors/cors_filter.h
@@ -96,7 +96,7 @@ class CorsFilter : public Http::StreamFilter {
   Http::StreamEncoderFilterCallbacks* encoder_callbacks_{};
   std::array<const Envoy::Router::CorsPolicy*, 2> policies_;
   bool is_cors_request_{};
-  const Http::HeaderEntry* origin_{};
+  std::string latched_origin_;
 
   CorsFilterConfigSharedPtr config_;
 };

diff --git a/test/extensions/filters/http/cors/cors_filter_test.cc b/test/extensions/filters/http/cors/cors_filter_test.cc
@@ -758,6 +758,37 @@ TEST_F(CorsFilterTest, OptionsRequestWithWildcardAllowHeaders) {
   EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers_));
 }
 
+TEST_F(CorsFilterTest, CorsFilterLatchesRequestOriginValue) {
+  Http::TestRequestHeaderMapImpl request_headers{{":method", "OPTIONS"},
+                                                 {"origin", "www.envoyproxy.com"},
+                                                 {"access-control-request-method", "GET"}};
+
+  Http::TestResponseHeaderMapImpl response_headers{
+      {":status", "200"},
+      {"access-control-allow-origin", "www.envoyproxy.com"},
+      {"access-control-allow-methods", "GET"},
+      {"access-control-allow-headers", "content-type"},
+      {"access-control-max-age", "0"},
+  };
+
+  cors_policy_->allow_methods_ = "*";
+
+  EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true));
+
+  EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
+            filter_.decodeHeaders(request_headers, false));
+  EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(data_, false));
+  EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(request_trailers_));
+
+  // Remove the origin request header from the header map. If we don't
+  // latch the value we will have a dangling pointer.
+  request_headers.remove("origin");
+
+  EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers_, false));
+  EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(data_, false));
+  EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers_));
+}
+
 } // namespace Cors
 } // namespace HttpFilters
 } // namespace Extensions