From d3982bdaf5a168c8a6d615e75a1be32263701d2b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 12 Aug 2025 17:02:23 +0000 Subject: [PATCH 1/3] Initial plan From 96e1a74901e60a4b43d7659aed964c00524f56f8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 12 Aug 2025 17:14:05 +0000 Subject: [PATCH 2/3] Fix security issue with malicious issuer handling in error messages - Add safe JSON serialization and size limiting for issuer in error messages - Implement format_issuer_for_error() helper method that safely serializes issuer claims - Limit issuer length to 256 characters in error messages to prevent abuse - Add comprehensive tests for malicious issuer handling including edge cases - All existing tests continue to pass Co-authored-by: djw8605 <79268+djw8605@users.noreply.github.com> --- src/scitokens_internal.h | 34 ++++++++++++- test/main.cpp | 100 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 2 deletions(-) diff --git a/src/scitokens_internal.h b/src/scitokens_internal.h index fe0600e..f1ca361 100644 --- a/src/scitokens_internal.h +++ b/src/scitokens_internal.h @@ -487,9 +487,10 @@ class Validator { } } if (!permitted) { + std::string safe_issuer = format_issuer_for_error(jwt); throw JWTVerificationException( - "Token issuer '" + issuer + - "' is not in list of allowed issuers."); + "Token issuer " + safe_issuer + + " is not in list of allowed issuers."); } } @@ -774,6 +775,35 @@ class Validator { const picojson::value &keys, int64_t next_update, int64_t expires); + /** + * Safely format an issuer for error messages. + * Serializes the issuer claim back to JSON format and limits the size + * to prevent malicious issuers from causing problems in error output. + */ + static std::string format_issuer_for_error( + const jwt::decoded_jwt &jwt) { + try { + if (!jwt.has_payload_claim("iss")) { + return ""; + } + + // Get the raw claim and serialize it back to JSON + const auto &claim = jwt.get_payload_claim("iss"); + std::string serialized = claim.to_json().serialize(); + + // Limit the size to prevent abuse + const size_t max_issuer_length = 256; + if (serialized.length() > max_issuer_length) { + serialized = serialized.substr(0, max_issuer_length - 3) + "..."; + } + + return serialized; + } catch (...) { + // If anything goes wrong, return a safe fallback + return ""; + } + } + bool m_validate_all_claims{true}; SciToken::Profile m_profile{SciToken::Profile::COMPAT}; SciToken::Profile m_validate_profile{SciToken::Profile::COMPAT}; diff --git a/test/main.cpp b/test/main.cpp index 6d786c4..470b4b5 100644 --- a/test/main.cpp +++ b/test/main.cpp @@ -806,6 +806,106 @@ TEST_F(KeycacheTest, RefreshExpiredTest) { EXPECT_EQ(jwks_str, "{\"keys\": []}"); } +class IssuerSecurityTest : public ::testing::Test { + protected: + void SetUp() override { + char *err_msg = nullptr; + m_key = KeyPtr( + scitoken_key_create("1", "ES256", ec_public, ec_private, &err_msg), + scitoken_key_destroy); + ASSERT_TRUE(m_key.get() != nullptr) << err_msg; + + m_token = TokenPtr(scitoken_create(m_key.get()), scitoken_destroy); + ASSERT_TRUE(m_token.get() != nullptr); + + // Store public key for verification + auto rv = scitoken_store_public_ec_key("https://demo.scitokens.org/gtest", + "1", ec_public, &err_msg); + ASSERT_TRUE(rv == 0) << err_msg; + + scitoken_set_lifetime(m_token.get(), 60); + + m_read_token.reset(scitoken_create(nullptr)); + ASSERT_TRUE(m_read_token.get() != nullptr); + } + + using KeyPtr = std::unique_ptr; + KeyPtr m_key{nullptr, scitoken_key_destroy}; + + using TokenPtr = std::unique_ptr; + TokenPtr m_token{nullptr, scitoken_destroy}; + + TokenPtr m_read_token{nullptr, scitoken_destroy}; +}; + +TEST_F(IssuerSecurityTest, LongIssuerTruncation) { + char *err_msg = nullptr; + + // Create a very long issuer (1000 characters) + std::string very_long_issuer(1000, 'A'); + auto rv = scitoken_set_claim_string(m_token.get(), "iss", very_long_issuer.c_str(), &err_msg); + ASSERT_TRUE(rv == 0) << err_msg; + + char *token_value = nullptr; + rv = scitoken_serialize(m_token.get(), &token_value, &err_msg); + ASSERT_TRUE(rv == 0) << err_msg; + std::unique_ptr token_value_ptr(token_value, free); + + // Try to verify with a restricted issuer list to trigger error + const char* allowed_issuers[] = {"https://good.issuer.com", nullptr}; + rv = scitoken_deserialize_v2(token_value, m_read_token.get(), allowed_issuers, &err_msg); + + // Should fail + ASSERT_FALSE(rv == 0); + ASSERT_TRUE(err_msg != nullptr); + + std::string error_message(err_msg); + std::unique_ptr err_msg_ptr(err_msg, free); + + // Error message should be reasonable length (< 400 chars) + EXPECT_LT(error_message.length(), 400); + + // Should contain expected error text + EXPECT_NE(error_message.find("is not in list of allowed issuers"), std::string::npos); + + // Should contain truncated issuer with ellipsis + EXPECT_NE(error_message.find("..."), std::string::npos); +} + +TEST_F(IssuerSecurityTest, SpecialCharacterIssuer) { + char *err_msg = nullptr; + + // Create an issuer with special characters and control chars + std::string special_issuer = "https://bad.com/\"\n\t\r\x01\x1f"; + auto rv = scitoken_set_claim_string(m_token.get(), "iss", special_issuer.c_str(), &err_msg); + ASSERT_TRUE(rv == 0) << err_msg; + + char *token_value = nullptr; + rv = scitoken_serialize(m_token.get(), &token_value, &err_msg); + ASSERT_TRUE(rv == 0) << err_msg; + std::unique_ptr token_value_ptr(token_value, free); + + // Try to verify with a restricted issuer list to trigger error + const char* allowed_issuers[] = {"https://good.issuer.com", nullptr}; + rv = scitoken_deserialize_v2(token_value, m_read_token.get(), allowed_issuers, &err_msg); + + // Should fail + ASSERT_FALSE(rv == 0); + ASSERT_TRUE(err_msg != nullptr); + + std::string error_message(err_msg); + std::unique_ptr err_msg_ptr(err_msg, free); + + // Error message should be reasonable length + EXPECT_LT(error_message.length(), 300); + + // Should contain expected error text + EXPECT_NE(error_message.find("is not in list of allowed issuers"), std::string::npos); + + // Should contain properly escaped JSON (with quotes) + EXPECT_NE(error_message.find("\""), std::string::npos); +} + int main(int argc, char **argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); From f8d0fb3076d141e6f7a89819f7f95e7398b4de80 Mon Sep 17 00:00:00 2001 From: Derek Weitzel Date: Wed, 13 Aug 2025 11:31:33 -0500 Subject: [PATCH 3/3] Apply suggestions from linter Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- src/scitokens_internal.h | 6 ++---- test/main.cpp | 40 ++++++++++++++++++++-------------------- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/src/scitokens_internal.h b/src/scitokens_internal.h index f1ca361..c947b46 100644 --- a/src/scitokens_internal.h +++ b/src/scitokens_internal.h @@ -786,17 +786,15 @@ class Validator { if (!jwt.has_payload_claim("iss")) { return ""; } - // Get the raw claim and serialize it back to JSON const auto &claim = jwt.get_payload_claim("iss"); std::string serialized = claim.to_json().serialize(); - // Limit the size to prevent abuse const size_t max_issuer_length = 256; if (serialized.length() > max_issuer_length) { - serialized = serialized.substr(0, max_issuer_length - 3) + "..."; + serialized = + serialized.substr(0, max_issuer_length - 3) + "..."; } - return serialized; } catch (...) { // If anything goes wrong, return a safe fallback diff --git a/test/main.cpp b/test/main.cpp index 470b4b5..70abc51 100644 --- a/test/main.cpp +++ b/test/main.cpp @@ -819,8 +819,8 @@ class IssuerSecurityTest : public ::testing::Test { ASSERT_TRUE(m_token.get() != nullptr); // Store public key for verification - auto rv = scitoken_store_public_ec_key("https://demo.scitokens.org/gtest", - "1", ec_public, &err_msg); + auto rv = scitoken_store_public_ec_key( + "https://demo.scitokens.org/gtest", "1", ec_public, &err_msg); ASSERT_TRUE(rv == 0) << err_msg; scitoken_set_lifetime(m_token.get(), 60); @@ -843,7 +843,8 @@ TEST_F(IssuerSecurityTest, LongIssuerTruncation) { // Create a very long issuer (1000 characters) std::string very_long_issuer(1000, 'A'); - auto rv = scitoken_set_claim_string(m_token.get(), "iss", very_long_issuer.c_str(), &err_msg); + auto rv = scitoken_set_claim_string(m_token.get(), "iss", + very_long_issuer.c_str(), &err_msg); ASSERT_TRUE(rv == 0) << err_msg; char *token_value = nullptr; @@ -852,22 +853,21 @@ TEST_F(IssuerSecurityTest, LongIssuerTruncation) { std::unique_ptr token_value_ptr(token_value, free); // Try to verify with a restricted issuer list to trigger error - const char* allowed_issuers[] = {"https://good.issuer.com", nullptr}; - rv = scitoken_deserialize_v2(token_value, m_read_token.get(), allowed_issuers, &err_msg); - + const char *allowed_issuers[] = {"https://good.issuer.com", nullptr}; + rv = scitoken_deserialize_v2(token_value, m_read_token.get(), + allowed_issuers, &err_msg); + // Should fail ASSERT_FALSE(rv == 0); ASSERT_TRUE(err_msg != nullptr); - std::string error_message(err_msg); std::unique_ptr err_msg_ptr(err_msg, free); - // Error message should be reasonable length (< 400 chars) EXPECT_LT(error_message.length(), 400); - // Should contain expected error text - EXPECT_NE(error_message.find("is not in list of allowed issuers"), std::string::npos); - + EXPECT_NE(error_message.find("is not in list of allowed issuers"), + std::string::npos); + // Should contain truncated issuer with ellipsis EXPECT_NE(error_message.find("..."), std::string::npos); } @@ -877,7 +877,8 @@ TEST_F(IssuerSecurityTest, SpecialCharacterIssuer) { // Create an issuer with special characters and control chars std::string special_issuer = "https://bad.com/\"\n\t\r\x01\x1f"; - auto rv = scitoken_set_claim_string(m_token.get(), "iss", special_issuer.c_str(), &err_msg); + auto rv = scitoken_set_claim_string(m_token.get(), "iss", + special_issuer.c_str(), &err_msg); ASSERT_TRUE(rv == 0) << err_msg; char *token_value = nullptr; @@ -886,22 +887,21 @@ TEST_F(IssuerSecurityTest, SpecialCharacterIssuer) { std::unique_ptr token_value_ptr(token_value, free); // Try to verify with a restricted issuer list to trigger error - const char* allowed_issuers[] = {"https://good.issuer.com", nullptr}; - rv = scitoken_deserialize_v2(token_value, m_read_token.get(), allowed_issuers, &err_msg); - + const char *allowed_issuers[] = {"https://good.issuer.com", nullptr}; + rv = scitoken_deserialize_v2(token_value, m_read_token.get(), + allowed_issuers, &err_msg); + // Should fail ASSERT_FALSE(rv == 0); ASSERT_TRUE(err_msg != nullptr); - std::string error_message(err_msg); std::unique_ptr err_msg_ptr(err_msg, free); - // Error message should be reasonable length EXPECT_LT(error_message.length(), 300); - // Should contain expected error text - EXPECT_NE(error_message.find("is not in list of allowed issuers"), std::string::npos); - + EXPECT_NE(error_message.find("is not in list of allowed issuers"), + std::string::npos); + // Should contain properly escaped JSON (with quotes) EXPECT_NE(error_message.find("\""), std::string::npos); }