Fix decoding short EC private keys

Starting in 3.6.0 a logic change started rejecting EC keys where the
byte encoding is shorter than required.

Most implementations including OpenSSL accept such keys.

GH #4539

doc/api_ref/ecc.rst

@@ -177,14 +177,9 @@ Only curves over prime fields are supported.
 
       The encoded integer should be no greater than ``n**2``.
 
-   .. cpp:function:: EC_Scalar(const EC_Group& group, std::span<const uint8_t> buf)
-
-      Deserialize a scalar. This is equivalent to :cpp:func:`deserialize` except that
-      it will throw an exception if the input is unacceptable.
-
    .. cpp:function:: static EC_Scalar random(const EC_Group& group, RandomNumberGenerator& rng)
 
-       Return a random scalar
+       Return a random non-zero scalar value
 
    .. cpp:function:: static EC_Scalar gk_x_mod_order(const EC_Scalar& scalar, RandomNumberGenerator& rng, std::vector<BigInt>& ws)
 

src/lib/pubkey/ecc_key/ec_key_data.cpp

@@ -23,8 +23,39 @@ EC_PrivateKey_Data::EC_PrivateKey_Data(EC_Group group, const BigInt& x) :
 EC_PrivateKey_Data::EC_PrivateKey_Data(EC_Group group, EC_Scalar x) :
       m_group(std::move(group)), m_scalar(std::move(x)), m_legacy_x(m_scalar.to_bigint()) {}
 
+namespace {
+
+EC_Scalar decode_ec_secret_key_scalar(const EC_Group& group, std::span<const uint8_t> bytes) {
+   const size_t order_bytes = group.get_order_bytes();
+
+   if(bytes.size() < order_bytes) {
+      /*
+      * Older versions had a bug which caused secret keys to not be encoded to
+      * the full byte length of the order if there were leading zero bytes. This
+      * was particularly a problem for P-521, where on average half of keys do
+      * not have their high bit set and so can be encoded in 65 bytes, vs 66
+      * bytes for the full order.
+      *
+      * To accomodate this, zero prefix the key if we see such a short input
+      */
+      secure_vector<uint8_t> padded_sk(order_bytes);
+      copy_mem(std::span{padded_sk}.last(bytes.size()), bytes);
+      return decode_ec_secret_key_scalar(group, padded_sk);
+   }
+
+   if(auto s = EC_Scalar::deserialize(group, bytes)) {
+      return s.value();
+   } else {
+      throw Decoding_Error("EC private key is invalid for this group");
+   }
+}
+
+}  // namespace
+
 EC_PrivateKey_Data::EC_PrivateKey_Data(EC_Group group, std::span<const uint8_t> bytes) :
-      m_group(std::move(group)), m_scalar(m_group, bytes), m_legacy_x(m_scalar.to_bigint()) {}
+      m_group(std::move(group)),
+      m_scalar(decode_ec_secret_key_scalar(m_group, bytes)),
+      m_legacy_x(m_scalar.to_bigint()) {}
 
 std::shared_ptr<EC_PublicKey_Data> EC_PrivateKey_Data::public_key(RandomNumberGenerator& rng,
                                                                   bool with_modular_inverse) const {

src/lib/pubkey/ecc_key/ecc_key.h

@@ -157,6 +157,7 @@ class BOTAN_PUBLIC_API(2, 0) EC_PrivateKey : public virtual EC_PublicKey,
       * Get the private key value of this key object.
       * @result the private key value of this key object
       */
+      BOTAN_DEPRECATED("Directly accessing the private key as a BigInt is deprecated")
       const BigInt& private_value() const;
 
       EC_PrivateKey(const EC_PrivateKey& other) = default;

src/tests/data/pubkey/key_encoding.vec

@@ -0,0 +1,3 @@
+
+# ECDSA secp521r1 key with short private key encoding
+Key = 3081ED020100301006072A8648CE3D020106052B810400230481D53081D20201010441C8DF8E00EAC62FA3006212108C69662B5604E14662288E07D230B023E9AC660C1D63669F756743685557145A9C6419F1AD13ECFF9B04D70FFF84FEC0EE4FFDE3FCA1818903818600040012B48CDE7F451012E9FA2230466DF5876DBE0A7C5CC874B7149DA77CC6B5BC3EA02D478A41FF7FF4966E4F79E0F2B24B9008A72F63159FF60070C4BB8E1BF05B9E019C4E61CDAAFE3D5D5302C9CCD0E253BA386D2F6C7F5FC25AC80A65FAF7DC54A6A86D385CF79147680F4863B6BFBCAAF7E7941C8AFF120DBEA2AADA12407913B0E3

src/tests/test_pubkey.cpp

@@ -907,6 +907,36 @@ class PK_API_Sign_Test : public Text_Based_Test {
 
 BOTAN_REGISTER_TEST("pubkey", "pk_api_sign", PK_API_Sign_Test);
 
+/**
+ * @brief Testing PK key decoding
+ */
+class PK_Key_Decoding_Test : public Text_Based_Test {
+   public:
+      PK_Key_Decoding_Test() : Text_Based_Test("pubkey/key_encoding.vec", "Key") {}
+
+   protected:
+      Test::Result run_one_test(const std::string&, const VarMap& vars) final {
+         const auto key = vars.get_req_bin("Key");
+
+         Test::Result result("PK Key Decoding");
+
+         try {
+            auto k = Botan::PKCS8::load_key(key);
+            result.test_success("Was able to deserialize the key");
+         } catch(Botan::Exception& e) {
+            if(std::string(e.what()).starts_with("Unknown or unavailable public key algorithm")) {
+               result.test_note("Skipping test due to to algorithm being unavailable");
+            } else {
+               result.test_failure("Failed to deserialize key", e.what());
+            }
+         }
+
+         return result;
+      }
+};
+
+BOTAN_REGISTER_TEST("pubkey", "pk_key_decoding", PK_Key_Decoding_Test);
+
 }  // namespace Botan_Tests
 
 #endif