security(twdata): Zeroize every TWData instance in TWDataDelete, and other TWData improvements

include/TrustWalletCore/TWData.h

@@ -53,13 +53,6 @@ size_t TWDataSize(TWData *_Nonnull data) TW_VISIBILITY_DEFAULT;
 /// \return the raw pointer to the contents of data
 uint8_t *_Nonnull TWDataBytes(TWData *_Nonnull data) TW_VISIBILITY_DEFAULT;
 
-/// Returns the byte at the provided index.
-///
-/// \param data A non-null valid block of data
-/// \param index index of the byte that we want to fetch - index need to be < TWDataSize(data)
-/// \return the byte at the provided index
-uint8_t TWDataGet(TWData *_Nonnull data, size_t index) TW_VISIBILITY_DEFAULT;
-
 /// Sets the byte at the provided index.
 ///
 /// \param data A non-null valid block of data

src/interface/TWData.cpp

@@ -4,6 +4,7 @@
 
 #include <TrustWalletCore/TWData.h>
 #include <TrustWalletCore/TWString.h>
+#include <TrezorCrypto/memzero.h>
 #include "Data.h"
 #include "HexCoding.h"
 #include <algorithm>
@@ -47,23 +48,29 @@ uint8_t *_Nonnull TWDataBytes(TWData *_Nonnull data) {
     return v->data();
 }
 
-uint8_t TWDataGet(TWData *_Nonnull data, size_t index) {
-    auto* v = reinterpret_cast<const Data*>(data);
-    return (*v)[index];
-}
-
 void TWDataSet(TWData *_Nonnull data, size_t index, uint8_t byte) {
     auto* v = const_cast<Data*>(reinterpret_cast<const Data*>(data));
+    if (v->size() <= index) {
+        return;
+    }
     (*v)[index] = byte;
 }
 
 void TWDataCopyBytes(TWData *_Nonnull data, size_t start, size_t size, uint8_t *_Nonnull output) {
     auto* v = reinterpret_cast<const Data*>(data);
+    // Check for overflow and bounds
+    if (start > v->size() || size > v->size() - start) {
+        return;
+    }
     std::copy(std::begin(*v) + start, std::begin(*v) + start + size, output);
 }
 
 void TWDataReplaceBytes(TWData *_Nonnull data, size_t start, size_t size, const uint8_t *_Nonnull bytes) {
     auto* v = const_cast<Data*>(reinterpret_cast<const Data*>(data));
+    // Check for overflow and bounds
+    if (start > v->size() || size > v->size() - start) {
+        return;
+    }
     std::copy(bytes, bytes + size, std::begin(*v) + start);
 }
 
@@ -95,8 +102,14 @@ void TWDataReset(TWData *_Nonnull data) {
 }
 
 void TWDataDelete(TWData *_Nonnull data) {
-    auto* v = reinterpret_cast<const Data*>(data);
-    delete v;
+    auto* vConst = reinterpret_cast<const Data*>(data);
+    if (!vConst->empty()) {
+        // `const_cast` is safe here despite that the pointer to the data is const
+        // but `Data` is not a constant value.
+        auto *v = const_cast<Data*>(vConst);
+        memzero(v->data(), v->size());
+    }
+    delete vConst;
 }
 
 bool TWDataEqual(TWData *_Nonnull lhs, TWData *_Nonnull rhs) {

src/interface/TWString+Hex.cpp

@@ -26,12 +26,13 @@ static inline char high_char(uint8_t v) {
 
 TWString *TWStringCreateWithHexData(TWData *_Nonnull data) {
     const size_t count = TWDataSize(data) * 2;
+    const auto *dataBytes = TWDataBytes(data);
     char *bytes = (char *)malloc(count + 1);
     bytes[count] = 0;
 
     for (size_t i = 0; i < TWDataSize(data); i += 1) {
-        bytes[2 * i] = high_char(TWDataGet(data, i));
-        bytes[2 * i + 1] = low_char(TWDataGet(data, i));
+        bytes[2 * i] = high_char(dataBytes[i]);
+        bytes[2 * i + 1] = low_char(dataBytes[i]);
     }
 
     const TWString *string = TWStringCreateWithUTF8Bytes(bytes);

src/interface/TWString.cpp

@@ -34,10 +34,12 @@ const char *_Nonnull TWStringUTF8Bytes(TWString *_Nonnull string) {
 
 void TWStringDelete(TWString *_Nonnull string) {
     auto *sConst = reinterpret_cast<const std::string*>(string);
-    // `const_cast` is safe here despite that the pointer to the string is const
-    // but `std::string` is not a constant value.
-    auto *s = const_cast<std::string*>(sConst);
-    memzero(s->data(), s->size());
+    if (!sConst->empty()) {
+        // `const_cast` is safe here despite that the pointer to the string is const
+        // but `std::string` is not a constant value.
+        auto *s = const_cast<std::string*>(sConst);
+        memzero(s->data(), s->size());
+    }
     delete sConst;
 }
 

tests/interface/TWBase32Tests.cpp

@@ -11,7 +11,7 @@
 
 TEST(TWBase32, InvalidDecode) {
     const auto encodedInput = STRING("JBSWY3DPK5XXE3DE=======");
-    auto result = WRAPD(TWBase32Decode(encodedInput.get()));
+    auto result = TWBase32Decode(encodedInput.get());
     ASSERT_EQ(result, nullptr);
 }
 

tests/interface/TWBase58Tests.cpp

@@ -40,8 +40,8 @@ TEST(TWBase58, DecodeNoCheck) {
 
 TEST(TWBase58, Decode_WrongChecksum) {
     const auto input = STRING("1Bp9U1ogV3A14FMvKbRJms7ctyso5FdSz2");
-    auto result = WRAPD(TWBase58Decode(input.get()));
-    ASSERT_EQ(result.get(), nullptr);
+    auto result = TWBase58Decode(input.get());
+    ASSERT_EQ(result, nullptr);
 }
 
 TEST(TWBase58, DecodeNoCheck_WrongChecksum) {
@@ -54,13 +54,13 @@ TEST(TWBase58, DecodeNoCheck_WrongChecksum) {
 TEST(TWBase58, Decode_InvalidChar) {
     // 0 is invalid
     const auto input = STRING("1Bp9U1ogV3A14FMvKbRJms7ctyso4Z4Tc0");
-    auto result = WRAPD(TWBase58Decode(input.get()));
-    ASSERT_EQ(result.get(), nullptr);
+    auto result = TWBase58Decode(input.get());
+    ASSERT_EQ(result, nullptr);
 }
 
 TEST(TWBase58, DecodeNoCheck_InvalidChar) {
     // 0 is invalid
     const auto input = STRING("1Bp9U1ogV3A14FMvKbRJms7ctyso4Z4Tc0");
-    auto result = WRAPD(TWBase58DecodeNoCheck(input.get()));
-    ASSERT_EQ(result.get(), nullptr);
+    auto result = TWBase58DecodeNoCheck(input.get());
+    ASSERT_EQ(result, nullptr);
 }

tests/interface/TWBech32Tests.cpp

@@ -24,8 +24,8 @@ TEST(TWBech32, Decode) {
 TEST(TWBech32, Decode_WrongChecksumVariant) {
     // This is a Bech32m variant, not Bech32 variant. So it should fail using Bech32 decoder.
     const auto input = STRING("abcdef1l7aum6echk45nj3s0wdvt2fg8x9yrzpqzd3ryx");
-    const auto result = WRAPD(TWBech32Decode(input.get()));
-    ASSERT_EQ(result.get(), nullptr);
+    const auto result = TWBech32Decode(input.get());
+    ASSERT_EQ(result, nullptr);
 }
 
 TEST(TWBech32, EncodeM) {
@@ -44,6 +44,6 @@ TEST(TWBech32, DecodeM) {
 TEST(TWBech32, DecodeM_WrongChecksumVariant) {
     // This is a Bech32 variant, not Bech32m variant. So it should fail using Bech32M decoder.
     const auto input = STRING("abcdef1qpzry9x8gf2tvdw0s3jn54khce6mua7lmqqqxw");
-    const auto result = WRAPD(TWBech32DecodeM(input.get()));
-    ASSERT_EQ(result.get(), nullptr);
+    const auto result = TWBech32DecodeM(input.get());
+    ASSERT_EQ(result, nullptr);
 }

tests/interface/TWDataTests.cpp

@@ -46,8 +46,8 @@ TEST(TWData, CreateWithHexString) {
     {
         // null input
         TWString* nullstring = nullptr;
-        const auto data = WRAPD(TWDataCreateWithHexString(nullstring));
-        ASSERT_EQ(data.get(), nullptr);
+        const auto data = TWDataCreateWithHexString(nullstring);
+        ASSERT_EQ(data, nullptr);
     }
 }
 
@@ -78,15 +78,6 @@ TEST(TWData, Delete) {
     TWDataDelete(data);
 }
 
-TEST(TWData, Get) {
-    const auto data = WRAPD(TWDataCreateWithHexString(STRING("deadbeef").get()));
-    assertHexEqual(data, "deadbeef");
-    EXPECT_EQ(TWDataGet(data.get(), 0), 0xde);
-    EXPECT_EQ(TWDataGet(data.get(), 1), 0xad);
-    EXPECT_EQ(TWDataGet(data.get(), 2), 0xbe);
-    EXPECT_EQ(TWDataGet(data.get(), 3), 0xef);
-}
-
 TEST(TWData, Set) {
     const auto data = WRAPD(TWDataCreateWithHexString(STRING("deadbeef").get()));
     assertHexEqual(data, "deadbeef");

tests/interface/TWStoredKeyTests.cpp

@@ -397,7 +397,8 @@ TEST(TWStoredKey, exportJSON) {
     const auto json = WRAPD(TWStoredKeyExportJSON(key.get()));
     // check size and first character
     EXPECT_TRUE(TWDataSize(json.get()) > 100);
-    EXPECT_EQ(TWDataGet(json.get(), 0), '{');
+    const auto jsonBytes = TWDataBytes(json.get());
+    EXPECT_EQ(jsonBytes[0], '{');
 }
 
 TEST(TWStoredKey, storeAndImportJSONAES256) {