Skip to content

Commit

Permalink
oauth2: add a separator for HMAC calculation
Browse files Browse the repository at this point in the history 
Signed-off-by: Lizan Zhou <[email protected]>
Signed-off-by: Boteng Yao <[email protected]>
  • Loading branch information
@lizan @phlax
lizan authored and phlax committed Jul 25, 2023
1 parent 496a5a7 commit a4dbd74
Show file tree
Hide file tree
Showing 3 changed files with 128 additions and 27 deletions.
5 changes: 5 additions & 0 deletions changelogs/current.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,11 @@ bug_fixes:
change: |
Fix a use-after-free bug that occurs in the CORS filter if the ``origin`` header is removed between
request header decoding and response header encoding.
- 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 <deprecated>`
Expand Down
40 changes: 21 additions & 19 deletions source/extensions/filters/http/oauth2/filter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,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 <class T>
std::vector<Http::HeaderUtility::HeaderData> headerMatchers(const T& matcher_protos) {
std::vector<Http::HeaderUtility::HeaderData> matchers;
Expand Down Expand Up @@ -136,6 +138,18 @@ Http::Utility::QueryParams buildAutorizationQueryParams(
: Http::Utility::PercentEncoding::encode(scopes_list, ":/=&? ");
return query_params;
}

std::string encodeHmac(const std::vector<uint8_t>& 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(
Expand Down Expand Up @@ -191,13 +205,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 {
Expand Down Expand Up @@ -474,21 +482,15 @@ void OAuth2Filter::updateTokens(const std::string& access_token, const std::stri
}

std::string OAuth2Filter::getEncodedToken() const {
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<uint8_t> 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_);
}
return encoded_token;
}

Expand Down
110 changes: 102 additions & 8 deletions test/extensions/filters/http/oauth2/filter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,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")},
};

Expand Down Expand Up @@ -796,6 +796,76 @@ TEST_F(OAuth2Test, CookieValidatorWithCustomNames) {
"CustomIdToken", "CustomRefreshToken"});
}

// 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", "IdToken", "RefreshToken"};
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<OAuth2CookieValidator>(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{
Expand All @@ -806,7 +876,7 @@ TEST_F(OAuth2Test, CookieValidatorInvalidExpiresAt) {
{Http::Headers::get().Cookie.get(), "BearerToken=xyztoken;version=test"},
{Http::Headers::get().Cookie.get(),
"OauthHMAC="
"M2NjZmIxYWE0NzQzOGZlZTJjMjQwMzBiZTU5OTdkN2Y0NDRhZjE5MjZiOWNhY2YzNjM0MWRmMTNkMDVmZWFlOQ=="
"NzNlZDZhY2YyYWNjOWFhMWJjZjhlZTFkOWZiNmY2ZjBlYmNkMzQzNTljNmY0ZTMyMjVmMzViNjQyMTM1Y2Q4MQ=="
";version=test"},
};

Expand Down Expand Up @@ -1102,7 +1172,7 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParametersLegacyEncoding) {
{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"},
Expand Down Expand Up @@ -1194,7 +1264,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"},
Expand All @@ -1220,12 +1290,20 @@ 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="
"MjI2YmI5YTRiZjJlNTFlNDUzZWVjOWUzYmU1MThlNGQyNDgyNzA0ZTBkMGQyY2M3M2QyMzg3NTRkZTY0YmU5YQ==;"
"ZTEzMmIyYzRmNTdmMTdiY2IyYmViZDE3ODA5ZDliOTE2MTRlNzNjYjc4MjBlMTVlOWY1OTM2ZjViZjM4MzAwNA==;"
"version=1;path=/;Max-Age=600;secure;HttpOnly"},
{Http::Headers::get().SetCookie.get(),
"OauthExpires=1600;version=1;path=/;Max-Age=600;secure;HttpOnly"},
Expand Down Expand Up @@ -1253,12 +1331,20 @@ TEST_F(OAuth2Test, OAuthAccessTokenSucessWithTokens_oauth_use_standard_max_age_v
// Set SystemTime to a fixed point so we get consistent HMAC encodings between test runs.
test_time_.setSystemTime(SystemTime(std::chrono::seconds(0)));

// 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="
"OTBhMzEwNjk4YzJiNjIxMTcwMTE0ZDE2NjUyNjIyNmI1YmE0Y2NhNTQ3ZWYwZGYzZTNhYjEwNzhmZmQyZGY4Zg==;"
"ZmMzNzFkOWVkY2ZmNzc3M2NjYjk0ZTA0NDM4YTlkOWIxMTUxNmI3NDkyMGRkYjM1Mzg4YTBiMzc4NGRmOWU4Mw==;"
"version=1;path=/;Max-Age=600;secure;HttpOnly"},
{Http::Headers::get().SetCookie.get(),
"OauthExpires=600;version=1;path=/;Max-Age=600;secure;HttpOnly"},
Expand All @@ -1285,12 +1371,20 @@ TEST_F(OAuth2Test, OAuthAccessTokenSucessWithTokens_oauth_make_token_cookie_http
// 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="
"MjI2YmI5YTRiZjJlNTFlNDUzZWVjOWUzYmU1MThlNGQyNDgyNzA0ZTBkMGQyY2M3M2QyMzg3NTRkZTY0YmU5YQ==;"
"ZTEzMmIyYzRmNTdmMTdiY2IyYmViZDE3ODA5ZDliOTE2MTRlNzNjYjc4MjBlMTVlOWY1OTM2ZjViZjM4MzAwNA==;"
"version=1;path=/;Max-Age=600;secure;HttpOnly"},
{Http::Headers::get().SetCookie.get(),
"OauthExpires=1600;version=1;path=/;Max-Age=600;secure;HttpOnly"},
Expand Down

0 comments on commit a4dbd74

Please sign in to comment.