CVE-2021-21378: Improper Authentication (CWE-287)
value: Envoy is a cloud-native high-performance edge/middle/service proxy. In Envoy version 1.17.0 an attacker can bypass authentication by presenting a JWT token with an issuer that is not in the provider list when Envoy's JWT Authentication filter is configured with the `allow_missing` requirement under `requires_any` due to a mistake in implementation. Envoy's JWT Authentication filter can be configured with the `allow_missing` requirement that will be satisfied if JWT is missing (JwtMissed error) and fail if JWT is presented or invalid. Due to a mistake in implementation, a JwtUnknownIssuer error was mistakenly converted to JwtMissed when `requires_any` was configured. So if `allow_missing` was configured under `requires_any`, an attacker can bypass authentication by presenting a JWT token with an issuer that is not in the provider list. Integrity may be impacted depending on configuration if the JWT token is used to protect against writes or modifications. This regression was introduced on 2020/11/12 in PR 13839 which fixed handling `allow_missing` under RequiresAny in a JwtRequirement (see issue 13458). The AnyVerifier aggregates the children verifiers' results into a final status where JwtMissing is the default error. However, a JwtUnknownIssuer was mistakenly treated the same as a JwtMissing error and the resulting final aggregation was the default JwtMissing. As a result, `allow_missing` would allow a JWT token with an unknown issuer status. This is fixed in version 1.17.1 by PR 15194. The fix works by preferring JwtUnknownIssuer over a JwtMissing error, fixing the accidental conversion and bypass with `allow_missing`. A user could detect whether a bypass occurred if they have Envoy logs enabled with debug verbosity. Users can enable component level debug logs for JWT. The JWT filter logs will indicate that there is a request with a JWT token and a failure that the JWT token is missing.
CWE description: When an actor claims to have a given identity, the product does not prove or insufficiently proves that the claim is correct.
CVSS v3 base score: 8.2
Published: 2021-03-11T03:15Z
Repository: https://github.com/envoyproxy/envoy
Language: C++
Files changed: docs/root/version_history/current.rst, source/extensions/filters/http/jwt_authn/verifier.cc, test/extensions/filters/http/jwt_authn/all_verifier_test.cc, test/extensions/filters/http/jwt_authn/group_verifier_test.cc
Commit date: 2021-02-25 19:49:10 -0500
Commit message: jwt_authn: fix a bug where JWT with wrong issuer is allowed in allow_missing case (#15194)

[jwt] When allow_missing is used inside RequiresAny, the requests with JWT with wrong issuer are accepted. This is a bug, allow_missing should only allow requests without any JWT. This change fixed the above issue by preserving JwtUnknownIssuer in allow_missing case.

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Diff: diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
index fe9e813b94..1c08fb66e6 100644
--- a/docs/root/version_history/current.rst
+++ b/docs/root/version_history/current.rst
@@ -56,10 +56,11 @@ Bug Fixes
 * filter_chain: fix filter chain matching with the server name as the case-insensitive way.
 * grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling` to false.
 * grpc_http_bridge: the downstream HTTP status is now correctly set for trailers-only responses from the upstream.
 * http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false.
 * http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false.
+* jwt_authn: reject requests with a proper error if JWT has the wrong issuer when allow_missing is used. Before this change, the requests are accepted.
 * listener: prevent crashing when an unknown listener config proto is received and debug logging is enabled.
 * overload: fix a bug that can cause use-after-free when one scaled timer disables another one with the same duration.
 * sni: as the server name in sni should be case-insensitive, envoy will convert the server name as lower case first before any other process inside envoy.
 * tls: fix the subject alternative name of the presented certificate matches the specified matchers as the case-insensitive way when it uses DNS name.
 * upstream: fix handling of moving endpoints between priorities when active health checks are enabled. Previously moving to a higher numbered priority was a NOOP, and moving to a lower numbered priority caused an abort.
diff --git a/source/extensions/filters/http/jwt_authn/verifier.cc b/source/extensions/filters/http/jwt_authn/verifier.cc
index cb920c1a54..83361c8edd 100644
--- a/source/extensions/filters/http/jwt_authn/verifier.cc
+++ b/source/extensions/filters/http/jwt_authn/verifier.cc
@@ -299,21 +299,20 @@ public:
     }
 
     // Then wait for all children to be done.
     if (++completion_state.number_completed_children_ == verifiers_.size()) {
       // Aggregate all children status into a final status.
-      // JwtMissing should be treated differently than other failure status
-      // since it simply means there is not Jwt token for the required provider.
-      // If there is a failure status other than JwtMissing in the children,
-      // it should be used as the final status.
+      // JwtMissed and JwtUnknownIssuer should be treated differently than other errors.
+      // JwtMissed means not Jwt token for the required provider.
+      // JwtUnknownIssuer means wrong issuer for the required provider.
       Status final_status = Status::JwtMissed;
       for (const auto& it : verifiers_) {
-        // If a Jwt is extracted from a location not specified by the required provider,
-        // the authenticator returns JwtUnknownIssuer. It should be treated the same as
-        // JwtMissed.
+        // Prefer errors which are not JwtMissed nor JwtUnknownIssuer.
+        // Prefer JwtUnknownIssuer between JwtMissed and JwtUnknownIssuer.
         Status child_status = context.getCompletionState(it.get()).status_;
-        if (child_status != Status::JwtMissed && child_status != Status::JwtUnknownIssuer) {
+        if ((child_status != Status::JwtMissed && child_status != Status::JwtUnknownIssuer) ||
+            final_status == Status::JwtMissed) {
           final_status = child_status;
         }
       }
 
       if (is_allow_missing_or_failed_) {
diff --git a/test/extensions/filters/http/jwt_authn/all_verifier_test.cc b/test/extensions/filters/http/jwt_authn/all_verifier_test.cc
index 08cea40b92..ba198bb922 100644
--- a/test/extensions/filters/http/jwt_authn/all_verifier_test.cc
+++ b/test/extensions/filters/http/jwt_authn/all_verifier_test.cc
@@ -195,11 +195,11 @@ TEST_F(SingleAllowMissingInOrListTest, BadJwt) {
   verifier_->verify(context_);
   EXPECT_THAT(headers, JwtOutputFailedOrIgnore(kExampleHeader));
 }
 
 TEST_F(SingleAllowMissingInOrListTest, MissingIssToken) {
-  EXPECT_CALL(mock_cb_, onComplete(Status::Ok));
+  EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer));
   auto headers = Http::TestRequestHeaderMapImpl{{kExampleHeader, ES256WithoutIssToken}};
   context_ = Verifier::createContext(headers, parent_span_, &mock_cb_);
   verifier_->verify(context_);
   EXPECT_THAT(headers, JwtOutputFailedOrIgnore(kExampleHeader));
 }
@@ -469,10 +469,19 @@ TEST_F(AllowMissingInOrListTest, OtherGoodJwt) {
   verifier_->verify(context_);
   // x-other JWT should be ignored.
   EXPECT_THAT(headers, JwtOutputFailedOrIgnore(kOtherHeader));
 }
 
+TEST_F(AllowMissingInOrListTest, WrongIssuer) {
+  EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer));
+  auto headers = Http::TestRequestHeaderMapImpl{{kExampleHeader, OtherGoodToken}};
+  context_ = Verifier::createContext(headers, parent_span_, &mock_cb_);
+  verifier_->verify(context_);
+  // x-other JWT should be ignored.
+  EXPECT_THAT(headers, JwtOutputFailedOrIgnore(kOtherHeader));
+}
+
 TEST_F(AllowMissingInOrListTest, BadAndGoodJwts) {
   EXPECT_CALL(mock_cb_, onComplete(Status::JwtVerificationFail));
   auto headers = Http::TestRequestHeaderMapImpl{{kExampleHeader, NonExistKidToken},
                                                 {kOtherHeader, OtherGoodToken}};
   context_ = Verifier::createContext(headers, parent_span_, &mock_cb_);
@@ -587,11 +596,11 @@ TEST_F(AllowMissingInAndOfOrListTest, TwoGoodJwts) {
   EXPECT_THAT(headers, JwtOutputSuccess(kExampleHeader));
   EXPECT_THAT(headers, JwtOutputSuccess(kOtherHeader));
 }
 
 TEST_F(AllowMissingInAndOfOrListTest, GoodAndBadJwts) {
-  EXPECT_CALL(mock_cb_, onComplete(Status::Ok));
+  EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer));
   // Use the token with example.com issuer for x-other.
   auto headers =
       Http::TestRequestHeaderMapImpl{{kExampleHeader, GoodToken}, {kOtherHeader, GoodToken}};
   context_ = Verifier::createContext(headers, parent_span_, &mock_cb_);
   verifier_->verify(context_);
diff --git a/test/extensions/filters/http/jwt_authn/group_verifier_test.cc b/test/extensions/filters/http/jwt_authn/group_verifier_test.cc
index ab14d6243c..0ccbe3aac7 100644
--- a/test/extensions/filters/http/jwt_authn/group_verifier_test.cc
+++ b/test/extensions/filters/http/jwt_authn/group_verifier_test.cc
@@ -580,21 +580,21 @@ TEST_F(GroupVerifierTest, TestRequiresAnyWithAllowMissingButFailed) {
   verifier_->verify(context_);
   callbacks_["example_provider"](Status::JwtMissed);
   callbacks_["other_provider"](Status::JwtExpired);
 }
 
-// Test RequiresAny with two providers and allow_missing, but OK
-TEST_F(GroupVerifierTest, TestRequiresAnyWithAllowMissingButOk) {
+// Test RequiresAny with two providers and allow_missing, but one returns JwtUnknownIssuer
+TEST_F(GroupVerifierTest, TestRequiresAnyWithAllowMissingButUnknownIssuer) {
   TestUtility::loadFromYaml(RequiresAnyConfig, proto_config_);
   proto_config_.mutable_rules(0)
       ->mutable_requires()
       ->mutable_requires_any()
       ->add_requirements()
       ->mutable_allow_missing();
 
   createAsyncMockAuthsAndVerifier(std::vector<std::string>{"example_provider", "other_provider"});
-  EXPECT_CALL(mock_cb_, onComplete(Status::Ok));
+  EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer));
 
   auto headers = Http::TestRequestHeaderMapImpl{};
   context_ = Verifier::createContext(headers, parent_span_, &mock_cb_);
   verifier_->verify(context_);
   callbacks_["example_provider"](Status::JwtMissed);

Vulnerable code:       // JwtMissing should be treated differently than other failure status
      // since it simply means there is not Jwt token for the required provider.
      // If there is a failure status other than JwtMissing in the children,
      // it should be used as the final status.
        // If a Jwt is extracted from a location not specified by the required provider,
        // the authenticator returns JwtUnknownIssuer. It should be treated the same as
        // JwtMissed.
        if (child_status != Status::JwtMissed && child_status != Status::JwtUnknownIssuer) {
  EXPECT_CALL(mock_cb_, onComplete(Status::Ok));
  EXPECT_CALL(mock_cb_, onComplete(Status::Ok));
// Test RequiresAny with two providers and allow_missing, but OK
TEST_F(GroupVerifierTest, TestRequiresAnyWithAllowMissingButOk) {
  EXPECT_CALL(mock_cb_, onComplete(Status::Ok));
Fixed code: * jwt_authn: reject requests with a proper error if JWT has the wrong issuer when allow_missing is used. Before this change, the requests are accepted.
      // JwtMissed and JwtUnknownIssuer should be treated differently than other errors.
      // JwtMissed means not Jwt token for the required provider.
      // JwtUnknownIssuer means wrong issuer for the required provider.
        // Prefer errors which are not JwtMissed nor JwtUnknownIssuer.
        // Prefer JwtUnknownIssuer between JwtMissed and JwtUnknownIssuer.
        if ((child_status != Status::JwtMissed && child_status != Status::JwtUnknownIssuer) ||
            final_status == Status::JwtMissed) {
  EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer));
TEST_F(AllowMissingInOrListTest, WrongIssuer) {
  EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer));
  auto headers = Http::TestRequestHeaderMapImpl{{kExampleHeader, OtherGoodToken}};
  context_ = Verifier::createContext(headers, parent_span_, &mock_cb_);
  verifier_->verify(context_);
  // x-other JWT should be ignored.
  EXPECT_THAT(headers, JwtOutputFailedOrIgnore(kOtherHeader));
}

  EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer));
// Test RequiresAny with two providers and allow_missing, but one returns JwtUnknownIssuer
TEST_F(GroupVerifierTest, TestRequiresAnyWithAllowMissingButUnknownIssuer) {
  EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer));