diff --git a/src/PrivateKey.cpp b/src/PrivateKey.cpp index 5f3dfa79454..55192171203 100644 --- a/src/PrivateKey.cpp +++ b/src/PrivateKey.cpp @@ -256,10 +256,9 @@ PublicKey PrivateKey::getPublicKey(TWPublicKeyType type) const { } int ecdsa_sign_digest_checked(const ecdsa_curve* curve, const uint8_t* priv_key, const uint8_t* digest, size_t digest_size, uint8_t* sig, uint8_t* pby, int (*is_canonical)(uint8_t by, uint8_t sig[64])) { - if (digest_size < 32) { + if (digest_size != PrivateKey::ecdsaMessageSize) { return -1; } - assert(digest_size >= 32); return ecdsa_sign_digest(curve, priv_key, digest, sig, pby, is_canonical); } @@ -371,7 +370,7 @@ Data PrivateKey::signAsDER(const Data& digest) const { } Data sig(64); bool success = - ecdsa_sign_digest(&secp256k1, key().data(), digest.data(), sig.data(), nullptr, nullptr) == 0; + ecdsa_sign_digest_checked(&secp256k1, key().data(), digest.data(), digest.size(), sig.data(), nullptr, nullptr) == 0; if (!success) { return {}; } diff --git a/src/PrivateKey.h b/src/PrivateKey.h index 4e59e95207b..2afad32605a 100644 --- a/src/PrivateKey.h +++ b/src/PrivateKey.h @@ -20,6 +20,8 @@ class PrivateKey { static const size_t _size = 32; /// The number of bytes in a Cardano key (two extended ed25519 keys + chain code) static const size_t cardanoKeySize = 2 * 3 * 32; + /// The number of bytes in an ECDSA message digest (e.g., 32-byte hash) that can be signed. + static const size_t ecdsaMessageSize = 32; /// The private key bytes: /// - common case: 'size' bytes diff --git a/src/PublicKey.cpp b/src/PublicKey.cpp index d6b3c715b01..9cee86379ed 100644 --- a/src/PublicKey.cpp +++ b/src/PublicKey.cpp @@ -20,7 +20,7 @@ namespace TW { -bool validateSignatureLength(TWPublicKeyType type, const Data& signature) { +static bool validateSignatureLength(TWPublicKeyType type, const Data& signature) { switch (type) { case TWPublicKeyTypeSECP256k1: case TWPublicKeyTypeSECP256k1Extended: @@ -35,6 +35,27 @@ bool validateSignatureLength(TWPublicKeyType type, const Data& signature) { } } +static bool validateMessageLength(TWPublicKeyType type, const Data& message) { + switch (type) { + case TWPublicKeyTypeED25519: + case TWPublicKeyTypeCURVE25519: + case TWPublicKeyTypeED25519Blake2b: + case TWPublicKeyTypeED25519Cardano: + // Allow any message size for ed25519. + return true; + case TWPublicKeyTypeSECP256k1: + case TWPublicKeyTypeNIST256p1: + case TWPublicKeyTypeSECP256k1Extended: + case TWPublicKeyTypeNIST256p1Extended: + return message.size() == PublicKey::ecdsaMessageSize; + case TWPublicKeyTypeStarkex: + // Digest shorter than 32 bytes will be left-padded with zeros before verification. + return message.size() <= PublicKey::starkexMessageMaxSize; + default: + return false; + } +} + /// Determines if a collection of bytes makes a valid public key of the /// given type. bool PublicKey::isValid(const Data& data, enum TWPublicKeyType type) { @@ -165,6 +186,9 @@ bool PublicKey::verify(const Data& signature, const Data& message) const { if (!validateSignatureLength(type, signature)) { return false; } + if (!validateMessageLength(type, message)) { + return false; + } switch (type) { case TWPublicKeyTypeSECP256k1: @@ -183,7 +207,7 @@ bool PublicKey::verify(const Data& signature, const Data& message) const { } case TWPublicKeyTypeCURVE25519: { auto ed25519PublicKey = Data(); - ed25519PublicKey.resize(PublicKey::ed25519Size); + ed25519PublicKey.resize(ed25519Size); curve25519_pk_to_ed25519(ed25519PublicKey.data(), bytes.data()); ed25519PublicKey[31] &= 0x7F; @@ -203,6 +227,13 @@ bool PublicKey::verify(const Data& signature, const Data& message) const { } bool PublicKey::verifyAsDER(const Data& signature, const Data& message) const { + if (signature.size() < derSignatureMinSize || signature.size() > derSignatureMaxSize) { + return false; + } + if (message.size() != ecdsaMessageSize) { + return false; + } + switch (type) { case TWPublicKeyTypeSECP256k1: case TWPublicKeyTypeSECP256k1Extended: { diff --git a/src/PublicKey.h b/src/PublicKey.h index 6ea239c96b1..d116be3ae0a 100644 --- a/src/PublicKey.h +++ b/src/PublicKey.h @@ -37,9 +37,20 @@ class PublicKey { /// The number of bytes in a secp256k1 signature. static const size_t secp256k1SignatureSize = 65; - /// Magic number used in V compnent encoding + /// Magic number used in V component encoding static const byte SignatureVOffset = 27; + /// The number of bytes in an ECDSA message digest (e.g., 32-byte hash) that can be verified. + static const size_t ecdsaMessageSize = 32; + /// The maximum number of bytes in a message that can be verified with a starkex public key. + /// Digest shorter than 32 bytes will be left-padded with zeros before verification. + static const size_t starkexMessageMaxSize = 32; + + /// The minimum number of bytes in a valid DER-encoded ECDSA signature. + static const size_t derSignatureMinSize = 2; + /// The maximum number of bytes in a valid DER-encoded ECDSA signature. + static const size_t derSignatureMaxSize = 72; + /// The public key bytes. Data bytes; diff --git a/tests/common/PrivateKeyTests.cpp b/tests/common/PrivateKeyTests.cpp index fccfebcff6f..6eb97f5e436 100644 --- a/tests/common/PrivateKeyTests.cpp +++ b/tests/common/PrivateKeyTests.cpp @@ -323,6 +323,24 @@ TEST(PrivateKey, SignShortDigest) { } } +TEST(PrivateKey, SignLongDigest) { + Data privKeyData = parse_hex("afeefca74d9a325cf1d6b6911d61a65c32afa8e02bd5e78e2e4ac2910bab45f5"); + auto privateKey = PrivateKey(privKeyData); + Data shortDigest = TW::data("123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef012"); + { + Data actual = privateKey.sign(shortDigest, TWCurveSECP256k1); + EXPECT_EQ(actual.size(), 0ul); + } + { + Data actual = privateKey.sign(shortDigest, TWCurveNIST256p1); + EXPECT_EQ(actual.size(), 0ul); + } + { + Data actual = privateKey.sign(shortDigest, TWCurveSECP256k1, isCanonical); + EXPECT_EQ(actual.size(), 0ul); + } +} + TEST(PrivateKey, SignWithDifferentCurveWorks) { Data privKeyData = parse_hex("afeefca74d9a325cf1d6b6911d61a65c32afa8e02bd5e78e2e4ac2910bab45f5"); // Using the deprecated constructor without specifying a curve @@ -372,4 +390,10 @@ TEST(PrivateKey, SignWithDifferentCurveThrows) { } } +TEST(PrivateKey, SignAsDERRejectsShortDigest) { + const auto privateKey = PrivateKey(parse_hex("afeefca74d9a325cf1d6b6911d61a65c32afa8e02bd5e78e2e4ac2910bab45f5")); + const auto shortDigest = parse_hex("0102030405060708"); + EXPECT_TRUE(privateKey.signAsDER(shortDigest).empty()); +} + } // namespace TW::tests diff --git a/tests/common/PublicKeyTests.cpp b/tests/common/PublicKeyTests.cpp index d69e1bc7952..c903f66a054 100644 --- a/tests/common/PublicKeyTests.cpp +++ b/tests/common/PublicKeyTests.cpp @@ -366,3 +366,37 @@ TEST(PublicKeyTests, isValidED25519) { EXPECT_FALSE(PublicKey::isValid(parse_hex("0101beff0e5d6f6e6e6d573d3044f3e2bfb353400375dc281da3337468d4aa527908"), TWPublicKeyTypeED25519)); EXPECT_FALSE(PublicKey(parse_hex("0399c6f51ad6f98c9c583f8e92bb7758ab2ca9a04110c0a1126ec43e5453d196c1"), TWPublicKeyTypeSECP256k1).isValidED25519()); } + +TEST(PublicKeyTests, VerifyRejectsShortDigest) { + const auto publicKey = PublicKey(parse_hex("0399c6f51ad6f98c9c583f8e92bb7758ab2ca9a04110c0a1126ec43e5453d196c1"), TWPublicKeyTypeSECP256k1); + const auto validSig = parse_hex("0f5d5a9e5fc4b82a625312f3be5d3e8ad017d882de86c72c92fcefa924e894c12071772a14201a3a0debf381b5e8dea39fadb9bcabdc02ee71ab018f55bf717f"); + const auto shortDigest = parse_hex("0102030405060708"); + + EXPECT_FALSE(publicKey.verify(validSig, shortDigest)); +} + +TEST(PublicKeyTests, VerifyAsDERRejectsShortDigest) { + const auto publicKey = PublicKey(parse_hex("0399c6f51ad6f98c9c583f8e92bb7758ab2ca9a04110c0a1126ec43e5453d196c1"), TWPublicKeyTypeSECP256k1); + const auto derSig = parse_hex("304402200f5d5a9e5fc4b82a625312f3be5d3e8ad017d882de86c72c92fcefa924e894c102202071772a14201a3a0debf381b5e8dea39fadb9bcabdc02ee71ab018f55bf717f"); + const auto shortDigest = parse_hex("0102030405060708"); + + EXPECT_FALSE(publicKey.verifyAsDER(derSig, shortDigest)); +} + +TEST(PublicKeyTests, VerifyAsDERRejectsInvalidSignatureSize) { + const auto publicKey = PublicKey(parse_hex("0399c6f51ad6f98c9c583f8e92bb7758ab2ca9a04110c0a1126ec43e5453d196c1"), TWPublicKeyTypeSECP256k1); + const auto digest = parse_hex("afeefca74d9a325cf1d6b6911d61a65c32afa8e02bd5e78e2e4ac2910bab45f5"); + + EXPECT_FALSE(publicKey.verifyAsDER(Data(), digest)); + EXPECT_FALSE(publicKey.verifyAsDER(parse_hex("30"), digest)); + EXPECT_FALSE(publicKey.verifyAsDER(Data(73, 0x30), digest)); +} + +TEST(PublicKeyTests, VerifyNist256p1RejectsShortDigest) { + const auto privateKey = PrivateKey(parse_hex("afeefca74d9a325cf1d6b6911d61a65c32afa8e02bd5e78e2e4ac2910bab45f5"), TWCurveNIST256p1); + const auto publicKey = privateKey.getPublicKey(TWPublicKeyTypeNIST256p1); + const auto validSig = parse_hex("0f5d5a9e5fc4b82a625312f3be5d3e8ad017d882de86c72c92fcefa924e894c12071772a14201a3a0debf381b5e8dea39fadb9bcabdc02ee71ab018f55bf717f"); + const auto shortDigest = parse_hex("0102030405060708"); + + EXPECT_FALSE(publicKey.verify(validSig, shortDigest)); +} diff --git a/tests/interface/TWPublicKeyTests.cpp b/tests/interface/TWPublicKeyTests.cpp index 7aa372ffc80..7f567ba3494 100644 --- a/tests/interface/TWPublicKeyTests.cpp +++ b/tests/interface/TWPublicKeyTests.cpp @@ -91,6 +91,19 @@ TEST(TWPublicKeyTests, VerifyInvalidLength) { ASSERT_FALSE(TWPublicKeyVerify(publicKey.get(), signature.get(), message.get())); } +TEST(TWPublicKeyTests, VerifyInvalidMessageLength) { + const PrivateKey key(parse_hex("afeefca74d9a325cf1d6b6911d61a65c32afa8e02bd5e78e2e4ac2910bab45f5"), TWCurveSECP256k1); + const auto privateKey = WRAP(TWPrivateKey, new TWPrivateKey{ key }); + + // Invalid message length - 31 bytes instead of 32 + const auto invalidMessage = DATA("de4e9524586d6fce45667f9ff12f661e79870c4105fa0fb58af976619bb114"); + // Valid 64-byte signature + const auto signature = DATA("00000000000000000000000000000000000000000000000000000000000000020123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef80"); + + auto publicKey = WRAP(TWPublicKey, TWPrivateKeyGetPublicKeySecp256k1(privateKey.get(), false)); + ASSERT_FALSE(TWPublicKeyVerify(publicKey.get(), signature.get(), invalidMessage.get())); +} + TEST(TWPublicKeyTests, VerifyAsDER) { const PrivateKey key = PrivateKey(parse_hex("afeefca74d9a325cf1d6b6911d61a65c32afa8e02bd5e78e2e4ac2910bab45f5"), TWCurveSECP256k1); const auto privateKey = WRAP(TWPrivateKey, new TWPrivateKey{ key }); @@ -108,6 +121,19 @@ TEST(TWPublicKeyTests, VerifyAsDER) { ASSERT_FALSE(TWPublicKeyVerify(publicKey.get(), signature.get(), digest.get())); } +TEST(TWPublicKeyTests, VerifyAsDERInvalidMessageLength) { + const PrivateKey key = PrivateKey(parse_hex("afeefca74d9a325cf1d6b6911d61a65c32afa8e02bd5e78e2e4ac2910bab45f5"), TWCurveSECP256k1); + const auto privateKey = WRAP(TWPrivateKey, new TWPrivateKey{ key }); + + // Invalid message length - 31 bytes instead of 32 + const auto invalidMessage = DATA("de4e9524586d6fce45667f9ff12f661e79870c4105fa0fb58af976619bb114"); + // Valid 64-byte signature + const auto signature = DATA("00000000000000000000000000000000000000000000000000000000000000020123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef80"); + + auto publicKey = WRAP(TWPublicKey, TWPrivateKeyGetPublicKeySecp256k1(privateKey.get(), false)); + ASSERT_FALSE(TWPublicKeyVerifyAsDER(publicKey.get(), signature.get(), invalidMessage.get())); +} + TEST(TWPublicKeyTests, VerifyEd25519) { const PrivateKey key(parse_hex("afeefca74d9a325cf1d6b6911d61a65c32afa8e02bd5e78e2e4ac2910bab45f5")); const auto privateKey = WRAP(TWPrivateKey, new TWPrivateKey{ key });