From 3cf52ad3ec2e5dff0a6b59be8f23d6cbaccc69df Mon Sep 17 00:00:00 2001 From: Satoshi Otomakan Date: Thu, 17 Oct 2024 11:34:31 +0200 Subject: [PATCH] [Security]: Generate random salt when creating/importing new wallet --- src/Keystore/EncryptionParameters.h | 12 +++++------ src/Keystore/ScryptParameters.cpp | 27 +++++++++++++++++++----- src/Keystore/ScryptParameters.h | 15 +++++++------ tests/common/Keystore/StoredKeyTests.cpp | 17 ++++++++++++++- 4 files changed, 53 insertions(+), 18 deletions(-) diff --git a/src/Keystore/EncryptionParameters.h b/src/Keystore/EncryptionParameters.h index 711700d7aec..d12de1b90a2 100644 --- a/src/Keystore/EncryptionParameters.h +++ b/src/Keystore/EncryptionParameters.h @@ -22,13 +22,13 @@ struct EncryptionParameters { static EncryptionParameters getPreset(enum TWStoredKeyEncryptionLevel preset, enum TWStoredKeyEncryption encryption = TWStoredKeyEncryptionAes128Ctr) { switch (preset) { case TWStoredKeyEncryptionLevelMinimal: - return EncryptionParameters(AESParameters::AESParametersFromEncryption(encryption), ScryptParameters::Minimal); + return { AESParameters::AESParametersFromEncryption(encryption), ScryptParameters::minimal() }; case TWStoredKeyEncryptionLevelWeak: case TWStoredKeyEncryptionLevelDefault: default: - return EncryptionParameters(AESParameters::AESParametersFromEncryption(encryption), ScryptParameters::Weak); + return { AESParameters::AESParametersFromEncryption(encryption), ScryptParameters::weak() }; case TWStoredKeyEncryptionLevelStandard: - return EncryptionParameters(AESParameters::AESParametersFromEncryption(encryption), ScryptParameters::Standard); + return { AESParameters::AESParametersFromEncryption(encryption), ScryptParameters::standard() }; } } @@ -54,7 +54,7 @@ struct EncryptionParameters { } /// Initializes with a JSON object. - EncryptionParameters(const nlohmann::json& json); + explicit EncryptionParameters(const nlohmann::json& json); /// Saves `this` as a JSON object. nlohmann::json json() const; @@ -91,7 +91,7 @@ struct EncryptedPayload { EncryptedPayload() = default; /// Initializes with standard values. - EncryptedPayload(const EncryptionParameters& params, const Data& encrypted, const Data& mac) + EncryptedPayload(EncryptionParameters params, Data encrypted, Data mac) : params(std::move(params)) , encrypted(std::move(encrypted)) , _mac(std::move(mac)) {} @@ -101,7 +101,7 @@ struct EncryptedPayload { EncryptedPayload(const Data& password, const Data& data, const EncryptionParameters& params); /// Initializes with a JSON object. - EncryptedPayload(const nlohmann::json& json); + explicit EncryptedPayload(const nlohmann::json& json); /// Decrypts the payload with the given password. Data decrypt(const Data& password) const; diff --git a/src/Keystore/ScryptParameters.cpp b/src/Keystore/ScryptParameters.cpp index 6f4bedf9a4d..4ec194dd5e8 100644 --- a/src/Keystore/ScryptParameters.cpp +++ b/src/Keystore/ScryptParameters.cpp @@ -11,13 +11,30 @@ using namespace TW; namespace TW::Keystore { -ScryptParameters ScryptParameters::Minimal = ScryptParameters(Data(), minimalN, defaultR, minimalP, defaultDesiredKeyLength); -ScryptParameters ScryptParameters::Weak = ScryptParameters(Data(), weakN, defaultR, weakP, defaultDesiredKeyLength); -ScryptParameters ScryptParameters::Standard = ScryptParameters(Data(), standardN, defaultR, standardP, defaultDesiredKeyLength); +namespace internal { -ScryptParameters::ScryptParameters() - : salt(32) { +Data randomSalt() { + Data salt(32); random_buffer(salt.data(), salt.size()); + return salt; +} + +} // namespace internal + +ScryptParameters ScryptParameters::minimal() { + return { internal::randomSalt(), minimalN, defaultR, minimalP, defaultDesiredKeyLength }; +} + +ScryptParameters ScryptParameters::weak() { + return { internal::randomSalt(), weakN, defaultR, weakP, defaultDesiredKeyLength }; +} + +ScryptParameters ScryptParameters::standard() { + return { internal::randomSalt(), standardN, defaultR, standardP, defaultDesiredKeyLength }; +} + +ScryptParameters::ScryptParameters() + : salt(internal::randomSalt()) { } #pragma GCC diagnostic ignored "-Wtautological-constant-out-of-range-compare" diff --git a/src/Keystore/ScryptParameters.h b/src/Keystore/ScryptParameters.h index 94cf3f1b1f2..8c8a3b3de99 100644 --- a/src/Keystore/ScryptParameters.h +++ b/src/Keystore/ScryptParameters.h @@ -21,10 +21,6 @@ enum class ScryptValidationError { /// Scrypt function parameters. struct ScryptParameters { - static ScryptParameters Minimal; - static ScryptParameters Weak; - static ScryptParameters Standard; - /// The N and P parameters of Scrypt encryption algorithm, using 256MB memory and /// taking approximately 1s CPU time on a modern processor. static const uint32_t standardN = 1 << 18; @@ -59,13 +55,20 @@ struct ScryptParameters { /// Block size factor. uint32_t r = defaultR; + /// Generates Scrypt encryption parameters with the minimal sufficient level (4096), and with a random salt. + static ScryptParameters minimal(); + /// Generates Scrypt encryption parameters with the weak sufficient level (16k), and with a random salt. + static ScryptParameters weak(); + /// Generates Scrypt encryption parameters with the standard sufficient level (262k), and with a random salt. + static ScryptParameters standard(); + /// Initializes with default scrypt parameters and a random salt. ScryptParameters(); /// Initializes `ScryptParameters` with all values. /// /// @throws ScryptValidationError if the parameters are invalid. - ScryptParameters(const Data& salt, uint32_t n, uint32_t r, uint32_t p, std::size_t desiredKeyLength) + ScryptParameters(Data salt, uint32_t n, uint32_t r, uint32_t p, std::size_t desiredKeyLength) : salt(std::move(salt)), desiredKeyLength(desiredKeyLength), n(n), p(p), r(r) { auto error = validate(); if (error) { @@ -79,7 +82,7 @@ struct ScryptParameters { std::optional validate() const; /// Initializes `ScryptParameters` with a JSON object. - ScryptParameters(const nlohmann::json& json); + explicit ScryptParameters(const nlohmann::json& json); /// Saves `this` as a JSON object. nlohmann::json json() const; diff --git a/tests/common/Keystore/StoredKeyTests.cpp b/tests/common/Keystore/StoredKeyTests.cpp index ba189cfe5a1..02c9bed047b 100644 --- a/tests/common/Keystore/StoredKeyTests.cpp +++ b/tests/common/Keystore/StoredKeyTests.cpp @@ -45,13 +45,15 @@ TEST(StoredKey, CreateWithMnemonic) { EXPECT_EQ(json["name"], "name"); EXPECT_EQ(json["type"], "mnemonic"); EXPECT_EQ(json["version"], 3); + // Salt is 32 bytes, encoded as hex. + EXPECT_EQ(json["crypto"]["kdfparams"]["salt"].get().size(), 64ul); } TEST(StoredKey, CreateWithMnemonicInvalid) { try { auto key = StoredKey::createWithMnemonic("name", gPassword, "_THIS_IS_NOT_A_VALID_MNEMONIC_", TWStoredKeyEncryptionLevelDefault); } catch (std::invalid_argument&) { - // expedcted exception OK + // expected exception OK return; } FAIL() << "Missing excpected excpetion"; @@ -539,6 +541,7 @@ TEST(StoredKey, CreateMinimalEncryptionParameters) { EXPECT_EQ(json["crypto"]["kdf"], "scrypt"); EXPECT_EQ(json["crypto"]["kdfparams"]["n"], 4096); + EXPECT_EQ(json["crypto"]["kdfparams"]["salt"].get().size(), 64ul); // load it back const auto key2 = StoredKey::createWithJson(json); @@ -557,6 +560,7 @@ TEST(StoredKey, CreateWeakEncryptionParameters) { EXPECT_EQ(json["crypto"]["kdf"], "scrypt"); EXPECT_EQ(json["crypto"]["kdfparams"]["n"], 16384); + EXPECT_EQ(json["crypto"]["kdfparams"]["salt"].get().size(), 64ul); // load it back const auto key2 = StoredKey::createWithJson(json); @@ -575,12 +579,23 @@ TEST(StoredKey, CreateStandardEncryptionParameters) { EXPECT_EQ(json["crypto"]["kdf"], "scrypt"); EXPECT_EQ(json["crypto"]["kdfparams"]["n"], 262144); + EXPECT_EQ(json["crypto"]["kdfparams"]["salt"].get().size(), 64ul); // load it back const auto key2 = StoredKey::createWithJson(json); EXPECT_EQ(key2.wallet(gPassword).getMnemonic(), string(gMnemonic)); } +TEST(StoredKey, CreateEncryptionParametersRandomSalt) { + const auto key1 = StoredKey::createWithMnemonic("name", gPassword, gMnemonic, TWStoredKeyEncryptionLevelStandard); + const auto salt1 = parse_hex(key1.json()["crypto"]["kdfparams"]["salt"]); + + const auto key2 = StoredKey::createWithMnemonic("name", gPassword, gMnemonic, TWStoredKeyEncryptionLevelStandard); + const auto salt2 = parse_hex(key2.json()["crypto"]["kdfparams"]["salt"]); + + EXPECT_NE(salt1, salt2) << "salt must be random on every StoredKey creation"; +} + TEST(StoredKey, CreateMultiAccounts) { // Multiple accounts for the same coin auto key = StoredKey::createWithMnemonic("name", gPassword, gMnemonic, TWStoredKeyEncryptionLevelDefault); EXPECT_EQ(key.type, StoredKeyType::mnemonicPhrase);