Skip to content

Commit 96e1a74

Browse files
Copilotdjw8605
andcommitted
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>
1 parent d3982bd commit 96e1a74

File tree

2 files changed

+132
-2
lines changed

2 files changed

+132
-2
lines changed

src/scitokens_internal.h

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -487,9 +487,10 @@ class Validator {
487487
}
488488
}
489489
if (!permitted) {
490+
std::string safe_issuer = format_issuer_for_error(jwt);
490491
throw JWTVerificationException(
491-
"Token issuer '" + issuer +
492-
"' is not in list of allowed issuers.");
492+
"Token issuer " + safe_issuer +
493+
" is not in list of allowed issuers.");
493494
}
494495
}
495496

@@ -774,6 +775,35 @@ class Validator {
774775
const picojson::value &keys,
775776
int64_t next_update, int64_t expires);
776777

778+
/**
779+
* Safely format an issuer for error messages.
780+
* Serializes the issuer claim back to JSON format and limits the size
781+
* to prevent malicious issuers from causing problems in error output.
782+
*/
783+
static std::string format_issuer_for_error(
784+
const jwt::decoded_jwt<jwt::traits::kazuho_picojson> &jwt) {
785+
try {
786+
if (!jwt.has_payload_claim("iss")) {
787+
return "<missing issuer>";
788+
}
789+
790+
// Get the raw claim and serialize it back to JSON
791+
const auto &claim = jwt.get_payload_claim("iss");
792+
std::string serialized = claim.to_json().serialize();
793+
794+
// Limit the size to prevent abuse
795+
const size_t max_issuer_length = 256;
796+
if (serialized.length() > max_issuer_length) {
797+
serialized = serialized.substr(0, max_issuer_length - 3) + "...";
798+
}
799+
800+
return serialized;
801+
} catch (...) {
802+
// If anything goes wrong, return a safe fallback
803+
return "<invalid issuer>";
804+
}
805+
}
806+
777807
bool m_validate_all_claims{true};
778808
SciToken::Profile m_profile{SciToken::Profile::COMPAT};
779809
SciToken::Profile m_validate_profile{SciToken::Profile::COMPAT};

test/main.cpp

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -806,6 +806,106 @@ TEST_F(KeycacheTest, RefreshExpiredTest) {
806806
EXPECT_EQ(jwks_str, "{\"keys\": []}");
807807
}
808808

809+
class IssuerSecurityTest : public ::testing::Test {
810+
protected:
811+
void SetUp() override {
812+
char *err_msg = nullptr;
813+
m_key = KeyPtr(
814+
scitoken_key_create("1", "ES256", ec_public, ec_private, &err_msg),
815+
scitoken_key_destroy);
816+
ASSERT_TRUE(m_key.get() != nullptr) << err_msg;
817+
818+
m_token = TokenPtr(scitoken_create(m_key.get()), scitoken_destroy);
819+
ASSERT_TRUE(m_token.get() != nullptr);
820+
821+
// Store public key for verification
822+
auto rv = scitoken_store_public_ec_key("https://demo.scitokens.org/gtest",
823+
"1", ec_public, &err_msg);
824+
ASSERT_TRUE(rv == 0) << err_msg;
825+
826+
scitoken_set_lifetime(m_token.get(), 60);
827+
828+
m_read_token.reset(scitoken_create(nullptr));
829+
ASSERT_TRUE(m_read_token.get() != nullptr);
830+
}
831+
832+
using KeyPtr = std::unique_ptr<void, decltype(&scitoken_key_destroy)>;
833+
KeyPtr m_key{nullptr, scitoken_key_destroy};
834+
835+
using TokenPtr = std::unique_ptr<void, decltype(&scitoken_destroy)>;
836+
TokenPtr m_token{nullptr, scitoken_destroy};
837+
838+
TokenPtr m_read_token{nullptr, scitoken_destroy};
839+
};
840+
841+
TEST_F(IssuerSecurityTest, LongIssuerTruncation) {
842+
char *err_msg = nullptr;
843+
844+
// Create a very long issuer (1000 characters)
845+
std::string very_long_issuer(1000, 'A');
846+
auto rv = scitoken_set_claim_string(m_token.get(), "iss", very_long_issuer.c_str(), &err_msg);
847+
ASSERT_TRUE(rv == 0) << err_msg;
848+
849+
char *token_value = nullptr;
850+
rv = scitoken_serialize(m_token.get(), &token_value, &err_msg);
851+
ASSERT_TRUE(rv == 0) << err_msg;
852+
std::unique_ptr<char, decltype(&free)> token_value_ptr(token_value, free);
853+
854+
// Try to verify with a restricted issuer list to trigger error
855+
const char* allowed_issuers[] = {"https://good.issuer.com", nullptr};
856+
rv = scitoken_deserialize_v2(token_value, m_read_token.get(), allowed_issuers, &err_msg);
857+
858+
// Should fail
859+
ASSERT_FALSE(rv == 0);
860+
ASSERT_TRUE(err_msg != nullptr);
861+
862+
std::string error_message(err_msg);
863+
std::unique_ptr<char, decltype(&free)> err_msg_ptr(err_msg, free);
864+
865+
// Error message should be reasonable length (< 400 chars)
866+
EXPECT_LT(error_message.length(), 400);
867+
868+
// Should contain expected error text
869+
EXPECT_NE(error_message.find("is not in list of allowed issuers"), std::string::npos);
870+
871+
// Should contain truncated issuer with ellipsis
872+
EXPECT_NE(error_message.find("..."), std::string::npos);
873+
}
874+
875+
TEST_F(IssuerSecurityTest, SpecialCharacterIssuer) {
876+
char *err_msg = nullptr;
877+
878+
// Create an issuer with special characters and control chars
879+
std::string special_issuer = "https://bad.com/\"\n\t\r\x01\x1f";
880+
auto rv = scitoken_set_claim_string(m_token.get(), "iss", special_issuer.c_str(), &err_msg);
881+
ASSERT_TRUE(rv == 0) << err_msg;
882+
883+
char *token_value = nullptr;
884+
rv = scitoken_serialize(m_token.get(), &token_value, &err_msg);
885+
ASSERT_TRUE(rv == 0) << err_msg;
886+
std::unique_ptr<char, decltype(&free)> token_value_ptr(token_value, free);
887+
888+
// Try to verify with a restricted issuer list to trigger error
889+
const char* allowed_issuers[] = {"https://good.issuer.com", nullptr};
890+
rv = scitoken_deserialize_v2(token_value, m_read_token.get(), allowed_issuers, &err_msg);
891+
892+
// Should fail
893+
ASSERT_FALSE(rv == 0);
894+
ASSERT_TRUE(err_msg != nullptr);
895+
896+
std::string error_message(err_msg);
897+
std::unique_ptr<char, decltype(&free)> err_msg_ptr(err_msg, free);
898+
899+
// Error message should be reasonable length
900+
EXPECT_LT(error_message.length(), 300);
901+
902+
// Should contain expected error text
903+
EXPECT_NE(error_message.find("is not in list of allowed issuers"), std::string::npos);
904+
905+
// Should contain properly escaped JSON (with quotes)
906+
EXPECT_NE(error_message.find("\""), std::string::npos);
907+
}
908+
809909
int main(int argc, char **argv) {
810910
::testing::InitGoogleTest(&argc, argv);
811911
return RUN_ALL_TESTS();

0 commit comments

Comments
 (0)