fix(aes): Fix AESParameters deserialization for AES-128-CBC and improve error handling for unsupported ciphers

include/TrustWalletCore/TWStoredKeyEncryption.h

@@ -12,7 +12,6 @@ TW_EXTERN_C_BEGIN
 TW_EXPORT_ENUM(uint32_t)
 enum TWStoredKeyEncryption {
     TWStoredKeyEncryptionAes128Ctr = 0,
-    TWStoredKeyEncryptionAes128Cbc = 1,
     TWStoredKeyEncryptionAes192Ctr = 2,
     TWStoredKeyEncryptionAes256Ctr = 3,
 };

src/Keystore/AESParameters.cpp

@@ -6,6 +6,7 @@
 
 #include "../HexCoding.h"
 
+#include <sstream>
 #include <TrezorCrypto/rand.h>
 
 using namespace TW;
@@ -21,17 +22,21 @@ Data generateIv(std::size_t blockSize = TW::Keystore::gBlockSize) {
 static TWStoredKeyEncryption getCipher(const std::string& cipher) {
     if (cipher == Keystore::gAes128Ctr) {
         return TWStoredKeyEncryption::TWStoredKeyEncryptionAes128Ctr;
-    } else if (cipher == Keystore::gAes192Ctr) {
+    }
+    if (cipher == Keystore::gAes192Ctr) {
         return TWStoredKeyEncryption::TWStoredKeyEncryptionAes192Ctr;
-    } else if (cipher == Keystore::gAes256Ctr) {
+    }
+    if (cipher == Keystore::gAes256Ctr) {
         return TWStoredKeyEncryption::TWStoredKeyEncryptionAes256Ctr;
     }
-    return TWStoredKeyEncryptionAes128Ctr;
+
+    std::stringstream ss;
+    ss << "Unsupported cipher: " << cipher;
+    throw std::invalid_argument(ss.str());
 }
 
 const std::unordered_map<TWStoredKeyEncryption, Keystore::AESParameters> gEncryptionRegistry{
     {TWStoredKeyEncryptionAes128Ctr, Keystore::AESParameters{.mKeyLength = Keystore::A128, .mCipher = Keystore::gAes128Ctr, .mCipherEncryption = TWStoredKeyEncryptionAes128Ctr, .iv{}}},
-    {TWStoredKeyEncryptionAes128Cbc, Keystore::AESParameters{.mKeyLength = Keystore::A128, .mCipher = Keystore::gAes128Cbc, .mCipherEncryption = TWStoredKeyEncryptionAes128Cbc, .iv{}}},
     {TWStoredKeyEncryptionAes192Ctr, Keystore::AESParameters{.mKeyLength = Keystore::A192, .mCipher = Keystore::gAes192Ctr, .mCipherEncryption = TWStoredKeyEncryptionAes192Ctr, .iv{}}},
     {TWStoredKeyEncryptionAes256Ctr, Keystore::AESParameters{.mKeyLength = Keystore::A256, .mCipher = Keystore::gAes256Ctr, .mCipherEncryption = TWStoredKeyEncryptionAes256Ctr, .iv{}}}
 };
@@ -43,6 +48,15 @@ namespace CodingKeys {
 static const auto iv = "iv";
 } // namespace CodingKeys
 
+std::string toString(AESValidationError error) {
+    switch (error) {
+    case AESValidationError::InvalidIV:
+        return "IV must be 16 bytes long";
+    default:
+        return "Unknown error";
+    }
+}
+
 /// Initializes `AESParameters` with a JSON object.
 AESParameters AESParameters::AESParametersFromJson(const nlohmann::json& json, const std::string& cipher) {
     auto parameters = AESParameters::AESParametersFromEncryption(getCipher(cipher));
@@ -64,4 +78,12 @@ AESParameters AESParameters::AESParametersFromEncryption(TWStoredKeyEncryption e
     return parameters;
 }
 
+std::optional<AESValidationError> AESParameters::validate() const noexcept {
+    if (iv.size() != static_cast<std::size_t>(mBlockSize)) {
+        return AESValidationError::InvalidIV;
+    }
+
+    return {};
+}
+
 } // namespace TW::Keystore

src/Keystore/AESParameters.h

@@ -20,10 +20,15 @@ enum AESKeySize : std::int32_t {
 
 inline constexpr std::size_t gBlockSize{16};
 inline constexpr const char* gAes128Ctr{"aes-128-ctr"};
-inline constexpr const char* gAes128Cbc{"aes-128-cbc"};
 inline constexpr const char* gAes192Ctr{"aes-192-ctr"};
 inline constexpr const char* gAes256Ctr{"aes-256-ctr"};
 
+enum class AESValidationError {
+    InvalidIV,
+};
+
+std::string toString(AESValidationError error);
+
 // AES128/192/256 parameters.
 struct AESParameters {
     // For AES, your block length is always going to be 128 bits/16 bytes
@@ -43,9 +48,7 @@ struct AESParameters {
     nlohmann::json json() const;
 
     /// Validates AES parameters.
-    [[nodiscard]] bool isValid() const {
-        return iv.size() == static_cast<std::size_t>(mBlockSize);
-    }
+    [[nodiscard]] std::optional<AESValidationError> validate() const noexcept;
 };
 
 } // namespace TW::Keystore

src/Keystore/EncryptionParameters.cpp

@@ -40,8 +40,10 @@ static const auto mac = "mac";
 EncryptionParameters::EncryptionParameters(const nlohmann::json& json) {
     auto cipher = json[CodingKeys::cipher].get<std::string>();
     cipherParams = AESParameters::AESParametersFromJson(json[CodingKeys::cipherParams], cipher);
-    if (!cipherParams.isValid()) {
-        throw std::invalid_argument("Invalid cipher params");
+    if (const auto error = cipherParams.validate(); error.has_value()) {
+        std::stringstream ss;
+        ss << "Invalid cipher params: " << toString(*error);
+        throw std::invalid_argument(ss.str());
     }
 
     auto kdf = json[CodingKeys::kdf].get<std::string>();
@@ -69,9 +71,11 @@ nlohmann::json EncryptionParameters::json() const {
 }
 
 EncryptedPayload::EncryptedPayload(const Data& password, const Data& data, const EncryptionParameters& params)
-    : params(std::move(params)), _mac() {
-    if (!this->params.cipherParams.isValid()) {
-        throw std::invalid_argument("Invalid cipher params");
+    : params(params), _mac() {
+    if (const auto error = this->params.cipherParams.validate(); error.has_value()) {
+        std::stringstream ss;
+        ss << "Invalid cipher params: " << toString(*error);
+        throw std::invalid_argument(ss.str());
     }
 
     auto scryptParams = std::get<ScryptParameters>(this->params.kdfParams);
@@ -84,7 +88,6 @@ EncryptedPayload::EncryptedPayload(const Data& password, const Data& data, const
     auto result = 0;
     switch(this->params.cipherParams.mCipherEncryption) {
     case TWStoredKeyEncryptionAes128Ctr:
-    case TWStoredKeyEncryptionAes128Cbc:
         result = aes_encrypt_key128(derivedKey.data(), &ctx);
         break;
     case TWStoredKeyEncryptionAes192Ctr:
@@ -137,7 +140,7 @@ Data EncryptedPayload::decrypt(const Data& password) const {
 
     // Even though the cipher params should have been validated in `EncryptedPayload` constructor,
     // double check them here.
-    if (!params.cipherParams.isValid()) {
+    if (params.cipherParams.validate().has_value()) {
         throw DecryptionError::invalidCipher;
     }
     assert(params.cipherParams.iv.size() == gBlockSize);
@@ -152,14 +155,6 @@ Data EncryptedPayload::decrypt(const Data& password) const {
 
         aes_ctr_decrypt(encrypted.data(), decrypted.data(), static_cast<int>(encrypted.size()), iv.data(),
                         aes_ctr_cbuf_inc, &ctx);
-    } else if (encryption == TWStoredKeyEncryptionAes128Cbc) {
-        aes_decrypt_ctx ctx;
-        [[maybe_unused]] auto result = aes_decrypt_key(derivedKey.data(), params.getKeyBytesSize(), &ctx);
-        assert(result != EXIT_FAILURE);
-
-        for (auto i = 0ul; i < encrypted.size(); i += params.getKeyBytesSize()) {
-            aes_cbc_decrypt(encrypted.data() + i, decrypted.data() + i, params.getKeyBytesSize(), iv.data(), &ctx);
-        }
     } else {
         throw DecryptionError::unsupportedCipher;
     }

tests/common/Keystore/Data/cbc-encrypted.json

@@ -0,0 +1,19 @@
+{
+  "version": 3,
+  "crypto": {
+    "cipher": "aes-128-cbc",
+    "cipherparams": {
+      "iv": "a4976ad73057007ad788d1f792d8851d"
+    },
+    "ciphertext": "fd630455c685b412ac8ab7d5a2ee5df0320d8870ef599e49ed9ff68e88fe0034",
+    "kdf": "scrypt",
+    "kdfparams": {
+      "dklen": 32,
+      "n": 4096,
+      "r": 8,
+      "p": 1,
+      "salt": "0102030405060708090a0b0c0d0e0f10"
+    },
+    "mac": "e465d8b0bd7946388c8415623ab28c0a001efcc1257f9eb96609ebe0f62c6e7a"
+  }
+}

tests/common/Keystore/StoredKeyTests.cpp

@@ -414,6 +414,10 @@ TEST(StoredKey, InvalidIv) {
     ASSERT_THROW(StoredKey::load(testDataPath("invalid-iv.json")), std::invalid_argument);
 }
 
+TEST(StoredKey, LoadCbcEncrypted) {
+    ASSERT_THROW(StoredKey::load(testDataPath("cbc-encrypted.json")), std::invalid_argument);
+}
+
 TEST(StoredKey, EmptyAccounts) {
     const auto key = StoredKey::load(testDataPath("empty-accounts.json"));