Skip to content

Commit 1a13583

Browse files
satoshiotomakansatoshiotomakan
authored
fix(aes): Add iv size validation (#4580)
* fix(aes): Add iv size validation * fix(aes): Add unit tests
1 parent 7e848e0 commit 1a13583

File tree

6 files changed

+95
-4
lines changed

6 files changed

+95
-4
lines changed

‎src/Encrypt.cpp‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@ size_t paddingSize(size_t origSize, size_t blockSize, TWAESPaddingMode paddingMo
2424
}
2525

2626
Data AESCBCEncrypt(const Data& key, const Data& data, Data& iv, TWAESPaddingMode paddingMode) {
27+
if (iv.size() != AES_BLOCK_SIZE) {
28+
throw std::invalid_argument("Invalid iv size");
29+
}
30+
2731
aes_encrypt_ctx ctx;
2832
if (aes_encrypt_key(key.data(), static_cast<int>(key.size()), &ctx) == EXIT_FAILURE) {
2933
throw std::invalid_argument("Invalid key");
@@ -52,6 +56,10 @@ Data AESCBCEncrypt(const Data& key, const Data& data, Data& iv, TWAESPaddingMode
5256
}
5357

5458
Data AESCBCDecrypt(const Data& key, const Data& data, Data& iv, TWAESPaddingMode paddingMode) {
59+
if (iv.size() != AES_BLOCK_SIZE) {
60+
throw std::invalid_argument("Invalid iv size");
61+
}
62+
5563
const size_t blockSize = AES_BLOCK_SIZE;
5664
if (data.size() % blockSize != 0) {
5765
throw std::invalid_argument("Invalid data size");
@@ -83,7 +91,11 @@ Data AESCBCDecrypt(const Data& key, const Data& data, Data& iv, TWAESPaddingMode
8391
}
8492

8593
Data AESCTREncrypt(const Data& key, const Data& data, Data& iv) {
86-
aes_encrypt_ctx ctx;
94+
if (iv.size() != AES_BLOCK_SIZE) {
95+
throw std::invalid_argument("Invalid iv size");
96+
}
97+
98+
aes_encrypt_ctx ctx;
8799
if (aes_encrypt_key(key.data(), static_cast<int>(key.size()), &ctx) != EXIT_SUCCESS) {
88100
throw std::invalid_argument("Invalid key");
89101
}
@@ -94,6 +106,10 @@ Data AESCTREncrypt(const Data& key, const Data& data, Data& iv) {
94106
}
95107

96108
Data AESCTRDecrypt(const Data& key, const Data& data, Data& iv) {
109+
if (iv.size() != AES_BLOCK_SIZE) {
110+
throw std::invalid_argument("Invalid iv size");
111+
}
112+
97113
aes_encrypt_ctx ctx;
98114
if (aes_encrypt_key(key.data(), static_cast<int>(key.size()), &ctx) != EXIT_SUCCESS) {
99115
throw std::invalid_argument("Invalid key");

‎src/Keystore/AESParameters.h‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,18 @@ struct AESParameters {
3434
Data iv;
3535

3636
/// Initializes `AESParameters` with a encryption cipher.
37-
static AESParameters AESParametersFromEncryption(TWStoredKeyEncryption encryption);;
37+
static AESParameters AESParametersFromEncryption(TWStoredKeyEncryption encryption);
3838

3939
/// Initializes `AESParameters` with a JSON object.
4040
static AESParameters AESParametersFromJson(const nlohmann::json& json, const std::string& cipher);
4141

4242
/// Saves `this` as a JSON object.
4343
nlohmann::json json() const;
44+
45+
/// Validates AES parameters.
46+
[[nodiscard]] bool isValid() const {
47+
return iv.size() == static_cast<std::size_t>(mBlockSize);
48+
}
4449
};
4550

4651
} // namespace TW::Keystore

‎src/Keystore/EncryptionParameters.cpp‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ static const auto mac = "mac";
4040
EncryptionParameters::EncryptionParameters(const nlohmann::json& json) {
4141
auto cipher = json[CodingKeys::cipher].get<std::string>();
4242
cipherParams = AESParameters::AESParametersFromJson(json[CodingKeys::cipherParams], cipher);
43+
if (!cipherParams.isValid()) {
44+
throw std::invalid_argument("Invalid cipher params");
45+
}
4346

4447
auto kdf = json[CodingKeys::kdf].get<std::string>();
4548
if (kdf == "scrypt") {
@@ -67,6 +70,10 @@ nlohmann::json EncryptionParameters::json() const {
6770

6871
EncryptedPayload::EncryptedPayload(const Data& password, const Data& data, const EncryptionParameters& params)
6972
: params(std::move(params)), _mac() {
73+
if (!this->params.cipherParams.isValid()) {
74+
throw std::invalid_argument("Invalid cipher params");
75+
}
76+
7077
auto scryptParams = std::get<ScryptParameters>(this->params.kdfParams);
7178
auto derivedKey = Data(scryptParams.desiredKeyLength);
7279
scrypt(reinterpret_cast<const byte*>(password.data()), password.size(), scryptParams.salt.data(),
@@ -90,6 +97,9 @@ EncryptedPayload::EncryptedPayload(const Data& password, const Data& data, const
9097
assert(result == EXIT_SUCCESS);
9198
if (result == EXIT_SUCCESS) {
9299
Data iv = this->params.cipherParams.iv;
100+
// iv size should have been validated in `AESParameters::isValid()`.
101+
assert(iv.size() == gBlockSize);
102+
93103
encrypted = Data(data.size());
94104
aes_ctr_encrypt(data.data(), encrypted.data(), static_cast<int>(data.size()), iv.data(), aes_ctr_cbuf_inc, &ctx);
95105
_mac = computeMAC(derivedKey.end() - params.getKeyBytesSize(), derivedKey.end(), encrypted);
@@ -125,6 +135,13 @@ Data EncryptedPayload::decrypt(const Data& password) const {
125135
throw DecryptionError::invalidPassword;
126136
}
127137

138+
// Even though the cipher params should have been validated in `EncryptedPayload` constructor,
139+
// double check them here.
140+
if (!params.cipherParams.isValid()) {
141+
throw DecryptionError::invalidCipher;
142+
}
143+
assert(params.cipherParams.iv.size() == gBlockSize);
144+
128145
Data decrypted(encrypted.size());
129146
Data iv = params.cipherParams.iv;
130147
const auto encryption = params.cipherParams.mCipherEncryption;

‎tests/common/EncryptTests.cpp‎

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,12 @@ TEST(Encrypt, AESCBCEncrypt) {
4949
assertHexEqual(encryptResult, "f58c4c04d6e5f1ba779eabfb5f7bfbd6");
5050
}
5151

52+
TEST(Encrypt, AESCBCEncryptInvalidIv) {
53+
auto iv = parse_hex("0001020304050607");
54+
auto data = parse_hex("6bc1bee22e409f96e93d7e117393172a");
55+
ASSERT_THROW(AESCBCEncrypt(gKey, data, iv), std::invalid_argument);
56+
}
57+
5258
TEST(Encrypt, AESCBCEncryptWithPadding) {
5359
const Data key = parse_hex("bf6cfdd852f79460981062f551f1dc3215b5e26609bc2a275d5b2da21798b489");
5460
const Data message = TW::data("secret message");
@@ -66,13 +72,19 @@ TEST(Encrypt, AESCBCEncryptWithPadding) {
6672
}
6773

6874
TEST(Encrypt, AESCBCDecrypt) {
69-
auto iv = parse_hex("000102030405060708090A0B0C0D0E0F");
75+
auto iv = parse_hex("000102030405060708090A0B0C0D0E0F");
7076
auto cipher = parse_hex("f58c4c04d6e5f1ba779eabfb5f7bfbd6");
7177

7278
auto decryptResult = AESCBCDecrypt(gKey, cipher, iv);
7379
assertHexEqual(decryptResult, "6bc1bee22e409f96e93d7e117393172a");
7480
}
7581

82+
TEST(Encrypt, AESCBCDecryptInvalidIv) {
83+
auto iv = parse_hex("000102030405060708090A0B0C0D0E0F000102030405060708090A0B0C0D0E0F");
84+
auto cipher = parse_hex("f58c4c04d6e5f1ba779eabfb5f7bfbd6");
85+
ASSERT_THROW(AESCBCDecrypt(gKey, cipher, iv), std::invalid_argument);
86+
}
87+
7688
TEST(Encrypt, AESCBCDecryptWithPadding) {
7789
const Data key = parse_hex("bf6cfdd852f79460981062f551f1dc3215b5e26609bc2a275d5b2da21798b489");
7890
{
@@ -101,14 +113,28 @@ TEST(Encrypt, AESCTREncrypt) {
101113
assertHexEqual(encryptResult, "601ec313775789a5b7a7f504bbf3d228");
102114
}
103115

116+
TEST(Encrypt, AESCTREncryptIvalidIv) {
117+
// iv is too short.
118+
auto iv = parse_hex("ff");
119+
auto data = parse_hex("6bc1bee22e409f96e93d7e117393172a");
120+
ASSERT_THROW(AESCTREncrypt(gKey, data, iv), std::invalid_argument);
121+
}
122+
104123
TEST(Encrypt, AESCTRDecrypt) {
105-
auto iv = parse_hex("f0f1f2f3f4f5f6f7f8f9fafbfcfdfeff");
124+
auto iv = parse_hex("f0f1f2f3f4f5f6f7f8f9fafbfcfdfeff");
106125
auto cipher = parse_hex("601ec313775789a5b7a7f504bbf3d228");
107126

108127
auto decryptResult = AESCTRDecrypt(gKey, cipher, iv);
109128
assertHexEqual(decryptResult, "6bc1bee22e409f96e93d7e117393172a");
110129
}
111130

131+
TEST(Encrypt, AESCTRDecryptIvalidIv) {
132+
// iv is too long.
133+
auto iv = parse_hex("f0f1f2f3f4f5f6f7f8f9fafbfcfdfefff0");
134+
auto cipher = parse_hex("601ec313775789a5b7a7f504bbf3d228");
135+
ASSERT_THROW(AESCTRDecrypt(gKey, cipher, iv), std::invalid_argument);
136+
}
137+
112138
TEST(Encrypt, AESCBCEncryptMultipleBlocks) {
113139
auto key = parse_hex("e1094a016e6029eabc6f9e3c3cd9afb8");
114140
auto iv = parse_hex("884b972d70acece4ecf9b790ffce177e");

‎tests/common/Keystore/Data/invalid-iv.json‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
{
2+
"version": 3,
3+
"id": "e0fe53d0-7a3d-4f65-88b1-9bb4e245a169",
4+
"crypto": {
5+
"ciphertext": "64b5b416bb2bef882eb7cc63ed92c064e53c818ec46351e07ac140e5ba871596f1595fe6cad8333147fe68c031ba001b79b64dd1edd513043134217b7ffe1903ca23b1fbe823671827e3b2dff69bbd448d9cb79a3321ec8801f2a995",
6+
"cipherparams": {
7+
"iv": "7aaf7eb6f4b0e7d9"
8+
},
9+
"kdf": "scrypt",
10+
"kdfparams": {
11+
"r": 8,
12+
"p": 6,
13+
"n": 4096,
14+
"dklen": 32,
15+
"salt": "80132842c6cde8f9d04582932ef92c3cad3ba6b41e1296ef681692372886db86"
16+
},
17+
"mac": "01816d0a5c31cd03b644f2d756ac8167c2498808040cbace8c35c46dcf06b7a1",
18+
"cipher": "aes-128-ctr"
19+
},
20+
"type": "mnemonic",
21+
"coin": 60,
22+
"address": "32dd55E0BCF509a35A3F5eEb8593fbEb244796b1"
23+
}

‎tests/common/Keystore/StoredKeyTests.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,10 @@ TEST(StoredKey, InvalidPassword) {
410410
ASSERT_THROW(key.payload.decrypt(gPassword), DecryptionError);
411411
}
412412

413+
TEST(StoredKey, InvalidIv) {
414+
ASSERT_THROW(StoredKey::load(testDataPath("invalid-iv.json")), std::invalid_argument);
415+
}
416+
413417
TEST(StoredKey, EmptyAccounts) {
414418
const auto key = StoredKey::load(testDataPath("empty-accounts.json"));
415419

0 commit comments

Comments
 (0)