From 2c44e459e3a1ad1e71047f9c770f30b6086ef7cf Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Fri, 18 Nov 2022 01:17:09 +0000 Subject: [PATCH] oauth2: add a separator for HMAC calculation Signed-off-by: Lizan Zhou Signed-off-by: Boteng Yao --- changelogs/current.yaml | 5 + .../extensions/filters/http/oauth2/filter.cc | 40 +++--- .../filters/http/oauth2/filter_test.cc | 119 +++++++++++++++++- 3 files changed, 141 insertions(+), 23 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 55ddbd8545df..f0201ae8a694 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -26,6 +26,11 @@ bug_fixes: server scope for stats instead of the listener's global scope. This fixes a use-after-free that can occur if the listener is drained but the cached gRPC access logger uses the listener's global scope for stats. +- area: oauth2 + change: | + Fixed a cookie validator bug that meant the HMAC calculation could be the same for different payloads. + + This prevents malicious clients from constructing credentials with permanent validity in some specific scenarios. removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/extensions/filters/http/oauth2/filter.cc b/source/extensions/filters/http/oauth2/filter.cc index 9cf51b6ae0a7..dc243251e5e7 100644 --- a/source/extensions/filters/http/oauth2/filter.cc +++ b/source/extensions/filters/http/oauth2/filter.cc @@ -68,6 +68,8 @@ constexpr absl::string_view REDIRECT_FOR_CREDENTIALS = "oauth.missing_credential constexpr absl::string_view SIGN_OUT = "oauth.sign_out"; constexpr absl::string_view DEFAULT_AUTH_SCOPE = "user"; +constexpr absl::string_view HmacPayloadSeparator = "\n"; + template std::vector headerMatchers(const T& matcher_protos) { std::vector matchers; @@ -143,6 +145,18 @@ Http::Utility::QueryParams buildAutorizationQueryParams( absl::StrJoin(authScopesList(proto_config.auth_scopes()), " "), ":/=&? "); return query_params; } + +std::string encodeHmac(const std::vector& secret, absl::string_view host, + absl::string_view expires, absl::string_view token = "", + absl::string_view id_token = "", absl::string_view refresh_token = "") { + auto& crypto_util = Envoy::Common::Crypto::UtilitySingleton::get(); + const auto hmac_payload = + absl::StrJoin({host, expires, token, id_token, refresh_token}, HmacPayloadSeparator); + std::string encoded_hmac; + absl::Base64Escape(Hex::encode(crypto_util.getSha256Hmac(secret, hmac_payload)), &encoded_hmac); + return encoded_hmac; +} + } // namespace FilterConfig::FilterConfig( @@ -197,13 +211,7 @@ void OAuth2CookieValidator::setParams(const Http::RequestHeaderMap& headers, } bool OAuth2CookieValidator::hmacIsValid() const { - auto& crypto_util = Envoy::Common::Crypto::UtilitySingleton::get(); - const auto hmac_payload = absl::StrCat(host_, expires_, token_, id_token_, refresh_token_); - const auto pre_encoded_hmac = Hex::encode(crypto_util.getSha256Hmac(secret_, hmac_payload)); - std::string encoded_hmac; - absl::Base64Escape(pre_encoded_hmac, &encoded_hmac); - - return encoded_hmac == hmac_; + return encodeHmac(secret_, host_, expires_, token_, id_token_, refresh_token_) == hmac_; } bool OAuth2CookieValidator::timestampIsValid() const { @@ -458,21 +466,15 @@ void OAuth2Filter::onGetAccessTokenSuccess(const std::string& access_code, } void OAuth2Filter::finishFlow() { - std::string token_payload; - if (config_->forwardBearerToken()) { - token_payload = absl::StrCat(host_, new_expires_, access_token_, id_token_, refresh_token_); - } else { - token_payload = absl::StrCat(host_, new_expires_); - } - - auto& crypto_util = Envoy::Common::Crypto::UtilitySingleton::get(); - auto token_secret = config_->tokenSecret(); std::vector token_secret_vec(token_secret.begin(), token_secret.end()); - const std::string pre_encoded_token = - Hex::encode(crypto_util.getSha256Hmac(token_secret_vec, token_payload)); std::string encoded_token; - absl::Base64Escape(pre_encoded_token, &encoded_token); + if (config_->forwardBearerToken()) { + encoded_token = + encodeHmac(token_secret_vec, host_, new_expires_, access_token_, id_token_, refresh_token_); + } else { + encoded_token = encodeHmac(token_secret_vec, host_, new_expires_); + } // We use HTTP Only cookies for the HMAC and Expiry. const std::string cookie_tail = fmt::format(CookieTailFormatString, new_expires_); diff --git a/test/extensions/filters/http/oauth2/filter_test.cc b/test/extensions/filters/http/oauth2/filter_test.cc index 85e6d056d4b5..5842e5a3aefd 100644 --- a/test/extensions/filters/http/oauth2/filter_test.cc +++ b/test/extensions/filters/http/oauth2/filter_test.cc @@ -162,8 +162,8 @@ class OAuth2Test : public testing::Test { absl::StrCat(cookie_names.bearer_token_, "=xyztoken;version=test")}, {Http::Headers::get().Cookie.get(), absl::StrCat(cookie_names.oauth_hmac_, "=" - "NGQ3MzVjZGExNGM5NTFiZGJjODBkMjBmYjAyYjNiOTFjMmNjYj" - "IxMTUzNmNiNWU0NjQzMmMxMWUzZmE2ZWJjYg==" + "NzQyYmI0YTJkMzFjMmU4Njg2MTdiZGUzYWQzZjkxZjJiMTgwZD" + "I5OWQ2YzhjODBkN2Y4Zjg2OGFmMWFiMWM0Mg==" ";version=test")}, }; @@ -694,6 +694,75 @@ TEST_F(OAuth2Test, CookieValidatorWithCustomNames) { expectValidCookies(CookieNames{"CustomBearerToken", "CustomOauthHMAC", "CustomOauthExpires"}); } +// Validates the behavior of the cookie validator when the combination of some fields could be same. +TEST_F(OAuth2Test, CookieValidatorSame) { + test_time_.setSystemTime(SystemTime(std::chrono::seconds(0))); + auto cookie_names = CookieNames{"BearerToken", "OauthHMAC", "OauthExpires"}; + const auto expires_at_s = DateUtil::nowToSeconds(test_time_.timeSystem()) + 5; + + // Host name is `traffic.example.com:101` and the expire time is 5. + Http::TestRequestHeaderMapImpl request_headers{ + {Http::Headers::get().Host.get(), "traffic.example.com:101"}, + {Http::Headers::get().Path.get(), "/anypath"}, + {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, + {Http::Headers::get().Cookie.get(), + fmt::format("{}={};version=test", cookie_names.oauth_expires_, expires_at_s)}, + {Http::Headers::get().Cookie.get(), + absl::StrCat(cookie_names.bearer_token_, "=xyztoken;version=test")}, + {Http::Headers::get().Cookie.get(), + absl::StrCat(cookie_names.oauth_hmac_, "=" + "MzEyYWJjOWE0MzUwMTlkNWYxZDhiMjg2OTRiMWNjYzEyMjIzZj" + "JiMmQ5NDY3YWM3MTNhMTE2YmVmNGQ0MTcxZA==" + ";version=test")}, + }; + + auto cookie_validator = std::make_shared(test_time_, cookie_names); + EXPECT_EQ(cookie_validator->token(), ""); + cookie_validator->setParams(request_headers, "mock-secret"); + + EXPECT_TRUE(cookie_validator->hmacIsValid()); + EXPECT_TRUE(cookie_validator->timestampIsValid()); + EXPECT_TRUE(cookie_validator->isValid()); + + // If we advance time beyond 5s the timestamp should no longer be valid. + test_time_.advanceTimeWait(std::chrono::seconds(6)); + + EXPECT_FALSE(cookie_validator->timestampIsValid()); + EXPECT_FALSE(cookie_validator->isValid()); + + test_time_.setSystemTime(SystemTime(std::chrono::seconds(0))); + const auto new_expires_at_s = DateUtil::nowToSeconds(test_time_.timeSystem()) + 15; + + // Host name is `traffic.example.com:10` and the expire time is 15. + // HMAC should be different from the above one with the separator fix. + Http::TestRequestHeaderMapImpl request_headers_second{ + {Http::Headers::get().Host.get(), "traffic.example.com:10"}, + {Http::Headers::get().Path.get(), "/anypath"}, + {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, + {Http::Headers::get().Cookie.get(), + fmt::format("{}={};version=test", cookie_names.oauth_expires_, new_expires_at_s)}, + {Http::Headers::get().Cookie.get(), + absl::StrCat(cookie_names.bearer_token_, "=xyztoken;version=test")}, + {Http::Headers::get().Cookie.get(), + absl::StrCat(cookie_names.oauth_hmac_, "=" + "NzViOTc0ZTAyNGFiZTllNTg1ZTc2YzFkMzQzMDkxYjdmNTMwZT" + "gwZTMyZTM1YzFiYTY2YjU0NTkxYzgzZDg1YQ==" + ";version=test")}, + }; + + cookie_validator->setParams(request_headers_second, "mock-secret"); + + EXPECT_TRUE(cookie_validator->hmacIsValid()); + EXPECT_TRUE(cookie_validator->timestampIsValid()); + EXPECT_TRUE(cookie_validator->isValid()); + + // If we advance time beyond 15s the timestamp should no longer be valid. + test_time_.advanceTimeWait(std::chrono::seconds(16)); + + EXPECT_FALSE(cookie_validator->timestampIsValid()); + EXPECT_FALSE(cookie_validator->isValid()); +} + // Validates the behavior of the cookie validator when the expires_at value is not a valid integer. TEST_F(OAuth2Test, CookieValidatorInvalidExpiresAt) { Http::TestRequestHeaderMapImpl request_headers{ @@ -704,7 +773,7 @@ TEST_F(OAuth2Test, CookieValidatorInvalidExpiresAt) { {Http::Headers::get().Cookie.get(), "BearerToken=xyztoken;version=test"}, {Http::Headers::get().Cookie.get(), "OauthHMAC=" - "M2NjZmIxYWE0NzQzOGZlZTJjMjQwMzBiZTU5OTdkN2Y0NDRhZjE5MjZiOWNhY2YzNjM0MWRmMTNkMDVmZWFlOQ==" + "NzNlZDZhY2YyYWNjOWFhMWJjZjhlZTFkOWZiNmY2ZjBlYmNkMzQzNTljNmY0ZTMyMjVmMzViNjQyMTM1Y2Q4MQ==" ";version=test"}, }; @@ -931,7 +1000,7 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParameters) { {Http::Headers::get().Status.get(), "302"}, {Http::Headers::get().SetCookie.get(), "OauthHMAC=" - "NWUzNzE5MWQwYTg0ZjA2NjIyMjVjMzk3MzY3MzMyZmE0NjZmMWI2MjI1NWFhNDhkYjQ4NDFlZmRiMTVmMTk0MQ==;" + "N2Q1ZWI2M2EwMmUyYTQyODUzNDEwMGI3NTA1ODAzYTdlOTc5YjAyODkyNmY3Y2VkZWU3MGE4MjYyNTYyYmQ2Yw==;" "version=1;path=/;Max-Age=;secure;HttpOnly"}, {Http::Headers::get().SetCookie.get(), "OauthExpires=;version=1;path=/;Max-Age=;secure;HttpOnly"}, @@ -946,6 +1015,48 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParameters) { filter_->finishFlow(); } +/** + * Testing oauth response after tokens are set. + * + * Expected behavior: cookies are set. + */ +TEST_F(OAuth2Test, OAuthAccessTokenSucessWithTokens) { + + // Set SystemTime to a fixed point so we get consistent HMAC encodings between test runs. + test_time_.setSystemTime(SystemTime(std::chrono::seconds(1000))); + + // host_ must be set, which is guaranteed (ASAN). + Http::TestRequestHeaderMapImpl request_headers{ + {Http::Headers::get().Host.get(), "traffic.example.com"}, + {Http::Headers::get().Path.get(), "/_signout"}, + {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, + }; + filter_->decodeHeaders(request_headers, false); + + // Expected response after the callback is complete. + Http::TestRequestHeaderMapImpl expected_headers{ + {Http::Headers::get().Status.get(), "302"}, + {Http::Headers::get().SetCookie.get(), + "OauthHMAC=" + "ZTEzMmIyYzRmNTdmMTdiY2IyYmViZDE3ODA5ZDliOTE2MTRlNzNjYjc4MjBlMTVlOWY1OTM2ZjViZjM4MzAwNA==;" + "version=1;path=/;Max-Age=1600;secure;HttpOnly"}, + {Http::Headers::get().SetCookie.get(), + "OauthExpires=1600;version=1;path=/;Max-Age=1600;secure;HttpOnly"}, + {Http::Headers::get().SetCookie.get(), + "BearerToken=access_code;version=1;path=/;Max-Age=1600;secure"}, + {Http::Headers::get().SetCookie.get(), + "IdToken=some-id-token;version=1;path=/;Max-Age=1600;secure"}, + {Http::Headers::get().SetCookie.get(), + "RefreshToken=some-refresh-token;version=1;path=/;Max-Age=1600;secure"}, + {Http::Headers::get().Location.get(), ""}, + }; + + EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&expected_headers), true)); + + filter_->onGetAccessTokenSuccess("access_code", "some-id-token", "some-refresh-token", + std::chrono::seconds(600)); +} + TEST_F(OAuth2Test, OAuthBearerTokenFlowFromHeader) { Http::TestRequestHeaderMapImpl request_headers{ {Http::Headers::get().Path.get(), "/test?role=bearer"},