From 1dcb8afa9ba8715447284826bb47d9be7f25800f Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Fri, 19 Jun 2026 10:57:30 -0400 Subject: [PATCH] Improve validation of algorithm identifiers in keys and signatures Some key types would for example completely ignore the OIDs in passed algorithm identifiers, on the assumption they were being called from one of the decoder factory functions. Likewise in some contexts where the algorithm identifier parameters are unused, any junk in there would be silently ignored. Also there were some cases where either NULL or absent parameters was accepted, even though the relevant standard specifically requires one or the other. --- src/lib/asn1/alg_id.cpp | 1 + src/lib/asn1/pss_params.cpp | 4 + src/lib/pubkey/curve448/ed448/ed448.cpp | 2 +- .../dilithium/dilithium_common/dilithium.cpp | 2 +- src/lib/pubkey/ecc_key/ecc_key.cpp | 10 +++ src/lib/pubkey/ecc_key/ecc_key.h | 3 + src/lib/pubkey/ecdh/ecdh.h | 4 +- src/lib/pubkey/ecdsa/ecdsa.cpp | 2 +- src/lib/pubkey/ecdsa/ecdsa.h | 4 +- src/lib/pubkey/ecgdsa/ecgdsa.h | 4 +- src/lib/pubkey/eckcdsa/eckcdsa.h | 4 +- src/lib/pubkey/ed25519/ed25519_key.cpp | 2 +- src/lib/pubkey/gost_3410/gost_3410.cpp | 90 +++++++++++++++++-- src/lib/pubkey/gost_3410/gost_3410.h | 5 +- src/lib/pubkey/hss_lms/hss_lms.cpp | 2 +- src/lib/pubkey/pk_algs.cpp | 10 +-- src/lib/pubkey/rsa/rsa.cpp | 18 ++-- src/lib/pubkey/sm2/sm2.cpp | 18 +++- src/lib/pubkey/sm2/sm2.h | 3 +- .../sphincsplus_common/sphincsplus.cpp | 2 +- src/lib/pubkey/xmss/xmss_publickey.cpp | 2 +- src/lib/x509/x509_crl.cpp | 2 +- src/lib/x509/x509cert.cpp | 3 +- src/tests/test_asn1.cpp | 36 ++++++-- src/tests/test_x509_path.cpp | 35 ++++++-- src/tests/unit_ecdsa.cpp | 18 +++- src/tests/unit_x509.cpp | 9 +- 27 files changed, 234 insertions(+), 61 deletions(-) diff --git a/src/lib/asn1/alg_id.cpp b/src/lib/asn1/alg_id.cpp index ec594e4eba3..75e569af103 100644 --- a/src/lib/asn1/alg_id.cpp +++ b/src/lib/asn1/alg_id.cpp @@ -57,6 +57,7 @@ bool operator==(const AlgorithmIdentifier& a1, const AlgorithmIdentifier& a2) { /* * Treat NULL and empty as equivalent + * TODO(Botan4) remove this */ if(a1.parameters_are_null_or_empty() && a2.parameters_are_null_or_empty()) { return true; diff --git a/src/lib/asn1/pss_params.cpp b/src/lib/asn1/pss_params.cpp index 956a3944f18..4e44d15b2e8 100644 --- a/src/lib/asn1/pss_params.cpp +++ b/src/lib/asn1/pss_params.cpp @@ -76,6 +76,10 @@ void PSS_Params::decode_from(BER_Decoder& from) { .end_cons(); BER_Decoder(m_mgf.parameters(), from.limits()).decode(m_mgf_hash).verify_end(); + + if(!m_hash.parameters_are_null_or_empty() || !m_mgf_hash.parameters_are_null_or_empty()) { + throw Decoding_Error("Unexpected parameters for PSS hash algorithm identifier"); + } } } // namespace Botan diff --git a/src/lib/pubkey/curve448/ed448/ed448.cpp b/src/lib/pubkey/curve448/ed448/ed448.cpp index 3812e32a05a..bf99d6ac416 100644 --- a/src/lib/pubkey/curve448/ed448/ed448.cpp +++ b/src/lib/pubkey/curve448/ed448/ed448.cpp @@ -283,7 +283,7 @@ std::unique_ptr Ed448_PublicKey::create_verification_op(st std::unique_ptr Ed448_PublicKey::create_x509_verification_op(const AlgorithmIdentifier& alg_id, std::string_view provider) const { if(provider == "base" || provider.empty()) { - if(alg_id != this->algorithm_identifier()) { + if(alg_id.oid() != this->object_identifier() || !alg_id.parameters_are_empty()) { throw Decoding_Error("Unexpected AlgorithmIdentifier for Ed448 X509 signature"); } diff --git a/src/lib/pubkey/dilithium/dilithium_common/dilithium.cpp b/src/lib/pubkey/dilithium/dilithium_common/dilithium.cpp index ae071d7bacc..fd04b2f05ee 100644 --- a/src/lib/pubkey/dilithium/dilithium_common/dilithium.cpp +++ b/src/lib/pubkey/dilithium/dilithium_common/dilithium.cpp @@ -405,7 +405,7 @@ std::unique_ptr Dilithium_PublicKey::create_verification_o std::unique_ptr Dilithium_PublicKey::create_x509_verification_op( const AlgorithmIdentifier& alg_id, std::string_view provider) const { if(provider.empty() || provider == "base") { - if(alg_id != this->algorithm_identifier()) { + if(alg_id.oid() != this->object_identifier() || !alg_id.parameters_are_empty()) { throw Decoding_Error("Unexpected AlgorithmIdentifier for Dilithium X.509 signature"); } return std::make_unique(m_public); diff --git a/src/lib/pubkey/ecc_key/ecc_key.cpp b/src/lib/pubkey/ecc_key/ecc_key.cpp index 9258e968642..9321e4201b5 100644 --- a/src/lib/pubkey/ecc_key/ecc_key.cpp +++ b/src/lib/pubkey/ecc_key/ecc_key.cpp @@ -43,6 +43,16 @@ EC_Group_Encoding default_encoding_for(const EC_Group& group) { } // namespace +const AlgorithmIdentifier& EC_PublicKey::assert_algorithm_identifier(const AlgorithmIdentifier& alg_id, + std::string_view alg_name) { + if(alg_id.oid() != OID::from_string(alg_name)) { + throw Decoding_Error( + fmt("Unexpected AlgorithmIdentifier OID {} in association with {} key", alg_id.oid(), alg_name)); + } + + return alg_id; +} + #if defined(BOTAN_HAS_LEGACY_EC_POINT) EC_PublicKey::EC_PublicKey(const EC_Group& group, const EC_Point& pub_point) { auto pt = EC_AffinePoint(group, pub_point); diff --git a/src/lib/pubkey/ecc_key/ecc_key.h b/src/lib/pubkey/ecc_key/ecc_key.h index b33b20b2b8d..ce0d250a790 100644 --- a/src/lib/pubkey/ecc_key/ecc_key.h +++ b/src/lib/pubkey/ecc_key/ecc_key.h @@ -134,6 +134,9 @@ class BOTAN_PUBLIC_API(2, 0) EC_PublicKey : public virtual Public_Key { */ EC_PublicKey(const AlgorithmIdentifier& alg_id, std::span key_bits); + static const AlgorithmIdentifier& assert_algorithm_identifier(const AlgorithmIdentifier& alg_id, + std::string_view alg_name); + EC_PublicKey() = default; std::shared_ptr m_public_key; // NOLINT(*non-private-member-variable*) diff --git a/src/lib/pubkey/ecdh/ecdh.h b/src/lib/pubkey/ecdh/ecdh.h index 0bb747209bd..7a457bb0e63 100644 --- a/src/lib/pubkey/ecdh/ecdh.h +++ b/src/lib/pubkey/ecdh/ecdh.h @@ -25,7 +25,7 @@ class BOTAN_PUBLIC_API(2, 0) ECDH_PublicKey : public virtual EC_PublicKey { * @param key_bits DER encoded public key bits */ ECDH_PublicKey(const AlgorithmIdentifier& alg_id, std::span key_bits) : - EC_PublicKey(alg_id, key_bits) {} + EC_PublicKey(assert_algorithm_identifier(alg_id, "ECDH"), key_bits) {} #if defined(BOTAN_HAS_LEGACY_EC_POINT) /** @@ -84,7 +84,7 @@ class BOTAN_PUBLIC_API(2, 0) ECDH_PrivateKey final : public ECDH_PublicKey, * @param key_bits ECPrivateKey bits */ ECDH_PrivateKey(const AlgorithmIdentifier& alg_id, std::span key_bits) : - EC_PrivateKey(alg_id, key_bits) {} + EC_PrivateKey(assert_algorithm_identifier(alg_id, "ECDH"), key_bits) {} /** * Create a private key from a given secret @p x diff --git a/src/lib/pubkey/ecdsa/ecdsa.cpp b/src/lib/pubkey/ecdsa/ecdsa.cpp index 1c79b3a506a..28c580df975 100644 --- a/src/lib/pubkey/ecdsa/ecdsa.cpp +++ b/src/lib/pubkey/ecdsa/ecdsa.cpp @@ -218,7 +218,7 @@ class ECDSA_Verification_Operation final : public PK_Ops::Verification_with_Hash PK_Ops::Verification_with_Hash(padding), m_group(ecdsa.domain()), m_gy_mul(ecdsa._public_ec_point()) {} ECDSA_Verification_Operation(const ECDSA_PublicKey& ecdsa, const AlgorithmIdentifier& alg_id) : - PK_Ops::Verification_with_Hash(alg_id, "ECDSA", true), + PK_Ops::Verification_with_Hash(alg_id, "ECDSA"), m_group(ecdsa.domain()), m_gy_mul(ecdsa._public_ec_point()) {} diff --git a/src/lib/pubkey/ecdsa/ecdsa.h b/src/lib/pubkey/ecdsa/ecdsa.h index d77110c5eca..1013ecfb24c 100644 --- a/src/lib/pubkey/ecdsa/ecdsa.h +++ b/src/lib/pubkey/ecdsa/ecdsa.h @@ -41,7 +41,7 @@ class BOTAN_PUBLIC_API(2, 0) ECDSA_PublicKey : public virtual EC_PublicKey { * @param key_bits DER encoded public key bits */ ECDSA_PublicKey(const AlgorithmIdentifier& alg_id, std::span key_bits) : - EC_PublicKey(alg_id, key_bits) {} + EC_PublicKey(assert_algorithm_identifier(alg_id, "ECDSA"), key_bits) {} /** * Recover a public key from a signature/msg pair @@ -95,7 +95,7 @@ class BOTAN_PUBLIC_API(2, 0) ECDSA_PrivateKey final : public ECDSA_PublicKey, * @param key_bits ECPrivateKey bits */ ECDSA_PrivateKey(const AlgorithmIdentifier& alg_id, std::span key_bits) : - EC_PrivateKey(alg_id, key_bits) {} + EC_PrivateKey(assert_algorithm_identifier(alg_id, "ECDSA"), key_bits) {} /** * Create a private key from a given secret @p x diff --git a/src/lib/pubkey/ecgdsa/ecgdsa.h b/src/lib/pubkey/ecgdsa/ecgdsa.h index 8c00b203c7e..0fc5b05bee3 100644 --- a/src/lib/pubkey/ecgdsa/ecgdsa.h +++ b/src/lib/pubkey/ecgdsa/ecgdsa.h @@ -39,7 +39,7 @@ class BOTAN_PUBLIC_API(2, 0) ECGDSA_PublicKey : public virtual EC_PublicKey { * @param key_bits DER encoded public key bits */ ECGDSA_PublicKey(const AlgorithmIdentifier& alg_id, std::span key_bits) : - EC_PublicKey(alg_id, key_bits) {} + EC_PublicKey(assert_algorithm_identifier(alg_id, "ECGDSA"), key_bits) {} /** * Get this keys algorithm name. @@ -79,7 +79,7 @@ class BOTAN_PUBLIC_API(2, 0) ECGDSA_PrivateKey final : public ECGDSA_PublicKey, * @param key_bits ECPrivateKey bits */ ECGDSA_PrivateKey(const AlgorithmIdentifier& alg_id, std::span key_bits) : - EC_PrivateKey(alg_id, key_bits, true) {} + EC_PrivateKey(assert_algorithm_identifier(alg_id, "ECGDSA"), key_bits, true) {} /** * Create a private key from a given secret @p x diff --git a/src/lib/pubkey/eckcdsa/eckcdsa.h b/src/lib/pubkey/eckcdsa/eckcdsa.h index 0773574996d..1f0ce518409 100644 --- a/src/lib/pubkey/eckcdsa/eckcdsa.h +++ b/src/lib/pubkey/eckcdsa/eckcdsa.h @@ -39,7 +39,7 @@ class BOTAN_PUBLIC_API(2, 0) ECKCDSA_PublicKey : public virtual EC_PublicKey { * @param key_bits DER encoded public key bits */ ECKCDSA_PublicKey(const AlgorithmIdentifier& alg_id, std::span key_bits) : - EC_PublicKey(alg_id, key_bits) {} + EC_PublicKey(assert_algorithm_identifier(alg_id, "ECKCDSA"), key_bits) {} /** * Get this keys algorithm name. @@ -78,7 +78,7 @@ class BOTAN_PUBLIC_API(2, 0) ECKCDSA_PrivateKey final : public ECKCDSA_PublicKey * @param key_bits ECPrivateKey bits */ ECKCDSA_PrivateKey(const AlgorithmIdentifier& alg_id, std::span key_bits) : - EC_PrivateKey(alg_id, key_bits, true) {} + EC_PrivateKey(assert_algorithm_identifier(alg_id, "ECKCDSA"), key_bits, true) {} /** * Create a private key from a given secret @p x diff --git a/src/lib/pubkey/ed25519/ed25519_key.cpp b/src/lib/pubkey/ed25519/ed25519_key.cpp index a088b74f07e..ecfa2d039cb 100644 --- a/src/lib/pubkey/ed25519/ed25519_key.cpp +++ b/src/lib/pubkey/ed25519/ed25519_key.cpp @@ -346,7 +346,7 @@ std::unique_ptr Ed25519_PublicKey::create_verification_op( std::unique_ptr Ed25519_PublicKey::create_x509_verification_op(const AlgorithmIdentifier& alg_id, std::string_view provider) const { if(provider == "base" || provider.empty()) { - if(alg_id != this->algorithm_identifier()) { + if(alg_id.oid() != this->object_identifier() || !alg_id.parameters_are_empty()) { throw Decoding_Error("Unexpected AlgorithmIdentifier for Ed25519 X509 signature"); } diff --git a/src/lib/pubkey/gost_3410/gost_3410.cpp b/src/lib/pubkey/gost_3410/gost_3410.cpp index 0969260e639..cee75d6a00e 100644 --- a/src/lib/pubkey/gost_3410/gost_3410.cpp +++ b/src/lib/pubkey/gost_3410/gost_3410.cpp @@ -27,6 +27,82 @@ EC_Group check_domain(EC_Group domain) { return domain; } +bool is_gost_3410_key_oid(const OID& oid) { + return oid == OID::from_string("GOST-34.10") || oid == OID::from_string("GOST-34.10-2012-256") || + oid == OID::from_string("GOST-34.10-2012-512"); +} + +const AlgorithmIdentifier& assert_gost_algorithm_identifier(const AlgorithmIdentifier& alg_id) { + if(!is_gost_3410_key_oid(alg_id.oid())) { + throw Decoding_Error( + fmt("Unexpected AlgorithmIdentifier OID {} in association with GOST 34.10 key", alg_id.oid())); + } + + return alg_id; +} + +OID decode_gost_key_parameters(const AlgorithmIdentifier& alg_id) { + OID ecc_param_id; + + auto params = BER_Decoder(alg_id.parameters(), BER_Decoder::Limits::DER()).start_sequence(); + params.decode(ecc_param_id); + + if(params.more_items()) { + OID digest_param_id; + params.decode(digest_param_id); + + if(alg_id.oid() == OID::from_string("GOST-34.10-2012-256") && + digest_param_id != OID::from_string("Streebog-256")) { + throw Decoding_Error("Unexpected digest parameters for GOST-34.10-2012-256 public key"); + } + + if(alg_id.oid() == OID::from_string("GOST-34.10-2012-512") && + digest_param_id != OID::from_string("Streebog-512")) { + throw Decoding_Error("Unexpected digest parameters for GOST-34.10-2012-512 public key"); + } + } + + if(params.more_items()) { + if(alg_id.oid() != OID::from_string("GOST-34.10")) { + throw Decoding_Error("Unexpected extra parameters for GOST-34.10-2012 public key"); + } + + OID encryption_param_id; + params.decode(encryption_param_id); + } + + params.verify_end(); + + return ecc_param_id; +} + +void check_gost_key_oid_matches_group(const OID& key_oid, const EC_Group& group) { + if(key_oid == OID::from_string("GOST-34.10-2012-256") && group.get_p_bits() != 256) { + throw Decoding_Error("GOST-34.10-2012-256 public key has unexpected parameters"); + } + + if(key_oid == OID::from_string("GOST-34.10-2012-512") && group.get_p_bits() != 512) { + throw Decoding_Error("GOST-34.10-2012-512 public key has unexpected parameters"); + } +} + +AlgorithmIdentifier gost_private_key_alg_id(const AlgorithmIdentifier& alg_id) { + assert_gost_algorithm_identifier(alg_id); + + OID ecc_param_id; + BER_Decoder decoder(alg_id.parameters(), BER_Decoder::Limits::DER()); + if(decoder.peek_next_object().type_tag() == ASN1_Type::ObjectId) { + decoder.decode(ecc_param_id).verify_end(); + } else { + ecc_param_id = decode_gost_key_parameters(alg_id); + } + + auto group = check_domain(EC_Group::from_OID(ecc_param_id)); + check_gost_key_oid_matches_group(alg_id.oid(), group); + + return AlgorithmIdentifier(alg_id.oid(), group.DER_encode()); +} + } // namespace std::optional GOST_3410_PublicKey::_signature_element_size_for_DER_encoding() const { @@ -71,17 +147,12 @@ AlgorithmIdentifier GOST_3410_PublicKey::algorithm_identifier() const { } GOST_3410_PublicKey::GOST_3410_PublicKey(const AlgorithmIdentifier& alg_id, std::span key_bits) { - OID ecc_param_id; + assert_gost_algorithm_identifier(alg_id); - // The parameters also includes hash and cipher OIDs - BER_Decoder(alg_id.parameters(), BER_Decoder::Limits::DER()) - .start_sequence() - .decode(ecc_param_id) - .discard_remaining() - .end_cons() - .verify_end(); + const OID ecc_param_id = decode_gost_key_parameters(alg_id); auto group = check_domain(EC_Group::from_OID(ecc_param_id)); + check_gost_key_oid_matches_group(alg_id.oid(), group); std::vector bits; BER_Decoder(key_bits, BER_Decoder::Limits::DER()).decode(bits, ASN1_Type::OctetString).verify_end(); @@ -111,6 +182,9 @@ GOST_3410_PrivateKey::GOST_3410_PrivateKey(RandomNumberGenerator& rng, EC_Group GOST_3410_PrivateKey::GOST_3410_PrivateKey(RandomNumberGenerator& rng, const EC_Group& domain, const BigInt& x) : EC_PrivateKey(rng, check_domain(domain), x) {} +GOST_3410_PrivateKey::GOST_3410_PrivateKey(const AlgorithmIdentifier& alg_id, std::span key_bits) : + EC_PrivateKey(gost_private_key_alg_id(alg_id), key_bits) {} + std::unique_ptr GOST_3410_PrivateKey::public_key() const { return std::make_unique(domain(), _public_ec_point()); } diff --git a/src/lib/pubkey/gost_3410/gost_3410.h b/src/lib/pubkey/gost_3410/gost_3410.h index 27a6aa93c32..27389331062 100644 --- a/src/lib/pubkey/gost_3410/gost_3410.h +++ b/src/lib/pubkey/gost_3410/gost_3410.h @@ -87,8 +87,7 @@ class BOTAN_PUBLIC_API(2, 0) GOST_3410_PrivateKey final : public GOST_3410_Publi * @param alg_id the X.509 algorithm identifier * @param key_bits ECPrivateKey bits */ - GOST_3410_PrivateKey(const AlgorithmIdentifier& alg_id, std::span key_bits) : - EC_PrivateKey(alg_id, key_bits) {} + GOST_3410_PrivateKey(const AlgorithmIdentifier& alg_id, std::span key_bits); /** * Create a private key from a given secret @p x @@ -116,7 +115,7 @@ class BOTAN_PUBLIC_API(2, 0) GOST_3410_PrivateKey final : public GOST_3410_Publi std::unique_ptr public_key() const override; AlgorithmIdentifier pkcs8_algorithm_identifier() const override { - return EC_PublicKey::algorithm_identifier(); // NOLINT(bugprone-parent-virtual-call) + return GOST_3410_PublicKey::algorithm_identifier(); // NOLINT(bugprone-parent-virtual-call) } std::unique_ptr create_signature_op(RandomNumberGenerator& rng, diff --git a/src/lib/pubkey/hss_lms/hss_lms.cpp b/src/lib/pubkey/hss_lms/hss_lms.cpp index 36f325c3a7d..34394b499f4 100644 --- a/src/lib/pubkey/hss_lms/hss_lms.cpp +++ b/src/lib/pubkey/hss_lms/hss_lms.cpp @@ -111,7 +111,7 @@ std::unique_ptr HSS_LMS_PublicKey::create_verification_op( std::unique_ptr HSS_LMS_PublicKey::create_x509_verification_op( const AlgorithmIdentifier& signature_algorithm, std::string_view provider) const { if(provider.empty() || provider == "base") { - if(signature_algorithm != this->algorithm_identifier()) { + if(signature_algorithm.oid() != this->object_identifier() || !signature_algorithm.parameters_are_empty()) { throw Decoding_Error("Unexpected AlgorithmIdentifier for HSS-LMS signature"); } return std::make_unique(m_public); diff --git a/src/lib/pubkey/pk_algs.cpp b/src/lib/pubkey/pk_algs.cpp index 4e8a4e13a3a..ae26ebc0628 100644 --- a/src/lib/pubkey/pk_algs.cpp +++ b/src/lib/pubkey/pk_algs.cpp @@ -131,10 +131,7 @@ std::unique_ptr load_public_key(const AlgorithmIdentifier& alg_id, [[maybe_unused]] std::span key_bits) { const std::string alg_name = [&]() -> std::string { if(const auto name = alg_id.oid().registered_name()) { - const std::vector alg_info = split_on(*name, '/'); - if(!alg_info.empty()) { - return alg_info[0]; - } + return *name; } throw Decoding_Error( @@ -298,10 +295,7 @@ std::unique_ptr load_private_key(const AlgorithmIdentifier& alg_id, [[maybe_unused]] std::span key_bits) { const std::string alg_name = [&]() -> std::string { if(const auto name = alg_id.oid().registered_name()) { - const std::vector alg_info = split_on(*name, '/'); - if(!alg_info.empty()) { - return alg_info[0]; - } + return *name; } throw Decoding_Error( diff --git a/src/lib/pubkey/rsa/rsa.cpp b/src/lib/pubkey/rsa/rsa.cpp index 9fddac6b231..d5ef7ef152b 100644 --- a/src/lib/pubkey/rsa/rsa.cpp +++ b/src/lib/pubkey/rsa/rsa.cpp @@ -166,9 +166,12 @@ void RSA_PublicKey::init(BigInt&& n, BigInt&& e) { RSA_PublicKey::RSA_PublicKey(const AlgorithmIdentifier& alg_id, std::span key_bits) { // RFC 4055 Section 1.2 has that parameters MUST be NULL, but historical // reasons make that difficult to enforce, so absent is also accepted. - // - // This only checks rsaEncryption; PSS/OAEP key identifiers have their own parameter encoding - if(alg_id.oid().registered_name() == "RSA" && !alg_id.parameters_are_null_or_empty()) { + if(alg_id.oid().registered_name() != "RSA") { + throw Decoding_Error( + fmt("Unexpected AlgorithmIdentifier OID {} in association with RSA public key", alg_id.oid())); + } + + if(!alg_id.parameters_are_null_or_empty()) { throw Decoding_Error("Unexpected parameters for RSA public key"); } @@ -280,7 +283,12 @@ void RSA_PrivateKey::init(BigInt&& d, BigInt&& p, BigInt&& q, BigInt&& d1, BigIn } RSA_PrivateKey::RSA_PrivateKey(const AlgorithmIdentifier& alg_id, std::span key_bits) { - if(alg_id.oid().registered_name() == "RSA" && !alg_id.parameters_are_null_or_empty()) { + if(alg_id.oid().registered_name() != "RSA") { + throw Decoding_Error( + fmt("Unexpected AlgorithmIdentifier OID {} in association with RSA private key", alg_id.oid())); + } + + if(!alg_id.parameters_are_null_or_empty()) { throw Decoding_Error("Unexpected parameters for RSA private key"); } @@ -682,7 +690,7 @@ AlgorithmIdentifier RSA_Signature_Operation::algorithm_identifier() const { try { const std::string full_name = "RSA/" + padding_name; const OID oid = OID::from_string(full_name); - return AlgorithmIdentifier(oid, AlgorithmIdentifier::USE_EMPTY_PARAM); + return AlgorithmIdentifier(oid, AlgorithmIdentifier::USE_NULL_PARAM); } catch(Lookup_Error&) {} if(padding_name.starts_with("PSS(")) { diff --git a/src/lib/pubkey/sm2/sm2.cpp b/src/lib/pubkey/sm2/sm2.cpp index d0b007633af..6e288651d8d 100644 --- a/src/lib/pubkey/sm2/sm2.cpp +++ b/src/lib/pubkey/sm2/sm2.cpp @@ -10,6 +10,7 @@ #include #include +#include #include #include #include @@ -20,6 +21,21 @@ std::string SM2_PublicKey::algo_name() const { return "SM2"; } +namespace { + +const AlgorithmIdentifier& assert_sm2_algorithm_identifier(const AlgorithmIdentifier& alg_id) { + if(alg_id.oid() != OID::from_string("SM2") && alg_id.oid() != OID::from_string("SM2_Enc")) { + throw Decoding_Error(fmt("Unexpected AlgorithmIdentifier OID {} in association with SM2 key", alg_id.oid())); + } + + return alg_id; +} + +} // namespace + +SM2_PublicKey::SM2_PublicKey(const AlgorithmIdentifier& alg_id, std::span key_bits) : + EC_PublicKey(assert_sm2_algorithm_identifier(alg_id), key_bits) {} + std::optional SM2_PublicKey::_signature_element_size_for_DER_encoding() const { return domain().get_order_bytes(); } @@ -50,7 +66,7 @@ bool SM2_PrivateKey::check_key(RandomNumberGenerator& rng, bool strong) const { } SM2_PrivateKey::SM2_PrivateKey(const AlgorithmIdentifier& alg_id, std::span key_bits) : - EC_PrivateKey(alg_id, key_bits), + EC_PrivateKey(assert_sm2_algorithm_identifier(alg_id), key_bits), m_da_inv((this->_private_key() + EC_Scalar::one(domain())).invert()), m_da_inv_legacy(m_da_inv.to_bigint()) {} diff --git a/src/lib/pubkey/sm2/sm2.h b/src/lib/pubkey/sm2/sm2.h index 827bc3668e9..6a16c1b98bc 100644 --- a/src/lib/pubkey/sm2/sm2.h +++ b/src/lib/pubkey/sm2/sm2.h @@ -40,8 +40,7 @@ class BOTAN_PUBLIC_API(2, 2) SM2_PublicKey : public virtual EC_PublicKey { * @param alg_id the X.509 algorithm identifier * @param key_bits DER encoded public key bits */ - SM2_PublicKey(const AlgorithmIdentifier& alg_id, std::span key_bits) : - EC_PublicKey(alg_id, key_bits) {} + SM2_PublicKey(const AlgorithmIdentifier& alg_id, std::span key_bits); /** * Get this keys algorithm name. diff --git a/src/lib/pubkey/sphincsplus/sphincsplus_common/sphincsplus.cpp b/src/lib/pubkey/sphincsplus/sphincsplus_common/sphincsplus.cpp index c4bd4a57564..7cc58b159d0 100644 --- a/src/lib/pubkey/sphincsplus/sphincsplus_common/sphincsplus.cpp +++ b/src/lib/pubkey/sphincsplus/sphincsplus_common/sphincsplus.cpp @@ -265,7 +265,7 @@ std::unique_ptr SphincsPlus_PublicKey::create_verification std::unique_ptr SphincsPlus_PublicKey::create_x509_verification_op( const AlgorithmIdentifier& signature_algorithm, std::string_view provider) const { if(provider.empty() || provider == "base") { - if(signature_algorithm != this->algorithm_identifier()) { + if(signature_algorithm.oid() != this->object_identifier() || !signature_algorithm.parameters_are_empty()) { throw Decoding_Error("Unexpected AlgorithmIdentifier for SLH-DSA (or SPHINCS+) signature"); } return std::make_unique(m_public); diff --git a/src/lib/pubkey/xmss/xmss_publickey.cpp b/src/lib/pubkey/xmss/xmss_publickey.cpp index 2c750f4fe16..9d6ea111fbf 100644 --- a/src/lib/pubkey/xmss/xmss_publickey.cpp +++ b/src/lib/pubkey/xmss/xmss_publickey.cpp @@ -175,7 +175,7 @@ std::unique_ptr XMSS_PublicKey::create_verification_op(std std::unique_ptr XMSS_PublicKey::create_x509_verification_op(const AlgorithmIdentifier& alg_id, std::string_view provider) const { if(provider == "base" || provider.empty()) { - if(alg_id != this->algorithm_identifier()) { + if(alg_id.oid() != this->object_identifier() || !alg_id.parameters_are_empty()) { throw Decoding_Error("Unexpected AlgorithmIdentifier for XMSS X509 signature"); } return std::make_unique(*this); diff --git a/src/lib/x509/x509_crl.cpp b/src/lib/x509/x509_crl.cpp index 37f45c230dd..d1726fb59d8 100644 --- a/src/lib/x509/x509_crl.cpp +++ b/src/lib/x509/x509_crl.cpp @@ -139,7 +139,7 @@ std::unique_ptr decode_crl_body(const std::vector& body, cons AlgorithmIdentifier sig_algo_inner; tbs_crl.decode(sig_algo_inner); - if(sig_algo != sig_algo_inner) { + if(sig_algo.oid() != sig_algo_inner.oid() || sig_algo.parameters() != sig_algo_inner.parameters()) { throw Decoding_Error("Algorithm identifier mismatch in CRL"); } diff --git a/src/lib/x509/x509cert.cpp b/src/lib/x509/x509cert.cpp index f59c3ab6f55..6df772abc76 100644 --- a/src/lib/x509/x509cert.cpp +++ b/src/lib/x509/x509cert.cpp @@ -136,7 +136,8 @@ std::unique_ptr parse_x509_cert_body(const X509_Object& o if(data->m_version > 2) { throw Decoding_Error("Unknown X.509 cert version " + std::to_string(data->m_version)); } - if(obj.signature_algorithm() != data->m_sig_algo_inner) { + if(obj.signature_algorithm().oid() != data->m_sig_algo_inner.oid() || + obj.signature_algorithm().parameters() != data->m_sig_algo_inner.parameters()) { throw Decoding_Error("X.509 Certificate had differing algorithm identifiers in inner and outer ID fields"); } diff --git a/src/tests/test_asn1.cpp b/src/tests/test_asn1.cpp index 4181cb4759f..fb6b958d9de 100644 --- a/src/tests/test_asn1.cpp +++ b/src/tests/test_asn1.cpp @@ -480,12 +480,12 @@ Test::Result test_pss_params_rejects_trailing_data_in_mgf1_params() { const Botan::AlgorithmIdentifier sha256_alg_id("SHA-256", Botan::AlgorithmIdentifier::USE_NULL_PARAM); const auto sha256_der = sha256_alg_id.BER_encode(); - auto encode_pss_params = [&](const std::vector& mgf_params) { - const Botan::AlgorithmIdentifier mgf("MGF1", mgf_params); + auto encode_pss_params = [](const Botan::AlgorithmIdentifier& hash, const Botan::AlgorithmIdentifier& mgf_hash) { + const Botan::AlgorithmIdentifier mgf("MGF1", mgf_hash.BER_encode()); Botan::DER_Encoder enc; enc.start_sequence() .start_context_specific(0) - .encode(sha256_alg_id) + .encode(hash) .end_cons() .start_context_specific(1) .encode(mgf) @@ -498,7 +498,7 @@ Test::Result test_pss_params_rejects_trailing_data_in_mgf1_params() { }; try { - const auto clean_der = encode_pss_params(sha256_der); + const auto clean_der = encode_pss_params(sha256_alg_id, sha256_alg_id); const Botan::PSS_Params clean(clean_der); result.test_success("control: clean PSS-Params decodes"); } catch(const std::exception& e) { @@ -508,11 +508,37 @@ Test::Result test_pss_params_rejects_trailing_data_in_mgf1_params() { std::vector mgf_params_with_junk = sha256_der; const std::vector trailing_junk{0x02, 0x01, 0x00}; mgf_params_with_junk.insert(mgf_params_with_junk.end(), trailing_junk.begin(), trailing_junk.end()); - const auto bad_der = encode_pss_params(mgf_params_with_junk); + const Botan::AlgorithmIdentifier mgf_with_trailing_junk("MGF1", mgf_params_with_junk); + + Botan::DER_Encoder bad_mgf_params_enc; + bad_mgf_params_enc.start_sequence() + .start_context_specific(0) + .encode(sha256_alg_id) + .end_cons() + .start_context_specific(1) + .encode(mgf_with_trailing_junk) + .end_cons() + .start_context_specific(2) + .encode(static_cast(32)) + .end_cons() + .end_cons(); + const auto bad_der = bad_mgf_params_enc.get_contents(); result.test_throws("PSS-Params rejects trailing data in MGF1 parameters", [&]() { const Botan::PSS_Params bad(bad_der); }); + const Botan::AlgorithmIdentifier sha256_with_params("SHA-256", std::vector{0x04, 0x00}); + + result.test_throws("PSS-Params rejects hash AlgorithmIdentifier parameters", [&]() { + const auto bad_hash_der = encode_pss_params(sha256_with_params, sha256_alg_id); + const Botan::PSS_Params bad(bad_hash_der); + }); + + result.test_throws("PSS-Params rejects MGF1 hash AlgorithmIdentifier parameters", [&]() { + const auto bad_mgf_hash_der = encode_pss_params(sha256_alg_id, sha256_with_params); + const Botan::PSS_Params bad(bad_mgf_hash_der); + }); + return result; } diff --git a/src/tests/test_x509_path.cpp b/src/tests/test_x509_path.cpp index d4544ea2877..1fe44fda9d6 100644 --- a/src/tests/test_x509_path.cpp +++ b/src/tests/test_x509_path.cpp @@ -10,6 +10,7 @@ #if defined(BOTAN_HAS_X509_CERTIFICATES) #include "test_arb_eq.h" #include + #include #include #include #include @@ -344,6 +345,18 @@ BOTAN_REGISTER_TEST("x509", "x509_path_extended", Extended_Path_Validation_Tests #if defined(BOTAN_HAS_PSS) +bool spki_uses_rsa_pss(std::span spki) { + Botan::AlgorithmIdentifier key_alg_id; + Botan::BER_Decoder(spki, Botan::BER_Decoder::Limits::DER()) + .start_sequence() + .decode(key_alg_id) + .discard_remaining() + .end_cons() + .verify_end(); + + return key_alg_id.oid() == Botan::OID::from_string("RSA/PSS"); +} + class PSS_Path_Validation_Tests : public Test { public: std::vector run() override; @@ -412,14 +425,24 @@ std::vector PSS_Path_Validation_Tests::run() { result.test_str_eq(test_name + " path validation result", validation_result.result_string(), expected_result); } else if(end && !root) { // CRT self signed test - auto pubkey = end->subject_public_key(); - const bool accept = expected_result == "Verified"; - result.test_bool_eq(test_name + " verify signature", end->check_signature(*pubkey), accept); + if(end->subject_public_key_algo().oid() == Botan::OID::from_string("RSA/PSS")) { + result.test_throws(test_name + " rejects RSA-PSS SubjectPublicKeyInfo", + [&]() { auto pubkey = end->subject_public_key(); }); + } else { + auto pubkey = end->subject_public_key(); + const bool accept = expected_result == "Verified"; + result.test_bool_eq(test_name + " verify signature", end->check_signature(*pubkey), accept); + } } else if(csr) { // PKCS#10 Request test - auto pubkey = csr->subject_public_key(); - const bool accept = expected_result == "Verified"; - result.test_bool_eq(test_name + " verify signature", csr->check_signature(*pubkey), accept); + if(spki_uses_rsa_pss(csr->raw_public_key())) { + result.test_throws(test_name + " rejects RSA-PSS SubjectPublicKeyInfo", + [&]() { auto pubkey = csr->subject_public_key(); }); + } else { + auto pubkey = csr->subject_public_key(); + const bool accept = expected_result == "Verified"; + result.test_bool_eq(test_name + " verify signature", csr->check_signature(*pubkey), accept); + } } result.end_timer(); diff --git a/src/tests/unit_ecdsa.cpp b/src/tests/unit_ecdsa.cpp index 0083537b3da..56272e325ad 100644 --- a/src/tests/unit_ecdsa.cpp +++ b/src/tests/unit_ecdsa.cpp @@ -48,7 +48,11 @@ Test::Result test_decode_ecdsa_X509() { "3B:6C:99:1C:D6:5A:51:FC:EB:17:E3:AA:F6:3C:1A:DA:14:1F:82:41:30:6F:64:EE:FF:63:F3:1F:D6:07:14:9F"); auto pubkey = cert.subject_public_key(); - result.test_is_true("verify self-signed signature", cert.check_signature(*pubkey)); + if(cert.signature_algorithm().parameters_are_null()) { + result.test_is_false("reject NULL ECDSA signature parameters", cert.check_signature(*pubkey)); + } else { + result.test_is_true("verify self-signed signature", cert.check_signature(*pubkey)); + } } catch(Botan::Exception& e) { result.test_failure(e.what()); } @@ -66,7 +70,11 @@ Test::Result test_decode_ver_link_SHA256() { const Botan::X509_Certificate link_cert(Test::data_file("x509/ecc/link_SHA256.cer")); auto pubkey = root_cert.subject_public_key(); - result.test_is_true("verified self-signed signature", link_cert.check_signature(*pubkey)); + if(link_cert.signature_algorithm().parameters_are_null()) { + result.test_is_false("reject NULL ECDSA signature parameters", link_cert.check_signature(*pubkey)); + } else { + result.test_is_true("verified self-signed signature", link_cert.check_signature(*pubkey)); + } } catch(Botan::Exception& e) { result.test_failure(e.what()); } @@ -92,7 +100,11 @@ Test::Result test_decode_ver_link_SHA1() { !link_cert.check_signature(*pubkey)); return result; } - result.test_is_true("verified self-signed signature", link_cert.check_signature(*pubkey)); + if(link_cert.signature_algorithm().parameters_are_null()) { + result.test_is_false("reject NULL ECDSA signature parameters", link_cert.check_signature(*pubkey)); + } else { + result.test_is_true("verified self-signed signature", link_cert.check_signature(*pubkey)); + } } catch(Botan::Exception& e) { result.test_failure(e.what()); } diff --git a/src/tests/unit_x509.cpp b/src/tests/unit_x509.cpp index 3c620cbb37d..a1ade1b3c89 100644 --- a/src/tests/unit_x509.cpp +++ b/src/tests/unit_x509.cpp @@ -619,8 +619,8 @@ Test::Result test_rsa_oaep() { #if defined(BOTAN_HAS_RSA) const Botan::X509_Certificate cert(Test::data_file("x509/misc/rsa_oaep.pem")); - auto public_key = cert.subject_public_key(); - result.test_not_null("Decoding RSA-OAEP worked", public_key.get()); + result.test_throws("RSA-OAEP SubjectPublicKeyInfo is not decoded as generic RSA", + [&]() { auto public_key = cert.subject_public_key(); }); const auto& pk_info = cert.subject_public_key_algo(); result.test_str_eq("RSA-OAEP OID", pk_info.oid().to_string(), Botan::OID::from_string("RSA/OAEP").to_string()); @@ -927,6 +927,8 @@ Test::Result test_padding_config() { test_result.test_opt_str_eq("CA certificate signature algorithm (default)", ca_cert_def.signature_algorithm().oid().registered_name(), "RSA/PKCS1v15(SHA-512)"); + test_result.test_is_true("CA certificate PKCS1v15 signature parameters are NULL", + ca_cert_def.signature_algorithm().parameters_are_null()); // Create X509 CA certificate; RSA-PSS is explicitly set opt.set_padding_scheme("PSSR"); @@ -980,6 +982,8 @@ Test::Result test_padding_config() { test_result.test_opt_str_eq("End certificate signature algorithm", end_cert_pkcs1.signature_algorithm().oid().registered_name(), "RSA/PKCS1v15(SHA-512)"); + test_result.test_is_true("End certificate PKCS1v15 signature parameters are NULL", + end_cert_pkcs1.signature_algorithm().parameters_are_null()); // Create X509 CA object: its signer will use the explicitly configured padding scheme, which is different from the CA certificate's scheme const Botan::X509_CA ca_diff(ca_cert_def, (*sk), "SHA-512", "PSS", *rng); @@ -1008,7 +1012,6 @@ Test::Result test_padding_config() { return test_result; } #endif - #endif Test::Result test_pkcs10_ext(const Botan::Private_Key& key,