Merge pull request #292 from dcillera/cve-and-tidy-up

dcillera · web-flow · commit bc44aadbc7d5 · 2025-01-09T10:29:56.000+01:00
CVEs fixes and undo some modifications
diff --git a/.github/config.yml b/.github/config.yml
@@ -1,4 +1,4 @@
-agent-ubuntu: ubuntu-22.04
+agent-ubuntu: ubuntu-24.04
 build-image:
   # Authoritative configuration for build image/s
   repo: envoyproxy/envoy-build-ubuntu
diff --git a/VERSION.txt b/VERSION.txt
@@ -1 +1 @@
-1.32.3-dev
+1.32.3
diff --git a/changelogs/1.29.12.yaml b/changelogs/1.29.12.yaml
@@ -0,0 +1,6 @@
+date: December 18, 2024
+
+bug_fixes:
+- area: http/1
+  change: |
+    Fixes sending overload crashes when HTTP/1 request is reset.
diff --git a/changelogs/1.30.9.yaml b/changelogs/1.30.9.yaml
@@ -0,0 +1,9 @@
+date: December 18, 2024
+
+bug_fixes:
+- area: http/1
+  change: |
+    Fixes sending overload crashes when HTTP/1 request is reset.
+- area: happy_eyeballs
+  change: |
+    Validate that ``additional_address`` are IP addresses instead of crashing when sorting.
diff --git a/changelogs/1.31.5.yaml b/changelogs/1.31.5.yaml
@@ -0,0 +1,13 @@
+date: December 18, 2024
+
+bug_fixes:
+- area: http/1
+  change: |
+    Fixes sending overload crashes when HTTP/1 request is reset.
+- area: happy_eyeballs
+  change: |
+    Validate that ``additional_address`` are IP addresses instead of crashing when sorting.
+- area: balsa
+  change: |
+    Fix incorrect handling of non-101 1xx responses. This fix can be temporarily reverted by setting runtime guard
+    ``envoy.reloadable_features.wait_for_first_byte_before_balsa_msg_done`` to false.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -1,17 +1,13 @@
-date: Pending
-
-behavior_changes:
-# *Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required*
-
-minor_behavior_changes:
-# *Changes that may cause incompatibilities for some users, but should not for most*
+date: December 18, 2024
 
 bug_fixes:
-# *Changes expected to improve the state of the world and are unlikely to have negative effects*
-
-removed_config_or_runtime:
-# *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`
-
-new_features:
-
-deprecated:
+- area: http/1
+  change: |
+    Fixes sending overload crashes when HTTP/1 request is reset.
+- area: happy_eyeballs
+  change: |
+    Validate that ``additional_address`` are IP addresses instead of crashing when sorting.
+- area: balsa
+  change: |
+    Fix incorrect handling of non-101 1xx responses. This fix can be temporarily reverted by setting runtime guard
+    ``envoy.reloadable_features.wait_for_first_byte_before_balsa_msg_done`` to false.
diff --git a/changelogs/summary.md b/changelogs/summary.md
@@ -1,14 +1 @@
-**Summary of changes**:
 
-* Envoy now logs warnings when `internal_address_config` is not set.  If you see this logged warning and wish to retain trusted status for internal addresses you must explicitly configure `internal_address_config` (which will turn off the warning) before the next Envoy release.
-* Removed support for (long deprecated) opentracing. 
-* Added a configuration setting for the maximum size of response headers in responses.
-* Added support for `connection_pool_per_downstream_connection` flag in tcp connection pool.
-* For the strict DNS and logical DNS cluster types, the `dns_jitter` field allows spreading out DNS refresh requests
-* Added dynamic metadata matcher support `dynamic metadata input` and `dynamic metadata input matcher`.
-* The xff original IP detection method now supports using a list of trusted CIDRs when parsing `x-forwarded-for`.
-* QUIC server and client support certificate compression, which can in some cases reduce the number of round trips required to setup a connection.
-* Added the ability to monitor CPU utilization in Linux based systems via `cpu utilization monitor` in overload manager.
-* Added new access log command operators (`%START_TIME_LOCAL%` and `%EMIT_TIME_LOCAL%`) formatters (`%UPSTREAM_CLUSTER_RAW%` `%DOWNSTREAM_PEER_CHAIN_FINGERPRINTS_256%`, and `%DOWNSTREAM_PEER_CHAIN_SERIALS%`) as well as significant boosts to json parsing.  See release notes for details
-* Added support for `%BYTES_RECEIVED%`, `%BYTES_SENT%`, `%UPSTREAM_HEADER_BYTES_SENT%`, `%UPSTREAM_HEADER_BYTES_RECEIVED%`, `%UPSTREAM_WIRE_BYTES_SENT%`, `%UPSTREAM_WIRE_BYTES_RECEIVED%` and access log substitution strings for UDP tunneling flows.
-* Added ECDS support for UDP session filters.
diff --git a/docs/inventories/v1.29/objects.inv b/docs/inventories/v1.29/objects.inv
diff --git a/docs/inventories/v1.30/objects.inv b/docs/inventories/v1.30/objects.inv
diff --git a/docs/inventories/v1.31/objects.inv b/docs/inventories/v1.31/objects.inv
diff --git a/docs/inventories/v1.32/objects.inv b/docs/inventories/v1.32/objects.inv
diff --git a/docs/versions.yaml b/docs/versions.yaml
@@ -22,7 +22,7 @@
 "1.26": 1.26.8
 "1.27": 1.27.7
 "1.28": 1.28.7
-"1.29": 1.29.11
-"1.30": 1.30.8
-"1.31": 1.31.4
-"1.32": 1.32.1
+"1.29": 1.29.12
+"1.30": 1.30.9
+"1.31": 1.31.5
+"1.32": 1.32.2
diff --git a/source/common/http/http1/balsa_parser.cc b/source/common/http/http1/balsa_parser.cc
@@ -360,7 +360,10 @@ void BalsaParser::HeaderDone() {
 void BalsaParser::ContinueHeaderDone() {}
 
 void BalsaParser::MessageDone() {
-  if (status_ == ParserStatus::Error) {
+  if (status_ == ParserStatus::Error ||
+      // In the case of early 1xx, MessageDone() can be called twice in a row.
+      // The !first_byte_processed_ check is to make this function idempotent.
+      (wait_for_first_byte_before_msg_done_ && !first_byte_processed_)) {
     return;
   }
   status_ = convertResult(connection_->onMessageComplete());
diff --git a/source/common/http/http1/balsa_parser.h b/source/common/http/http1/balsa_parser.h
@@ -85,6 +85,9 @@ class BalsaParser : public Parser, public quiche::BalsaVisitorInterface {
   // Latched value of `envoy.reloadable_features.http1_balsa_delay_reset`.
   const bool delay_reset_ =
       Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http1_balsa_delay_reset");
+  // Latched value of `envoy.reloadable_features.wait_for_first_byte_before_balsa_msg_done`.
+  const bool wait_for_first_byte_before_msg_done_ = Runtime::runtimeFeatureEnabled(
+      "envoy.reloadable_features.wait_for_first_byte_before_balsa_msg_done");
 };
 
 } // namespace Http1
diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc
@@ -1362,6 +1362,10 @@ Status ServerConnectionImpl::sendProtocolError(absl::string_view details) {
   if (active_request_ == nullptr) {
     RETURN_IF_ERROR(onMessageBeginImpl());
   }
+
+  if (resetStreamCalled()) {
+    return okStatus();
+  }
   ASSERT(active_request_);
 
   active_request_->response_encoder_.setDetails(details);
diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h
@@ -542,7 +542,6 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
     return *absl::get<RequestHeaderMapPtr>(headers_or_trailers_);
   }
   void allocHeaders(StatefulHeaderKeyFormatterPtr&& formatter) override {
-    ASSERT(nullptr == absl::get<RequestHeaderMapPtr>(headers_or_trailers_));
     ASSERT(!processing_trailers_);
     auto headers = RequestHeaderMapImpl::create(max_headers_kb_, max_headers_count_);
     headers->setFormatter(std::move(formatter));
diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -107,6 +107,7 @@ RUNTIME_GUARD(envoy_reloadable_features_use_typed_metadata_in_proxy_protocol_lis
 RUNTIME_GUARD(envoy_reloadable_features_validate_connect);
 RUNTIME_GUARD(envoy_reloadable_features_validate_grpc_header_before_log_grpc_status);
 RUNTIME_GUARD(envoy_reloadable_features_validate_upstream_headers);
+RUNTIME_GUARD(envoy_reloadable_features_wait_for_first_byte_before_balsa_msg_done);
 RUNTIME_GUARD(envoy_reloadable_features_xds_failover_to_primary_enabled);
 RUNTIME_GUARD(envoy_reloadable_features_xdstp_path_avoid_colon_encoding);
 RUNTIME_GUARD(envoy_restart_features_allow_client_socket_creation_failure);
diff --git a/source/extensions/clusters/eds/eds.cc b/source/extensions/clusters/eds/eds.cc
@@ -160,9 +160,15 @@ void EdsClusterImpl::BatchUpdateHelper::updateLocalityEndpoints(
   if (!lb_endpoint.endpoint().additional_addresses().empty()) {
     address_list.push_back(address);
     for (const auto& additional_address : lb_endpoint.endpoint().additional_addresses()) {
-      address_list.emplace_back(
-          THROW_OR_RETURN_VALUE(parent_.resolveProtoAddress(additional_address.address()),
-                                const Network::Address::InstanceConstSharedPtr));
+      Network::Address::InstanceConstSharedPtr address =
+          returnOrThrow(parent_.resolveProtoAddress(additional_address.address()));
+      address_list.emplace_back(address);
+    }
+    for (const Network::Address::InstanceConstSharedPtr& address : address_list) {
+      // All addresses must by IP addresses.
+      if (!address->ip()) {
+        throwEnvoyExceptionOrPanic("additional_addresses must be IP addresses.");
+      }
     }
   }
 
diff --git a/source/extensions/clusters/static/static_cluster.cc b/source/extensions/clusters/static/static_cluster.cc
@@ -31,9 +31,15 @@ StaticClusterImpl::StaticClusterImpl(const envoy::config::cluster::v3::Cluster&
             THROW_OR_RETURN_VALUE(resolveProtoAddress(lb_endpoint.endpoint().address()),
                                   const Network::Address::InstanceConstSharedPtr));
         for (const auto& additional_address : lb_endpoint.endpoint().additional_addresses()) {
-          address_list.emplace_back(
-              THROW_OR_RETURN_VALUE(resolveProtoAddress(additional_address.address()),
-                                    const Network::Address::InstanceConstSharedPtr));
+          Network::Address::InstanceConstSharedPtr address =
+              returnOrThrow(resolveProtoAddress(additional_address.address()));
+          address_list.emplace_back(address);
+        }
+        for (const Network::Address::InstanceConstSharedPtr& address : address_list) {
+          // All addresses must by IP addresses.
+          if (!address->ip()) {
+            throwEnvoyExceptionOrPanic("additional_addresses must be IP addresses.");
+          }
         }
       }
       priority_state_manager_->registerHostForPriority(
diff --git a/test/common/crypto/utility_test.cc b/test/common/crypto/utility_test.cc
@@ -146,15 +146,8 @@ TEST(UtilityTest, TestVerifySignature) {
     auto sig = Hex::decode(signature);
 
     auto result = UtilitySingleton::get().verifySignature(hash_func, *crypto, sig, text);
-    if(hash_func == "sha1"){
-      // In RHEL-OpenSSL sha1 is not supported
-      EXPECT_EQ(false, result.result_);
-      EXPECT_EQ("Failed to initialize digest verify.", result.error_message_);
-    } else {
-      EXPECT_EQ(true, result.result_);
-      EXPECT_EQ("", result.error_message_);
-    }
-
+    EXPECT_EQ(true, result.result_);
+    EXPECT_EQ("", result.error_message_);
   }
 
   auto signature =
diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc
@@ -2431,6 +2431,39 @@ TEST_P(Http1ServerConnectionImplTest, LoadShedPointCanCloseConnectionOnDispatchO
   EXPECT_TRUE(isEnvoyOverloadError(status));
 }
 
+TEST_P(Http1ServerConnectionImplTest, LoadShedPointForAlreadyResetStream) {
+  InSequence sequence;
+
+  Server::MockLoadShedPoint mock_abort_dispatch;
+  EXPECT_CALL(overload_manager_, getLoadShedPoint(_)).WillOnce(Return(&mock_abort_dispatch));
+  initialize();
+
+  EXPECT_CALL(mock_abort_dispatch, shouldShedLoad()).WillOnce(Return(false));
+
+  NiceMock<MockRequestDecoder> decoder;
+  Http::ResponseEncoder* response_encoder = nullptr;
+  EXPECT_CALL(callbacks_, newStream(_, _))
+      .WillOnce(Invoke([&](ResponseEncoder& encoder, bool) -> RequestDecoder& {
+        response_encoder = &encoder;
+        return decoder;
+      }));
+
+  Buffer::OwnedImpl request_line_buffer("GET / HTTP/1.1\r\n");
+  auto status = codec_->dispatch(request_line_buffer);
+  EXPECT_EQ(0, request_line_buffer.length());
+
+  EXPECT_CALL(mock_abort_dispatch, shouldShedLoad()).WillRepeatedly(Return(true));
+  Buffer::OwnedImpl headers_buffer("final-header: value\r\n\r\n");
+
+  // The reset stream can be triggered in the middle of dispatching.
+  connection_.dispatcher_.post(
+      [&] { response_encoder->getStream().resetStream(StreamResetReason::LocalReset); });
+
+  status = codec_->dispatch(headers_buffer);
+  EXPECT_FALSE(status.ok());
+  EXPECT_TRUE(isEnvoyOverloadError(status));
+}
+
 TEST_P(Http1ServerConnectionImplTest, LoadShedPointCanCloseConnectionOnDispatchOfContinuingStream) {
   Server::MockLoadShedPoint mock_abort_dispatch;
   EXPECT_CALL(overload_manager_, getLoadShedPoint(_)).WillOnce(Return(&mock_abort_dispatch));
diff --git a/test/common/tls/context_impl_test.cc b/test/common/tls/context_impl_test.cc
@@ -1322,7 +1322,12 @@ TEST_F(ClientContextConfigImplTest, RSA1024Cert) {
   Stats::IsolatedStoreImpl store;
 
   std::string error_msg(
-         "Failed to load certificate chain from .*selfsigned_rsa_1024_cert.pem"
+      "Failed to load certificate chain from .*selfsigned_rsa_1024_cert.pem, only RSA certificates "
+#ifdef BORINGSSL_FIPS
+      "with 2048-bit, 3072-bit or 4096-bit keys are supported in FIPS mode"
+#else
+      "with 2048-bit or larger keys are supported"
+#endif
   );
   EXPECT_THAT(manager_.createSslClientContext(*store.rootScope(), *client_context_config)
                   .status()
@@ -1341,7 +1346,14 @@ TEST_F(ClientContextConfigImplTest, RSA1024Pkcs12) {
                             *tls_context.mutable_common_tls_context()->add_tls_certificates());
   auto client_context_config = *ClientContextConfigImpl::create(tls_context, factory_context_);
   Stats::IsolatedStoreImpl store;
-  std::string error_msg("Failed to load certificate from .*selfsigned_rsa_1024_certkey.p12"
+
+  std::string error_msg("Failed to load certificate chain from .*selfsigned_rsa_1024_certkey.p12, "
+                        "only RSA certificates "
+#ifdef BORINGSSL_FIPS
+                        "with 2048-bit, 3072-bit or 4096-bit keys are supported in FIPS mode"
+#else
+                        "with 2048-bit or larger keys are supported"
+#endif
   );
   EXPECT_THAT(manager_.createSslClientContext(*store.rootScope(), *client_context_config)
                   .status()
diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc
@@ -6540,6 +6540,37 @@ TEST_F(ClusterManagerImplTest, CheckAddressesList) {
   ASSERT_EQ(hosts[0]->addressListOrNull()->size(), 2);
 }
 
+// Verify that non-IP additional addresses are rejected.
+TEST_F(ClusterManagerImplTest, RejectNonIpAdditionalAddresses) {
+  const std::string bootstrap = R"EOF(
+  static_resources:
+    clusters:
+    - name: cluster_0
+      connect_timeout: 0.250s
+      type: STATIC
+      lb_policy: ROUND_ROBIN
+      load_assignment:
+        cluster_name: cluster_0
+        endpoints:
+        - lb_endpoints:
+          - endpoint:
+              additionalAddresses:
+              - address:
+                  envoyInternalAddress:
+                   server_listener_name: internal_address
+              address:
+                socket_address:
+                  address: 127.0.0.1
+                  port_value: 11001
+  )EOF";
+  try {
+    create(parseBootstrapFromV3Yaml(bootstrap));
+    FAIL() << "Invalid address was not rejected";
+  } catch (const EnvoyException& e) {
+    EXPECT_STREQ("additional_addresses must be IP addresses.", e.what());
+  }
+}
+
 TEST_F(ClusterManagerImplTest, CheckActiveStaticCluster) {
   const std::string yaml = R"EOF(
   static_resources:
diff --git a/test/extensions/access_loggers/grpc/tcp_grpc_access_log_integration_test.cc b/test/extensions/access_loggers/grpc/tcp_grpc_access_log_integration_test.cc
@@ -666,17 +666,16 @@ TEST_P(TcpGrpcAccessLogIntegrationTest, SslNotTerminated) {
       downstream_local_address:
         socket_address:
           address: {}
-      tls_properties:
-        tls_sni_hostname: sni
-        local_certificate_properties:
-        peer_certificate_properties: 
       upstream_remote_address:
         socket_address:
       upstream_local_address:
         socket_address:
+      access_log_type: NotSet
       downstream_direct_remote_address:
         socket_address:
           address: {}
+      tls_properties:
+        tls_sni_hostname: sni
     connection_properties:
       received_bytes: 163
       sent_bytes: 163
@@ -721,15 +720,11 @@ TEST_P(TcpGrpcAccessLogIntegrationTest, SslNotTerminatedWithJA3) {
       downstream_local_address:
         socket_address:
           address: {}
-      tls_properties:
-        tls_sni_hostname: "sni"
-        local_certificate_properties:
-        peer_certificate_properties:
-        ja3_fingerprint: "f34cc73a821433e5f56e38868737a636"
       upstream_remote_address:
         socket_address:
       upstream_local_address:
         socket_address:
+      access_log_type: NotSet
       downstream_direct_remote_address:
         socket_address:
           address: {}
@@ -779,10 +774,6 @@ TEST_P(TcpGrpcAccessLogIntegrationTest, SslNotTerminatedWithJA3NoSNI) {
       downstream_local_address:
         socket_address:
           address: {}
-      tls_properties:
-        local_certificate_properties:
-        peer_certificate_properties:
-        ja3_fingerprint: "54619c7296adab310ed514d06812d95f"
       upstream_remote_address:
         socket_address:
       upstream_local_address:
diff --git a/test/extensions/clusters/eds/eds_test.cc b/test/extensions/clusters/eds/eds_test.cc
@@ -436,6 +436,36 @@ TEST_F(EdsTest, DualStackEndpoint) {
   EXPECT_NE(connection, connection_data.connection_.get());
 }
 
+// Verify that non-IP additional addresses are rejected.
+TEST_F(EdsTest, RejectNonIpAdditionalAddresses) {
+  envoy::config::endpoint::v3::ClusterLoadAssignment cluster_load_assignment;
+  cluster_load_assignment.set_cluster_name("fare");
+
+  // Add dual stack endpoint
+  auto* endpoints = cluster_load_assignment.add_endpoints();
+  auto* endpoint = endpoints->add_lb_endpoints();
+  endpoint->mutable_endpoint()->mutable_address()->mutable_socket_address()->set_address("::1");
+  endpoint->mutable_endpoint()->mutable_address()->mutable_socket_address()->set_port_value(80);
+  endpoint->mutable_endpoint()
+      ->mutable_additional_addresses()
+      ->Add()
+      ->mutable_address()
+      ->mutable_envoy_internal_address()
+      ->set_server_listener_name("internal_address");
+
+  endpoint->mutable_load_balancing_weight()->set_value(30);
+
+  initialize();
+  const auto decoded_resources =
+      TestUtility::decodeResources({cluster_load_assignment}, "cluster_name");
+  try {
+    (void)eds_callbacks_->onConfigUpdate(decoded_resources.refvec_, "");
+    FAIL() << "Invalid address was not rejected";
+  } catch (const EnvoyException& e) {
+    EXPECT_STREQ("additional_addresses must be IP addresses.", e.what());
+  }
+}
+
 // Validate that onConfigUpdate() updates the endpoint metadata.
 TEST_F(EdsTest, EndpointMetadata) {
   envoy::config::endpoint::v3::ClusterLoadAssignment cluster_load_assignment;
diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-agent-ubuntu: ubuntu-22.04`
	`1`	`+agent-ubuntu: ubuntu-24.04`
`2`	`2`	`build-image:`
`3`	`3`	`# Authoritative configuration for build image/s`
`4`	`4`	`repo: envoyproxy/envoy-build-ubuntu`