[Security]: Generate random salt when creating/importing new wallet (t…

…rustwallet#4069)

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;

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"

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<ScryptValidationError> 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;

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<std::string>().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<std::string>().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<std::string>().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<std::string>().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);