Improve validation of algorithm identifiers in keys and signatures

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;

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

src/lib/pubkey/curve448/ed448/ed448.cpp

@@ -283,7 +283,7 @@ std::unique_ptr<PK_Ops::Verification> Ed448_PublicKey::create_verification_op(st
 std::unique_ptr<PK_Ops::Verification> 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");
       }
 

src/lib/pubkey/dilithium/dilithium_common/dilithium.cpp

@@ -405,7 +405,7 @@ std::unique_ptr<PK_Ops::Verification> Dilithium_PublicKey::create_verification_o
 std::unique_ptr<PK_Ops::Verification> 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<Dilithium_Verification_Operation>(m_public);

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);

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<const uint8_t> key_bits);
 
+      static const AlgorithmIdentifier& assert_algorithm_identifier(const AlgorithmIdentifier& alg_id,
+                                                                    std::string_view alg_name);
+
       EC_PublicKey() = default;
 
       std::shared_ptr<const EC_PublicKey_Data> m_public_key;                // NOLINT(*non-private-member-variable*)

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<const uint8_t> 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<const uint8_t> 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

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()) {}
 

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<const uint8_t> 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<const uint8_t> 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

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<const uint8_t> 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<const uint8_t> 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

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<const uint8_t> 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<const uint8_t> 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

src/lib/pubkey/ed25519/ed25519_key.cpp

@@ -346,7 +346,7 @@ std::unique_ptr<PK_Ops::Verification> Ed25519_PublicKey::create_verification_op(
 std::unique_ptr<PK_Ops::Verification> 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");
       }
 

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<size_t> 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<const uint8_t> 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<uint8_t> 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<const uint8_t> key_bits) :
+      EC_PrivateKey(gost_private_key_alg_id(alg_id), key_bits) {}
+
 std::unique_ptr<Public_Key> GOST_3410_PrivateKey::public_key() const {
    return std::make_unique<GOST_3410_PublicKey>(domain(), _public_ec_point());
 }

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<const uint8_t> key_bits) :
-            EC_PrivateKey(alg_id, key_bits) {}
+      GOST_3410_PrivateKey(const AlgorithmIdentifier& alg_id, std::span<const uint8_t> 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> 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<PK_Ops::Signature> create_signature_op(RandomNumberGenerator& rng,

src/lib/pubkey/hss_lms/hss_lms.cpp

@@ -111,7 +111,7 @@ std::unique_ptr<PK_Ops::Verification> HSS_LMS_PublicKey::create_verification_op(
 std::unique_ptr<PK_Ops::Verification> 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<HSS_LMS_Verification_Operation>(m_public);

src/lib/pubkey/pk_algs.cpp

@@ -131,10 +131,7 @@ std::unique_ptr<Public_Key> load_public_key(const AlgorithmIdentifier& alg_id,
                                             [[maybe_unused]] std::span<const uint8_t> key_bits) {
    const std::string alg_name = [&]() -> std::string {
       if(const auto name = alg_id.oid().registered_name()) {
-         const std::vector<std::string> 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<Private_Key> load_private_key(const AlgorithmIdentifier& alg_id,
                                               [[maybe_unused]] std::span<const uint8_t> key_bits) {
    const std::string alg_name = [&]() -> std::string {
       if(const auto name = alg_id.oid().registered_name()) {
-         const std::vector<std::string> alg_info = split_on(*name, '/');
-         if(!alg_info.empty()) {
-            return alg_info[0];
-         }
+         return *name;
       }
 
       throw Decoding_Error(

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<const uint8_t> 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<const uint8_t> 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(")) {

src/lib/pubkey/sm2/sm2.cpp

@@ -10,6 +10,7 @@
 
 #include <botan/ec_group.h>
 #include <botan/hash.h>
+#include <botan/internal/fmt.h>
 #include <botan/internal/keypair.h>
 #include <botan/internal/loadstor.h>
 #include <botan/internal/pk_ops_impl.h>
@@ -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<const uint8_t> key_bits) :
+      EC_PublicKey(assert_sm2_algorithm_identifier(alg_id), key_bits) {}
+
 std::optional<size_t> 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<const uint8_t> 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()) {}