CVE-2022-21655: Always-Incorrect Control Flow Implementation (CWE-670)
value: Envoy is an open source edge and service proxy, designed for cloud-native applications. The envoy common router will segfault if an internal redirect selects a route configured with direct response or redirect actions. This will result in a denial of service. As a workaround turn off internal redirects if direct response entries are configured on the same listener.
CWE description: The code contains a control flow path that does not reflect the algorithm that the path is intended to implement, leading to incorrect behavior any time this path is navigated.
CVSS v3 base score: 7.5
Published: 2022-02-22T23:15Z
Repository: https://github.com/envoyproxy/envoy
Language: C++
Files changed: docs/root/version_history/current.rst, source/common/router/router.cc, test/common/router/router_test.cc, test/integration/redirect_integration_test.cc
Commit date: 2022-02-22 19:38:45 +0000
Commit message: CVE-2022-21655

Crash with direct_response

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Diff: diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
index a5b98afcf26..687c57a09f9 100644
--- a/docs/root/version_history/current.rst
+++ b/docs/root/version_history/current.rst
@@ -23,10 +23,11 @@ Minor Behavior Changes
 Bug Fixes
 ---------
 *Changes expected to improve the state of the world and are unlikely to have negative effects*
 
 * access_log: fix memory leak when reopening an access log fails. Access logs will now try to be reopened on each subsequent flush attempt after a failure.
+* data plane: fix crash when internal redirect selects a route configured with direct response or redirect actions.
 * data plane: fixing error handling where writing to a socket failed while under the stack of processing. This should genreally affect HTTP/3. This behavioral change can be reverted by setting ``envoy.reloadable_features.allow_upstream_inline_write`` to false.
 * eds: fix the eds cluster update by allowing update on the locality of the cluster endpoints. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.support_locality_update_on_eds_cluster_endpoints`` to false.
 * jwt_authn: fixed the crash when a CONNECT request is sent to JWT filter configured with regex match on the Host header.
 * tcp_proxy: fix a crash that occurs when configured for :ref:`upstream tunneling <envoy_v3_api_field_extensions.filters.network.tcp_proxy.v3.TcpProxy.tunneling_config>` and the downstream connection disconnects while the the upstream connection or http/2 stream is still being established.
 * tls: fix a bug while matching a certificate SAN with an exact value in ``match_typed_subject_alt_names`` of a listener where wildcard ``*`` character is not the only character of the dns label. Example, ``baz*.example.net`` and ``*baz.example.net`` and ``b*z.example.net`` will match ``baz1.example.net`` and ``foobaz.example.net`` and ``buzz.example.net``, respectively.
diff --git a/source/common/router/router.cc b/source/common/router/router.cc
index 9eb0a08e48e..a9fe96e6739 100644
--- a/source/common/router/router.cc
+++ b/source/common/router/router.cc
@@ -1690,11 +1690,12 @@ bool Filter::convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& do
     config_.stats_.passthrough_internal_redirect_no_route_.inc();
     ENVOY_STREAM_LOG(trace, "Internal redirect failed: no route found", *callbacks_);
     return false;
   }
 
-  const auto& route_name = route->routeEntry()->routeName();
+  const auto& route_name = route->directResponseEntry() ? route->directResponseEntry()->routeName()
+                                                        : route->routeEntry()->routeName();
   for (const auto& predicate : policy.predicates()) {
     if (!predicate->acceptTargetRoute(*filter_state, route_name, !scheme_is_http,
                                       !target_is_http)) {
       config_.stats_.passthrough_internal_redirect_predicate_.inc();
       ENVOY_STREAM_LOG(trace,
diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc
index 6404703e9b4..c43f3f6ae36 100644
--- a/test/common/router/router_test.cc
+++ b/test/common/router/router_test.cc
@@ -54,10 +54,11 @@
 using testing::_;
 using testing::AtLeast;
 using testing::Eq;
 using testing::InSequence;
 using testing::Invoke;
+using testing::InvokeWithoutArgs;
 using testing::MockFunction;
 using testing::NiceMock;
 using testing::Property;
 using testing::Return;
 using testing::ReturnRef;
@@ -317,11 +318,11 @@ TEST_F(RouterTest, MissingRequiredHeaders) {
   EXPECT_CALL(
       callbacks_,
       sendLocalReply(Http::Code::ServiceUnavailable,
                      testing::Eq("missing required header: :method"), _, _,
                      "filter_removed_required_request_headers{missing_required_header:_:method}"))
-      .WillOnce(testing::InvokeWithoutArgs([] {}));
+      .WillOnce(InvokeWithoutArgs([] {}));
   router_.decodeHeaders(headers, true);
   router_.onDestroy();
 }
 
 TEST_F(RouterTest, ClusterNotFound) {
@@ -3409,12 +3410,11 @@ TEST_F(RouterTest, RetryUpstreamResetResponseStarted) {
   EXPECT_CALL(cm_.thread_local_cluster_.conn_pool_.host_->outlier_detector_,
               putResult(Upstream::Outlier::Result::LocalOriginConnectFailed, _));
   // Normally, sendLocalReply will actually send the reply, but in this case the
   // HCM will detect the headers have already been sent and not route through
   // the encoder again.
-  EXPECT_CALL(callbacks_, sendLocalReply(_, _, _, _, _)).WillOnce(testing::InvokeWithoutArgs([] {
-  }));
+  EXPECT_CALL(callbacks_, sendLocalReply(_, _, _, _, _)).WillOnce(InvokeWithoutArgs([] {}));
   encoder1.stream_.resetStream(Http::StreamResetReason::RemoteReset);
   // For normal HTTP, once we have a 200 we consider this a success, even if a
   // later reset occurs.
   EXPECT_TRUE(verifyHostUpstreamStats(1, 0));
   EXPECT_EQ(1U,
@@ -4205,10 +4205,37 @@ TEST_F(RouterTest, HttpInternalRedirectSucceeded) {
                    .filterState()
                    ->getDataMutable<StreamInfo::UInt32Accessor>("num_internal_redirects")
                    ->value());
 }
 
+TEST_F(RouterTest, HttpInternalRedirectMatchedToDirectResponseSucceeded) {
+  NiceMock<MockDirectResponseEntry> direct_response;
+  std::string route_name("route-test-name");
+  EXPECT_CALL(direct_response, routeName()).WillOnce(ReturnRef(route_name));
+
+  enableRedirects();
+  sendRequest();
+  EXPECT_CALL(callbacks_, clearRouteCache()).WillOnce(InvokeWithoutArgs([&]() -> void {
+    // Direct message route should be matched after internal redirect
+    EXPECT_CALL(*callbacks_.route_, routeEntry()).WillRepeatedly(Return(nullptr));
+    EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response));
+  }));
+  EXPECT_CALL(callbacks_, recreateStream(_)).WillOnce(Return(true));
+
+  response_decoder_->decodeHeaders(std::move(redirect_headers_), false);
+  EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_
+                    .counter("upstream_internal_redirect_succeeded_total")
+                    .value());
+
+  // In production, the HCM recreateStream would have called this.
+  router_.onDestroy();
+  EXPECT_EQ(1, callbacks_.streamInfo()
+                   .filterState()
+                   ->getDataMutable<StreamInfo::UInt32Accessor>("num_internal_redirects")
+                   ->value());
+}
+
 TEST_F(RouterTest, InternalRedirectStripsFragment) {
   enableRedirects();
   default_request_headers_.setForwardedProto("http");
   sendRequest();
 
diff --git a/test/integration/redirect_integration_test.cc b/test/integration/redirect_integration_test.cc
index 2ee3e39e4f2..a40fe7f392f 100644
--- a/test/integration/redirect_integration_test.cc
+++ b/test/integration/redirect_integration_test.cc
@@ -37,10 +37,19 @@ public:
         ->mutable_internal_redirect_policy()
         ->mutable_max_internal_redirects()
         ->set_value(3);
     config_helper_.addVirtualHost(handle_max_3_hop);
 
+    auto handle_by_direct_response = config_helper_.createVirtualHost("handle.direct.response");
+    handle_by_direct_response.mutable_routes(0)->set_name("direct_response");
+    handle_by_direct_response.mutable_routes(0)->mutable_direct_response()->set_status(204);
+    handle_by_direct_response.mutable_routes(0)
+        ->mutable_direct_response()
+        ->mutable_body()
+        ->set_inline_string(EMPTY_STRING);
+    config_helper_.addVirtualHost(handle_by_direct_response);
+
     HttpProtocolIntegrationTest::initialize();
   }
 
 protected:
   // Returns the next stream that the fake upstream receives.
@@ -663,10 +672,43 @@ TEST_P(RedirectIntegrationTest, InvalidRedirect) {
               HasSubstr("302 via_upstream test-header-value\n"));
   EXPECT_EQ("test-header-value",
             response->headers().get(test_header_key_)[0]->value().getStringView());
 }
 
+TEST_P(RedirectIntegrationTest, InternalRedirectHandledByDirectResponse) {
+  useAccessLog("%RESPONSE_FLAGS% %RESPONSE_CODE% %RESPONSE_CODE_DETAILS% %RESP(test-header)%");
+  // Validate that header sanitization is only called once.
+  config_helper_.addConfigModifier(
+      [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
+             hcm) { hcm.set_via("via_value"); });
+  initialize();
+
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+
+  default_request_headers_.setHost("handle.internal.redirect");
+  IntegrationStreamDecoderPtr response =
+      codec_client_->makeHeaderOnlyRequest(default_request_headers_);
+
+  waitForNextUpstreamRequest();
+
+  redirect_response_.setLocation("http://handle.direct.response/");
+  upstream_request_->encodeHeaders(redirect_response_, true);
+
+  ASSERT_TRUE(response->waitForEndStream());
+  ASSERT_TRUE(response->complete());
+  EXPECT_EQ("204", response->headers().getStatusValue());
+  EXPECT_EQ(1, test_server_->counter("cluster.cluster_0.upstream_internal_redirect_succeeded_total")
+                   ->value());
+  // 302 was never returned downstream
+  EXPECT_EQ(0, test_server_->counter("http.config_test.downstream_rq_3xx")->value());
+  EXPECT_EQ(1, test_server_->counter("http.config_test.downstream_rq_2xx")->value());
+  EXPECT_THAT(waitForAccessLog(access_log_name_, 0),
+              HasSubstr("302 internal_redirect test-header-value\n"));
+  // No test header
+  EXPECT_THAT(waitForAccessLog(access_log_name_, 1), HasSubstr("204 direct_response -\n"));
+}
+
 INSTANTIATE_TEST_SUITE_P(Protocols, RedirectIntegrationTest,
                          testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()),
                          HttpProtocolIntegrationTest::protocolTestParamsToString);
 
 } // namespace Envoy

Vulnerable code:   const auto& route_name = route->routeEntry()->routeName();
      .WillOnce(testing::InvokeWithoutArgs([] {}));
  EXPECT_CALL(callbacks_, sendLocalReply(_, _, _, _, _)).WillOnce(testing::InvokeWithoutArgs([] {
  }));
Fixed code: * data plane: fix crash when internal redirect selects a route configured with direct response or redirect actions.
  const auto& route_name = route->directResponseEntry() ? route->directResponseEntry()->routeName()
                                                        : route->routeEntry()->routeName();
using testing::InvokeWithoutArgs;
      .WillOnce(InvokeWithoutArgs([] {}));
  EXPECT_CALL(callbacks_, sendLocalReply(_, _, _, _, _)).WillOnce(InvokeWithoutArgs([] {}));
TEST_F(RouterTest, HttpInternalRedirectMatchedToDirectResponseSucceeded) {
  NiceMock<MockDirectResponseEntry> direct_response;
  std::string route_name("route-test-name");
  EXPECT_CALL(direct_response, routeName()).WillOnce(ReturnRef(route_name));

  enableRedirects();
  sendRequest();
  EXPECT_CALL(callbacks_, clearRouteCache()).WillOnce(InvokeWithoutArgs([&]() -> void {
    // Direct message route should be matched after internal redirect
    EXPECT_CALL(*callbacks_.route_, routeEntry()).WillRepeatedly(Return(nullptr));
    EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response));
  }));
  EXPECT_CALL(callbacks_, recreateStream(_)).WillOnce(Return(true));

  response_decoder_->decodeHeaders(std::move(redirect_headers_), false);
  EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_
                    .counter("upstream_internal_redirect_succeeded_total")
                    .value());

  // In production, the HCM recreateStream would have called this.
  router_.onDestroy();
  EXPECT_EQ(1, callbacks_.streamInfo()
                   .filterState()
                   ->getDataMutable<StreamInfo::UInt32Accessor>("num_internal_redirects")
                   ->value());
}

    auto handle_by_direct_response = config_helper_.createVirtualHost("handle.direct.response");
    handle_by_direct_response.mutable_routes(0)->set_name("direct_response");
    handle_by_direct_response.mutable_routes(0)->mutable_direct_response()->set_status(204);
    handle_by_direct_response.mutable_routes(0)
        ->mutable_direct_response()
        ->mutable_body()
        ->set_inline_string(EMPTY_STRING);
    config_helper_.addVirtualHost(handle_by_direct_response);

TEST_P(RedirectIntegrationTest, InternalRedirectHandledByDirectResponse) {
  useAccessLog("%RESPONSE_FLAGS% %RESPONSE_CODE% %RESPONSE_CODE_DETAILS% %RESP(test-header)%");
  // Validate that header sanitization is only called once.
  config_helper_.addConfigModifier(
      [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
             hcm) { hcm.set_via("via_value"); });
  initialize();

  codec_client_ = makeHttpConnection(lookupPort("http"));

  default_request_headers_.setHost("handle.internal.redirect");
  IntegrationStreamDecoderPtr response =
      codec_client_->makeHeaderOnlyRequest(default_request_headers_);

  waitForNextUpstreamRequest();

  redirect_response_.setLocation("http://handle.direct.response/");
  upstream_request_->encodeHeaders(redirect_response_, true);

  ASSERT_TRUE(response->waitForEndStream());
  ASSERT_TRUE(response->complete());
  EXPECT_EQ("204", response->headers().getStatusValue());
  EXPECT_EQ(1, test_server_->counter("cluster.cluster_0.upstream_internal_redirect_succeeded_total")
                   ->value());
  // 302 was never returned downstream
  EXPECT_EQ(0, test_server_->counter("http.config_test.downstream_rq_3xx")->value());
  EXPECT_EQ(1, test_server_->counter("http.config_test.downstream_rq_2xx")->value());
  EXPECT_THAT(waitForAccessLog(access_log_name_, 0),
              HasSubstr("302 internal_redirect test-header-value\n"));
  // No test header
  EXPECT_THAT(waitForAccessLog(access_log_name_, 1), HasSubstr("204 direct_response -\n"));
}