Repository: randombit/botan
Pull request #2419 (2020-10-09 15:02:16)

improve check if signature algorithm supports DER SEQUENCE encoding
Recently we added in some test code RSA signing which used the DER SEQUENCE format. This of course failed, as it is currently not supported.

But because the error message was only `Encoding error: Unexpected size for DER signature`, it was not immediately obvious that this was not an error in our code, but that it is simply not supported. This PR should improve the given error message.

Note that this would break code there currently the following is done:
```c++
// key: a Key of an algorithm there DER SEQUENCE format is not supported, e.g. RSA
PK_Verifier verifier(key, "EMSA4(SHA-512)", Signature_Format::DER_SEQUENCE);
verifier.set_input_format(Signature_Format::IEEE_1363);
// code for verification ...
```

Also added a test case for using ECDSA signatures with the DER SEQUENCE format.

/cc @securitykernel 


Patch:
diff --git a/src/lib/pubkey/pubkey.cpp b/src/lib/pubkey/pubkey.cpp
index d98b5dc9e..3834489ef 100644
--- a/src/lib/pubkey/pubkey.cpp
+++ b/src/lib/pubkey/pubkey.cpp
@@ -223,6 +223,12 @@ SymmetricKey PK_Key_Agreement::derive_key(size_t key_len,
    return m_op->agree(key_len, in, in_len, salt, salt_len);
    }
 
+static void check_der_format_supported(Signature_Format format, size_t parts)
+   {
+      if(format != IEEE_1363 && parts == 1)
+         throw Invalid_Argument("PK: This algorithm does not support DER encoding");
+   }
+
 PK_Signer::PK_Signer(const Private_Key& key,
                      RandomNumberGenerator& rng,
                      const std::string& emsa,
@@ -235,6 +241,7 @@ PK_Signer::PK_Signer(const Private_Key& key,
    m_sig_format = format;
    m_parts = key.message_parts();
    m_part_size = key.message_part_size();
+   check_der_format_supported(format, m_parts);
    }
 
 PK_Signer::~PK_Signer() { /* for unique_ptr */ }
@@ -310,14 +317,14 @@ PK_Verifier::PK_Verifier(const Public_Key& key,
    m_sig_format = format;
    m_parts = key.message_parts();
    m_part_size = key.message_part_size();
+   check_der_format_supported(format, m_parts);
    }
 
 PK_Verifier::~PK_Verifier() { /* for unique_ptr */ }
 
 void PK_Verifier::set_input_format(Signature_Format format)
    {
-   if(format != IEEE_1363 && m_parts == 1)
-      throw Invalid_Argument("PK_Verifier: This algorithm does not support DER encoding");
+   check_der_format_supported(format, m_parts);
    m_sig_format = format;
    }
 


Test patch:
diff --git a/src/tests/test_ecdsa.cpp b/src/tests/test_ecdsa.cpp
index 860fe7e11..b3d9c5376 100644
--- a/src/tests/test_ecdsa.cpp
+++ b/src/tests/test_ecdsa.cpp
@@ -12,6 +12,7 @@
 #if defined(BOTAN_HAS_ECDSA)
    #include "test_pubkey.h"
    #include <botan/ecdsa.h>
+   #include <botan/pk_algs.h>
 #endif
 
 namespace Botan_Tests {
@@ -138,6 +139,18 @@ class ECDSA_Signature_KAT_Tests final : public PK_Signature_Generation_Test
 #endif
    };
 
+class ECDSA_Sign_Verify_DER_Test final : public PK_Sign_Verify_DER_Test
+   {
+   public:
+      ECDSA_Sign_Verify_DER_Test() :
+         PK_Sign_Verify_DER_Test("ECDSA", "EMSA1(SHA-512)") {}
+
+      std::unique_ptr<Botan::Private_Key> key() const override
+         {
+         return Botan::create_private_key( "ECDSA",  Test::rng(), "secp256r1" );
+         }
+   };
+
 class ECDSA_Keygen_Tests final : public PK_Key_Generation_Test
    {
    public:
@@ -245,6 +258,7 @@ class ECDSA_Invalid_Key_Tests final : public Text_Based_Test
 BOTAN_REGISTER_TEST("ecdsa_verify", ECDSA_Verification_Tests);
 BOTAN_REGISTER_TEST("ecdsa_verify_wycheproof", ECDSA_Wycheproof_Verification_Tests);
 BOTAN_REGISTER_TEST("ecdsa_sign", ECDSA_Signature_KAT_Tests);
+BOTAN_REGISTER_TEST("ecdsa_sign_verify_der", ECDSA_Sign_Verify_DER_Test);
 BOTAN_REGISTER_TEST("ecdsa_keygen", ECDSA_Keygen_Tests);
 BOTAN_REGISTER_TEST("ecdsa_invalid", ECDSA_Invalid_Key_Tests);
 
diff --git a/src/tests/test_pubkey.cpp b/src/tests/test_pubkey.cpp
index 4c612fcd0..703f7360f 100644
--- a/src/tests/test_pubkey.cpp
+++ b/src/tests/test_pubkey.cpp
@@ -278,6 +278,63 @@ PK_Signature_NonVerification_Test::run_one_test(const std::string& pad_hdr, cons
    return result;
    }
 
+std::vector<Test::Result>
+PK_Sign_Verify_DER_Test::run()
+   {
+   const std::vector<uint8_t> message = {'f', 'o', 'o', 'b', 'a', 'r'};
+   const std::string padding = m_padding;
+
+   std::unique_ptr<Botan::Private_Key> privkey = key();
+
+   Test::Result result(algo_name() + "/" + padding + " signature sign/verify using DER format");
+
+   for(auto const& provider : possible_providers(algo_name()))
+      {
+      std::unique_ptr<Botan::PK_Signer> signer;
+      std::unique_ptr<Botan::PK_Verifier> verifier;
+
+      try
+         {
+         signer.reset(new Botan::PK_Signer(*privkey, Test::rng(), padding, Botan::DER_SEQUENCE, provider));
+         verifier.reset(new Botan::PK_Verifier(*privkey, padding, Botan::DER_SEQUENCE, provider));
+         }
+      catch(Botan::Lookup_Error& e)
+         {
+         result.test_note("Skipping sign/verify with " + provider, e.what());
+         }
+
+      if(signer && verifier)
+         {
+         try
+            {
+            std::vector<uint8_t> generated_signature = signer->sign_message(message, Test::rng());
+            const bool verified = verifier->verify_message(message, generated_signature);
+
+            result.test_eq("correct signature valid with " + provider, verified, true);
+
+            if(test_random_invalid_sigs())
+               {
+               check_invalid_signatures(result, *verifier, message, generated_signature);
+               }
+            }
+         catch(std::exception& e)
+            {
+            result.test_failure("verification threw exception", e.what());
+            }
+         }
+      }
+
+   return {result};
+   }
+
+std::vector<std::string> PK_Sign_Verify_DER_Test::possible_providers(
+   const std::string& algo)
+   {
+   std::vector<std::string> pk_provider =
+      Botan::probe_provider_private_key(algo, { "base", "commoncrypto", "openssl", "tpm" });
+   return Test::provider_filter(pk_provider);
+   }
+
 Test::Result
 PK_Encryption_Decryption_Test::run_one_test(const std::string& pad_hdr, const VarMap& vars)
    {
diff --git a/src/tests/test_pubkey.h b/src/tests/test_pubkey.h
index f828a8134..dcefd5625 100644
--- a/src/tests/test_pubkey.h
+++ b/src/tests/test_pubkey.h
@@ -104,6 +104,32 @@ class PK_Signature_NonVerification_Test : public PK_Test
       Test::Result run_one_test(const std::string& header, const VarMap& vars) override final;
    };
 
+class PK_Sign_Verify_DER_Test : public Test
+   {
+   public:
+      PK_Sign_Verify_DER_Test(const std::string& algo,
+                              const std::string& padding)
+         : m_algo(algo), m_padding(padding) {}
+
+      std::string algo_name() const
+         {
+         return m_algo;
+         }
+
+   protected:
+      std::vector<Test::Result> run() override final;
+
+      virtual std::unique_ptr<Botan::Private_Key> key() const = 0;
+
+      virtual bool test_random_invalid_sigs() const { return true; }
+
+      std::vector<std::string> possible_providers(const std::string& params) override;
+
+   private:
+      std::string m_algo;
+      std::string m_padding;
+   };
+
 class PK_Encryption_Decryption_Test : public PK_Test
    {
    public: