[Security/Legacy]: Fix security findings (#3477)

trustwallet · Oct 12, 2023 · fb69cf8 · fb69cf8
1 parent 0f233e1
commit fb69cf8
Show file tree

Hide file tree

Showing 8 changed files with 1 addition and 272 deletions.
diff --git a/android/app/src/androidTest/java/com/trustwallet/core/app/utils/TestPrivateKey.kt b/android/app/src/androidTest/java/com/trustwallet/core/app/utils/TestPrivateKey.kt
@@ -80,69 +80,10 @@ class TestPrivateKey {
         assertNotNull(privateKey)
     }
 
-    @Test
-    fun testGetSharedKey() {
-        val privateKeyData = "9cd3b16e10bd574fed3743d8e0de0b7b4e6c69f3245ab5a168ef010d22bfefa0".toHexBytes()
-        val privateKey = PrivateKey(privateKeyData)
-
-        val publicKeyData = "02a18a98316b5f52596e75bfa5ca9fa9912edd0c989b86b73d41bb64c9c6adb992".toHexBytes()
-        val publicKey = PublicKey(publicKeyData, PublicKeyType.SECP256K1)
-
-        val derivedData = privateKey.getSharedKey(publicKey, Curve.SECP256K1)
-        assertNotNull(derivedData)
-
-        assertEquals(derivedData?.toHex(), "0xef2cf705af8714b35c0855030f358f2bee356ff3579cea2607b2025d80133c3a")
-    }
-
     @Test
     fun testGetPublicKeyCoinType() {
         val privateKeyData = "afeefca74d9a325cf1d6b6911d61a65c32afa8e02bd5e78e2e4ac2910bab45f5".toHexBytes()
         val privateKey = PrivateKey(privateKeyData)
         assertEquals(privateKey.getPublicKey(CoinType.ETHEREUM).data().toHex(), "0x0499c6f51ad6f98c9c583f8e92bb7758ab2ca9a04110c0a1126ec43e5453d196c166b489a4b7c491e7688e6ebea3a71fc3a1a48d60f98d5ce84c93b65e423fde91");
     }
-
-    @Test
-    fun testGetSharedKeyWycherproof() {
-        val privateKeyData = "f4b7ff7cccc98813a69fae3df222bfe3f4e28f764bf91b4a10d8096ce446b254".toHexBytes()
-        val privateKey = PrivateKey(privateKeyData)
-
-        val publicKeyData = "02d8096af8a11e0b80037e1ee68246b5dcbb0aeb1cf1244fd767db80f3fa27da2b".toHexBytes()
-        val publicKey = PublicKey(publicKeyData, PublicKeyType.SECP256K1)
-
-        val derivedData = privateKey.getSharedKey(publicKey, Curve.SECP256K1)
-        assertNotNull(derivedData)
-
-        assertEquals(derivedData?.toHex(), "0x81165066322732362ca5d3f0991d7f1f7d0aad7ea533276496785d369e35159a")
-    }
-
-    @Test
-    fun testGetSharedKeyBidirectional() {
-        val privateKeyData1 = "9cd3b16e10bd574fed3743d8e0de0b7b4e6c69f3245ab5a168ef010d22bfefa0".toHexBytes()
-        val privateKey1 = PrivateKey(privateKeyData1)
-        val publicKey1 = privateKey1.getPublicKeySecp256k1(true)
-
-        val privateKeyData2 = "ef2cf705af8714b35c0855030f358f2bee356ff3579cea2607b2025d80133c3a".toHexBytes()
-        val privateKey2 = PrivateKey(privateKeyData2)
-        val publicKey2 = privateKey2.getPublicKeySecp256k1(true)
-
-        val derivedData1 = privateKey1.getSharedKey(publicKey2, Curve.SECP256K1)
-        assertNotNull(derivedData1)
-
-        val derivedData2 = privateKey2.getSharedKey(publicKey1, Curve.SECP256K1)
-        assertNotNull(derivedData2)
-
-        assertEquals(derivedData1?.toHex(), derivedData2?.toHex())
-    }
-
-    @Test
-    fun testGetSharedKeyError() {
-        val privateKeyData = "9cd3b16e10bd574fed3743d8e0de0b7b4e6c69f3245ab5a168ef010d22bfefa0".toHexBytes()
-        val privateKey = PrivateKey(privateKeyData)
-
-        val publicKeyData = "02a18a98316b5f52596e75bfa5ca9fa9912edd0c989b86b73d41bb64c9c6adb992".toHexBytes()
-        val publicKey = PublicKey(publicKeyData, PublicKeyType.SECP256K1)
-
-        val derivedData = privateKey.getSharedKey(publicKey, Curve.ED25519)
-        assertNull(derivedData)
-    }    
 }
diff --git a/include/TrustWalletCore/TWPrivateKey.h b/include/TrustWalletCore/TWPrivateKey.h
@@ -123,16 +123,6 @@ struct TWPublicKey* _Nonnull TWPrivateKeyGetPublicKeyEd25519Cardano(struct TWPri
 TW_EXPORT_METHOD
 struct TWPublicKey* _Nonnull TWPrivateKeyGetPublicKeyCurve25519(struct TWPrivateKey* _Nonnull pk);
 
-/// Computes an EC Diffie-Hellman secret in constant time
-/// Supported curves: secp256k1
-///
-/// \param pk Non-null pointer to a Private key
-/// \param publicKey Non-null pointer to the corresponding public key
-/// \param curve Eliptic curve
-/// \return The corresponding shared key as a non-null block of data
-TW_EXPORT_METHOD
-TWData* _Nullable TWPrivateKeyGetSharedKey(const struct TWPrivateKey* _Nonnull pk, const struct TWPublicKey* _Nonnull publicKey, enum TWCurve curve);
-
 /// Signs a digest using ECDSA and given curve.
 ///
 /// \param pk  Non-null pointer to a Private key

diff --git a/src/PrivateKey.cpp b/src/PrivateKey.cpp
@@ -172,24 +172,6 @@ PublicKey PrivateKey::getPublicKey(TWPublicKeyType type) const {
     return PublicKey(result, type);
 }
 
-Data PrivateKey::getSharedKey(const PublicKey& pubKey, TWCurve curve) const {
-    if (curve != TWCurveSECP256k1) {
-        return {};
-    }
-
-    Data result(PublicKey::secp256k1ExtendedSize);
-    bool success = ecdh_multiply(&secp256k1, key().data(),
-                                 pubKey.bytes.data(), result.data()) == 0;
-
-    if (success) {
-        PublicKey sharedKey(result, TWPublicKeyTypeSECP256k1Extended);
-        auto hash = Hash::sha256(sharedKey.compressed().bytes);
-        return hash;
-    }
-
-    return {};
-}
-
 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) {
         return -1;
@@ -308,5 +290,5 @@ Data PrivateKey::signZilliqa(const Data& message) const {
 }
 
 void PrivateKey::cleanup() {
-    std::fill(bytes.begin(), bytes.end(), 0);
+    memzero(bytes.data(), bytes.size());
 }
diff --git a/src/PrivateKey.h b/src/PrivateKey.h
@@ -65,10 +65,6 @@ class PrivateKey {
     /// Returns the public key for this private key.
     PublicKey getPublicKey(enum TWPublicKeyType type) const;
 
-    /// Computes an EC Diffie-Hellman secret in constant time
-    /// Supported curves: secp256k1
-    Data getSharedKey(const PublicKey& publicKey, TWCurve curve) const;
-
     /// Signs a digest using the given ECDSA curve.
     Data sign(const Data& digest, TWCurve curve) const;
 

diff --git a/src/interface/TWPrivateKey.cpp b/src/interface/TWPrivateKey.cpp
@@ -90,15 +90,6 @@ struct TWPublicKey *_Nonnull TWPrivateKeyGetPublicKeyCurve25519(struct TWPrivate
     return TWPrivateKeyGetPublicKeyByType(pk, TWPublicKeyTypeCURVE25519);
 }
 
-TWData *_Nullable TWPrivateKeyGetSharedKey(const struct TWPrivateKey *_Nonnull pk, const struct TWPublicKey *_Nonnull publicKey, enum TWCurve curve) {
-    auto result = pk->impl.getSharedKey(publicKey->impl, curve);
-    if (result.empty()) {
-        return nullptr;
-    } else {
-        return TWDataCreateWithBytes(result.data(), result.size());
-    }
-}
-
 TWData *TWPrivateKeySign(struct TWPrivateKey *_Nonnull pk, TWData *_Nonnull digest, enum TWCurve curve) {
     const auto& d = *reinterpret_cast<const Data*>(digest);
     auto result = pk->impl.sign(d, curve);

diff --git a/swift/Tests/PrivateKeyTests.swift b/swift/Tests/PrivateKeyTests.swift
@@ -51,44 +51,6 @@ class PrivateKeyTests: XCTestCase {
         XCTAssertEqual(publicKey.data.hexString, "0499c6f51ad6f98c9c583f8e92bb7758ab2ca9a04110c0a1126ec43e5453d196c166b489a4b7c491e7688e6ebea3a71fc3a1a48d60f98d5ce84c93b65e423fde91")
     }
 
-    func testGetSharedKey() {
-        let privateKey = PrivateKey(data: Data(hexString: "9cd3b16e10bd574fed3743d8e0de0b7b4e6c69f3245ab5a168ef010d22bfefa0")!)!
-        let publicKey = PublicKey(data: Data(hexString: "02a18a98316b5f52596e75bfa5ca9fa9912edd0c989b86b73d41bb64c9c6adb992")!, type: .secp256k1)!
-
-        let derivedData = privateKey.getSharedKey(publicKey: publicKey, curve: .secp256k1)!
-
-        XCTAssertEqual(derivedData.hexString, "ef2cf705af8714b35c0855030f358f2bee356ff3579cea2607b2025d80133c3a")
-    }
-
-    func testGetSharedKeyWycherproof() {
-        let privateKey = PrivateKey(data: Data(hexString: "f4b7ff7cccc98813a69fae3df222bfe3f4e28f764bf91b4a10d8096ce446b254")!)!
-        let publicKey = PublicKey(data: Data(hexString: "02d8096af8a11e0b80037e1ee68246b5dcbb0aeb1cf1244fd767db80f3fa27da2b")!, type: .secp256k1)!
-
-        let derivedData = privateKey.getSharedKey(publicKey: publicKey, curve: .secp256k1)!
-
-        XCTAssertEqual(derivedData.hexString, "81165066322732362ca5d3f0991d7f1f7d0aad7ea533276496785d369e35159a")
-    }    
-
-    func testGetSharedKeyBidirectional() {
-        let privateKey1 = PrivateKey(data: Data(hexString: "9cd3b16e10bd574fed3743d8e0de0b7b4e6c69f3245ab5a168ef010d22bfefa0")!)!
-        let publicKey1 = privateKey1.getPublicKeySecp256k1(compressed: false)
-        let privateKey2 = PrivateKey(data: Data(hexString: "ef2cf705af8714b35c0855030f358f2bee356ff3579cea2607b2025d80133c3a")!)!
-        let publicKey2 = privateKey2.getPublicKeySecp256k1(compressed: false)
-
-        let derivedData1 = privateKey1.getSharedKey(publicKey: publicKey2, curve: .secp256k1)!
-        let derivedData2 = privateKey2.getSharedKey(publicKey: publicKey1, curve: .secp256k1)!
-
-        XCTAssertEqual(derivedData1.hexString, derivedData2.hexString)
-    }
-
-    func testGetSharedKeyError() {
-        let privateKey = PrivateKey(data: Data(hexString: "9cd3b16e10bd574fed3743d8e0de0b7b4e6c69f3245ab5a168ef010d22bfefa0")!)!
-        let publicKey = PublicKey(data: Data(hexString: "02a18a98316b5f52596e75bfa5ca9fa9912edd0c989b86b73d41bb64c9c6adb992")!, type: .secp256k1)!
-
-        let derivedData = privateKey.getSharedKey(publicKey: publicKey, curve: .ed25519)
-        XCTAssertNil(derivedData)
-    }
-
     func testSignSchnorr() {
         let privateKey = PrivateKey(data: Data(hexString: "afeefca74d9a325cf1d6b6911d61a65c32afa8e02bd5e78e2e4ac2910bab45f5")!)!
         let publicKey = privateKey.getPublicKeySecp256k1(compressed: true)

diff --git a/tests/common/PrivateKeyTests.cpp b/tests/common/PrivateKeyTests.cpp
@@ -221,77 +221,6 @@ TEST(PrivateKey, PrivateKeyExtendedError) {
     FAIL() << "Should throw Invalid empty key extension";
 }
 
-TEST(PrivateKey, getSharedKey) {
-    Data privKeyData = parse_hex("9cd3b16e10bd574fed3743d8e0de0b7b4e6c69f3245ab5a168ef010d22bfefa0");
-    EXPECT_TRUE(PrivateKey::isValid(privKeyData, TWCurveSECP256k1));
-    auto privateKey = PrivateKey(privKeyData);
-
-    const Data pubKeyData = parse_hex("02a18a98316b5f52596e75bfa5ca9fa9912edd0c989b86b73d41bb64c9c6adb992");
-    EXPECT_TRUE(PublicKey::isValid(pubKeyData, TWPublicKeyTypeSECP256k1));
-    PublicKey publicKey(pubKeyData, TWPublicKeyTypeSECP256k1);
-    EXPECT_TRUE(publicKey.isCompressed());
-
-    const Data derivedKeyData = privateKey.getSharedKey(publicKey, TWCurveSECP256k1);
-
-    EXPECT_EQ(
-        "ef2cf705af8714b35c0855030f358f2bee356ff3579cea2607b2025d80133c3a",
-        hex(derivedKeyData));
-}
-
-/**
- * Valid test vector from Wycherproof project
- * Source: https://github.com/google/wycheproof/blob/master/testvectors/ecdh_secp256k1_test.json#L31
- */
-TEST(PrivateKey, getSharedKeyWycherproof) {
-    // Stripped left-padded zeroes from: `00f4b7ff7cccc98813a69fae3df222bfe3f4e28f764bf91b4a10d8096ce446b254`
-    Data privKeyData = parse_hex("f4b7ff7cccc98813a69fae3df222bfe3f4e28f764bf91b4a10d8096ce446b254");
-    EXPECT_TRUE(PrivateKey::isValid(privKeyData, TWCurveSECP256k1));
-    auto privateKey = PrivateKey(privKeyData);
-
-    // Decoded from ASN.1 & uncompressed `3056301006072a8648ce3d020106052b8104000a03420004d8096af8a11e0b80037e1ee68246b5dcbb0aeb1cf1244fd767db80f3fa27da2b396812ea1686e7472e9692eaf3e958e50e9500d3b4c77243db1f2acd67ba9cc4`
-    const Data pubKeyData = parse_hex("02d8096af8a11e0b80037e1ee68246b5dcbb0aeb1cf1244fd767db80f3fa27da2b");
-    EXPECT_TRUE(PublicKey::isValid(pubKeyData, TWPublicKeyTypeSECP256k1));
-    PublicKey publicKey(pubKeyData, TWPublicKeyTypeSECP256k1);
-    EXPECT_TRUE(publicKey.isCompressed());
-
-    const Data derivedKeyData = privateKey.getSharedKey(publicKey, TWCurveSECP256k1);
-
-    // SHA-256 of encoded x-coordinate `02544dfae22af6af939042b1d85b71a1e49e9a5614123c4d6ad0c8af65baf87d65`
-    EXPECT_EQ(
-        "81165066322732362ca5d3f0991d7f1f7d0aad7ea533276496785d369e35159a",
-        hex(derivedKeyData));
-}
-
-TEST(PrivateKey, getSharedKeyBidirectional) {
-    Data privKeyData1 = parse_hex("9cd3b16e10bd574fed3743d8e0de0b7b4e6c69f3245ab5a168ef010d22bfefa0");
-    EXPECT_TRUE(PrivateKey::isValid(privKeyData1, TWCurveSECP256k1));
-    auto privateKey1 = PrivateKey(privKeyData1);
-    auto publicKey1 = privateKey1.getPublicKey(TWPublicKeyTypeSECP256k1);
-
-    Data privKeyData2 = parse_hex("ef2cf705af8714b35c0855030f358f2bee356ff3579cea2607b2025d80133c3a");
-    EXPECT_TRUE(PrivateKey::isValid(privKeyData2, TWCurveSECP256k1));
-    auto privateKey2 = PrivateKey(privKeyData2);
-    auto publicKey2 = privateKey2.getPublicKey(TWPublicKeyTypeSECP256k1);
-
-    const Data derivedKeyData1 = privateKey1.getSharedKey(publicKey2, TWCurveSECP256k1);
-    const Data derivedKeyData2 = privateKey2.getSharedKey(publicKey1, TWCurveSECP256k1);
-
-    EXPECT_EQ(hex(derivedKeyData1), hex(derivedKeyData2));
-}
-
-TEST(PrivateKey, getSharedKeyError) {
-    Data privKeyData = parse_hex("9cd3b16e10bd574fed3743d8e0de0b7b4e6c69f3245ab5a168ef010d22bfefa0");
-    auto privateKey = PrivateKey(privKeyData);
-
-    const Data pubKeyData = parse_hex("02a18a98316b5f52596e75bfa5ca9fa9912edd0c989b86b73d41bb64c9c6adb992");
-    PublicKey publicKey(pubKeyData, TWPublicKeyTypeSECP256k1);
-
-    const Data derivedKeyData = privateKey.getSharedKey(publicKey, TWCurveCurve25519);
-    const Data expected = {};
-
-    EXPECT_EQ(expected, derivedKeyData);
-}
-
 TEST(PrivateKey, SignSECP256k1) {
     Data privKeyData = parse_hex("afeefca74d9a325cf1d6b6911d61a65c32afa8e02bd5e78e2e4ac2910bab45f5");
     auto privateKey = PrivateKey(privKeyData);

diff --git a/tests/interface/TWPrivateKeyTests.cpp b/tests/interface/TWPrivateKeyTests.cpp
@@ -102,68 +102,6 @@ TEST(TWPrivateKeyTests, PublicKey) {
     }
 }
 
-TEST(TWPrivateKeyTests, GetSharedKey) {
-    const auto privateKeyHex = "9cd3b16e10bd574fed3743d8e0de0b7b4e6c69f3245ab5a168ef010d22bfefa0";
-    const auto privateKey = WRAP(TWPrivateKey, TWPrivateKeyCreateWithData(DATA(privateKeyHex).get()));
-    ASSERT_TRUE(privateKey.get() != nullptr);
-
-    const auto publicKeyHex = "02a18a98316b5f52596e75bfa5ca9fa9912edd0c989b86b73d41bb64c9c6adb992";
-    const auto publicKey = WRAP(TWPublicKey, TWPublicKeyCreateWithData(DATA(publicKeyHex).get(), TWPublicKeyTypeSECP256k1));
-    EXPECT_TRUE(publicKey != nullptr);
-
-    const auto derivedData = WRAPD(TWPrivateKeyGetSharedKey(privateKey.get(), publicKey.get(), TWCurveSECP256k1));
-    ASSERT_EQ(TW::hex(*((TW::Data*)derivedData.get())), "ef2cf705af8714b35c0855030f358f2bee356ff3579cea2607b2025d80133c3a");
-}
-
-/**
- * Valid test vector from Wycherproof project
- * Source: https://github.com/google/wycheproof/blob/master/testvectors/ecdh_secp256k1_test.json#L31
- */
-TEST(TWPrivateKeyTests, GetSharedKeyWycherproof) {
-    // Stripped left-padded zeroes from: `00f4b7ff7cccc98813a69fae3df222bfe3f4e28f764bf91b4a10d8096ce446b254`
-    const auto privateKeyHex = "f4b7ff7cccc98813a69fae3df222bfe3f4e28f764bf91b4a10d8096ce446b254";
-    const auto privateKey = WRAP(TWPrivateKey, TWPrivateKeyCreateWithData(DATA(privateKeyHex).get()));
-    ASSERT_TRUE(privateKey.get() != nullptr);
-
-    // Decoded from ASN.1 & uncompressed `3056301006072a8648ce3d020106052b8104000a03420004d8096af8a11e0b80037e1ee68246b5dcbb0aeb1cf1244fd767db80f3fa27da2b396812ea1686e7472e9692eaf3e958e50e9500d3b4c77243db1f2acd67ba9cc4`
-    const auto publicKeyHex = "02d8096af8a11e0b80037e1ee68246b5dcbb0aeb1cf1244fd767db80f3fa27da2b";
-    const auto publicKey = WRAP(TWPublicKey, TWPublicKeyCreateWithData(DATA(publicKeyHex).get(), TWPublicKeyTypeSECP256k1));
-    EXPECT_TRUE(publicKey != nullptr);
-
-    // SHA-256 of encoded x-coordinate `02544dfae22af6af939042b1d85b71a1e49e9a5614123c4d6ad0c8af65baf87d65`
-    const auto derivedData = WRAPD(TWPrivateKeyGetSharedKey(privateKey.get(), publicKey.get(), TWCurveSECP256k1));
-    ASSERT_EQ(TW::hex(*((TW::Data*)derivedData.get())), "81165066322732362ca5d3f0991d7f1f7d0aad7ea533276496785d369e35159a");
-}
-
-TEST(TWPrivateKeyTests, GetSharedKeyBidirectional) {
-    const auto privateKeyHex1 = "9cd3b16e10bd574fed3743d8e0de0b7b4e6c69f3245ab5a168ef010d22bfefa0";
-    const auto privateKey1 = WRAP(TWPrivateKey, TWPrivateKeyCreateWithData(DATA(privateKeyHex1).get()));
-    ASSERT_TRUE(privateKey1.get() != nullptr);
-    auto publicKey1 = WRAP(TWPublicKey, TWPrivateKeyGetPublicKeySecp256k1(privateKey1.get(), true));
-
-    const auto privateKeyHex2 = "ef2cf705af8714b35c0855030f358f2bee356ff3579cea2607b2025d80133c3a";
-    const auto privateKey2 = WRAP(TWPrivateKey, TWPrivateKeyCreateWithData(DATA(privateKeyHex2).get()));
-    ASSERT_TRUE(privateKey2.get() != nullptr);
-    auto publicKey2 = WRAP(TWPublicKey, TWPrivateKeyGetPublicKeySecp256k1(privateKey2.get(), true));
-
-    const auto derivedData1 = WRAPD(TWPrivateKeyGetSharedKey(privateKey1.get(), publicKey2.get(), TWCurveSECP256k1));
-    const auto derivedData2 = WRAPD(TWPrivateKeyGetSharedKey(privateKey2.get(), publicKey1.get(), TWCurveSECP256k1));
-    ASSERT_EQ(TW::hex(*((TW::Data*)derivedData1.get())), TW::hex(*((TW::Data*)derivedData2.get())));
-}
-
-TEST(TWPrivateKeyTests, GetSharedKeyError) {
-    const auto privateKeyHex = "9cd3b16e10bd574fed3743d8e0de0b7b4e6c69f3245ab5a168ef010d22bfefa0";
-    const auto privateKey = WRAP(TWPrivateKey, TWPrivateKeyCreateWithData(DATA(privateKeyHex).get()));
-    ASSERT_TRUE(privateKey.get() != nullptr);
-
-    const auto publicKeyHex = "02a18a98316b5f52596e75bfa5ca9fa9912edd0c989b86b73d41bb64c9c6adb992";
-    const auto publicKey = WRAP(TWPublicKey, TWPublicKeyCreateWithData(DATA(publicKeyHex).get(), TWPublicKeyTypeSECP256k1));
-    EXPECT_TRUE(publicKey != nullptr);
-
-    const auto derivedData = WRAPD(TWPrivateKeyGetSharedKey(privateKey.get(), publicKey.get(), TWCurveED25519));
-    EXPECT_TRUE(derivedData == nullptr);
-}
-
 TEST(TWPrivateKeyTests, Sign) {
     const auto privateKey = WRAP(TWPrivateKey, TWPrivateKeyCreateWithData(DATA("afeefca74d9a325cf1d6b6911d61a65c32afa8e02bd5e78e2e4ac2910bab45f5").get()));