Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mnemonic encryption/decryption params error #2341

Open
DeckerSU opened this issue Feb 6, 2025 · 1 comment
Open

mnemonic encryption/decryption params error #2341

DeckerSU opened this issue Feb 6, 2025 · 1 comment
Assignees
Labels
bug: security bug: wallet priority: urgent Critical tasks requiring immediate action.

Comments

@DeckerSU
Copy link
Collaborator

DeckerSU commented Feb 6, 2025

Found an issue with key derivation for encrypting/decrypting the mnemonic. The expected parameters for Argon2 are as follows:

const ARGON2_ALGORITHM: &str = "Argon2id";
const ARGON2ID_VERSION: &str = "0x13";
const ARGON2ID_M_COST: u32 = 65536;
const ARGON2ID_T_COST: u32 = 2;
const ARGON2ID_P_COST: u32 = 1;

const ARGON2_ALGORITHM: &str = "Argon2id";
const ARGON2ID_VERSION: &str = "0x13";
const ARGON2ID_M_COST: u32 = 65536;
const ARGON2ID_T_COST: u32 = 2;
const ARGON2ID_P_COST: u32 = 1;

At the very least, these parameters are passed as params (Argon2Params) in the KeyDerivationDetails structure as part of EncryptedData in decrypt_mnemonic and as the return result for encrypt_mnemonic.

In terms of the PHC string, this is equivalent to $argon2id$v=19$m=65536,t=2,p=1 (!). However, in reality, key derivation occurs inside the derive_keys_for_mnemonic function, which uses the following initialization for Argon2:

let argon2 = Argon2::default();

let argon2 = Argon2::default();

This happens regardless of EncryptedData.key_derivation_details.

Argon2::default() is equivalent to:

let params = Params::new(19456, 2, 1, Some(32)).unwrap(); 
let argon2 = Argon2::new(argon2::Algorithm::Argon2id, argon2::Version::V0x13, params);

for the Argon crate versions 0.5.2 and 0.5.3. In terms of the PHC string, this results in:

$argon2id$v=19$m=19456,t=2,p=1

In reality, we store the following information in IndexedDB:

{
  "encryption_algorithm": "AES-256-CBC",
  "key_derivation_details": {
    "params": {
      "algorithm": "Argon2id",
      "version": "0x13",
      "m_cost": 65536,
      "t_cost": 2,
      "p_cost": 1
    },
    "salt_aes": "c2FsdF9hZXNfZW5jb2RlZA==",
    "salt_hmac": "c2FsdF9obWFjX2VuY29kZWQ="
  },
  "iv": "aXYtZW5jb2RlZC1zdHJpbmc=",
  "ciphertext": "c2VkLWVuY3J5cHRlZA==",
  "tag": "aG1hYy10YWc="
}

But in reality, the derivation is performed using different parameters, including a different m_cost.

At first glance, nothing dangerous might seem to happen since encryption and decryption are always performed using Argon2::default() parameters. We are simply ignoring the parameters stored in IndexedDB and always using the default Argon crate parameters.

However, the issue arises because the default parameters in the Argon crate will change in the next release following new OWASP recommendations. As of 2024, the recommendations are here:
https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html

But for 2025, the recommended (default) parameters could change again, as has already happened:
RustCrypto/password-hashes@c69a68b

A simple update of the Argon crate in KDF will make all stored mnemonics undecryptable (!), as the default parameters will change, and it will attempt to decrypt mnemonics using different encryption parameters.

Short issue description

  1. The encryption/decryption works fine for now because we always use the same default parameters.
  2. However, once the Argon2 crate updates its defaults (e.g., to follow new OWASP recommendations), the derived keys will no longer match, rendering stored mnemonics undecryptable.

Possible Solution

The best approach would be to modify derive_keys_for_mnemonic to take key_derivation_details.params (Argon2Params) into account and ensure the derived keys are generated using the expected parameters.

@DeckerSU
Copy link
Collaborator Author

DeckerSU commented Feb 6, 2025

#2014 related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: security bug: wallet priority: urgent Critical tasks requiring immediate action.
Projects
None yet
Development

No branches or pull requests

2 participants