Cherry-pick JWT CVE fix into 1.4 (istio#12)

Neeraj Poddar · istio-testing · commit 859552a762ef · 2020-01-23T14:58:54.000-08:00
* Fixed JWT CVE related to exact PATH matches (istio#9) * Fixed JWT CVE related to exact PATH matches Problem: The JWT filter when matching exact paths included query parameters which meant the JWT requirement could be bypassed by adding a "?" after the path. The API was intended to only work for URIs. Solution: The fix updates the match logic to only include URIs i.e. path stripped off the query section. Added unit tests to validate these cases. * Fixed formatting * Strip fragment of Path Added unit tests to validate combination of query & fragment * Fix lint * Minor refactoring and more unit test cases (istio#11) * Minor refactoring and more unit test cases * Lint fixes
diff --git a/src/envoy/http/authn/origin_authenticator.cc b/src/envoy/http/authn/origin_authenticator.cc
@@ -18,6 +18,7 @@
 #include "absl/strings/match.h"
 #include "authentication/v1alpha1/policy.pb.h"
 #include "common/http/headers.h"
+#include "common/http/utility.h"
 #include "src/envoy/http/authn/authn_utils.h"
 
 using istio::authn::Payload;
@@ -64,11 +65,16 @@ bool OriginAuthenticator::run(Payload* payload) {
     return true;
   }
 
-  absl::string_view request_path;
+  absl::string_view path;
   if (filter_context()->headerMap().Path() != nullptr) {
-    request_path =
-        filter_context()->headerMap().Path()->value().getStringView();
-    ENVOY_LOG(debug, "Got request path {}", request_path);
+    path = filter_context()->headerMap().Path()->value().getStringView();
+
+    // Trim query parameters and/or fragment if present
+    size_t offset = path.find_first_of("?#");
+    if (offset != absl::string_view::npos) {
+      path.remove_suffix(path.length() - offset);
+    }
+    ENVOY_LOG(trace, "Got request path {}", path);
   } else {
     ENVOY_LOG(error,
               "Failed to get request path, JWT will always be used for "
@@ -80,8 +86,8 @@ bool OriginAuthenticator::run(Payload* payload) {
   for (const auto& method : policy_.origins()) {
     const auto& jwt = method.jwt();
 
-    if (AuthnUtils::ShouldValidateJwtPerPath(request_path, jwt)) {
-      ENVOY_LOG(debug, "Validating request path {} for jwt {}", request_path,
+    if (AuthnUtils::ShouldValidateJwtPerPath(path, jwt)) {
+      ENVOY_LOG(debug, "Validating request path {} for jwt {}", path,
                 jwt.DebugString());
       // set triggered to true if any of the jwt trigger rule matched.
       triggered = true;
diff --git a/src/envoy/http/authn/origin_authenticator_test.cc b/src/envoy/http/authn/origin_authenticator_test.cc
@@ -101,6 +101,20 @@ const char kSingleOriginMethodWithTriggerRulePolicy[] = R"(
   }
 )";
 
+const char kSingleOriginMethodWithExcludeTriggerRulePolicy[] = R"(
+  principal_binding: USE_ORIGIN
+  origins {
+    jwt {
+      issuer: "abc.xyz"
+      trigger_rules: {
+        excluded_paths: {
+          exact: "/login"
+        }
+      }
+    }
+  }
+)";
+
 const char kMultipleOriginMethodWithTriggerRulePolicy[] = R"(
   principal_binding: USE_ORIGIN
   origins {
@@ -327,15 +341,60 @@ TEST_P(OriginAuthenticatorTest, SingleRuleTriggered) {
                                       filter_context_.authenticationResult()));
 }
 
+TEST_P(OriginAuthenticatorTest, SingleRuleTriggeredWithComponents) {
+  const std::vector<std::string> input_paths{"/allow?",
+                                             "/allow?a=b&c=d",
+                                             "/allow??",
+                                             "/allow??",
+                                             "/allow?#",
+                                             "/allow#?",
+                                             "/allow#a",
+                                             "/allow#$"
+                                             "/allow?a=b#c",
+                                             "/allow#a?b=c"};
+  for (const auto& path : input_paths) {
+    ASSERT_TRUE(Protobuf::TextFormat::ParseFromString(
+        kSingleOriginMethodWithTriggerRulePolicy, &policy_));
+
+    createAuthenticator();
+
+    EXPECT_CALL(*authenticator_, validateJwt(_, _))
+        .Times(1)
+        .WillOnce(DoAll(SetArgPointee<1>(jwt_payload_), Return(true)));
+
+    setPath(path);
+    EXPECT_TRUE(authenticator_->run(payload_));
+    EXPECT_TRUE(TestUtility::protoEqual(
+        expected_result_when_pass_, filter_context_.authenticationResult()));
+  }
+}
+
 TEST_P(OriginAuthenticatorTest, SingleRuleNotTriggered) {
+  const std::vector<std::string> input_paths{"/bad", "/allow$?", "/allow$#"};
+  for (const auto& path : input_paths) {
+    ASSERT_TRUE(Protobuf::TextFormat::ParseFromString(
+        kSingleOriginMethodWithTriggerRulePolicy, &policy_));
+
+    createAuthenticator();
+
+    EXPECT_CALL(*authenticator_, validateJwt(_, _)).Times(0);
+
+    setPath(path);
+    EXPECT_TRUE(authenticator_->run(payload_));
+    EXPECT_TRUE(TestUtility::protoEqual(
+        initial_result_, filter_context_.authenticationResult()));
+  }
+}
+
+TEST_P(OriginAuthenticatorTest, SingleExcludeRuleTriggeredWithQueryParam) {
   ASSERT_TRUE(Protobuf::TextFormat::ParseFromString(
-      kSingleOriginMethodWithTriggerRulePolicy, &policy_));
+      kSingleOriginMethodWithExcludeTriggerRulePolicy, &policy_));
 
   createAuthenticator();
 
   EXPECT_CALL(*authenticator_, validateJwt(_, _)).Times(0);
 
-  setPath("/bad");
+  setPath("/login?a=b&c=d");
   EXPECT_TRUE(authenticator_->run(payload_));
   EXPECT_TRUE(TestUtility::protoEqual(initial_result_,
                                       filter_context_.authenticationResult()));
