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

[Design] New encryption standard #637

Open
r4mmer opened this issue Apr 19, 2024 · 5 comments
Open

[Design] New encryption standard #637

r4mmer opened this issue Apr 19, 2024 · 5 comments
Assignees

Comments

@r4mmer
Copy link
Member

r4mmer commented Apr 19, 2024

Summary

Update the wallet-lib's encryption standard, increasing security and offering more flexibility to the wallets.

Motivation

After a npm audit alert about some insecurities on PBKDF2 (Password Based Key Derivation Function 2) usage we decided to investigate our usage and this is meant to propose some improvements to how we store sensitive data.

Guide-level explanation

Current standard

The current encrypted data follows this model.

interface IEncryptedData {
  data: string; // encrypted data
  hash: string; // PBKDF2 of the secret
  salt: string; // salt, iterations and pbkdf2Hasher are used in the hash above
  iterations: number;
  pbkdf2Hasher: string;
}

We use PBKDF2 with the default of 1000 iterations to save the "hash" of the secret and use AES encryption to calculate the data using the secret.

This means data = AES.encrypt(value, secret) and hash = PBKDF2(secret, config) this means that we can use the hash to validate the user input before actually decrypting the data or to just validate that the user has inputted the correct secret.

This has some issues since PBKDF2 current security standard should use 1.3 million iterations with the sha1 hasher (pbkdf2Hasher) or 600 thousand iterations with the sha256 hasher.
Also AES encryption with the raw secret is less secure than if we used a key derivation function to generate the AES secret.

Proposed standard

AES encryption is pretty safe if a proper encryption key is used, so we will use the user password to generate the encryption key instead of using the password directly.
This means that we also cannot save the hash of the password, since it makes it easier to derive the key.

interface IEncryptedData {
  data: string; // encrypted secret+data
  hash: string; // sha256 of secret
  kdfConfig: IKdfConfig; // KDF config
}

interface IPBKDF2Config {
  kdf: 'PBKDF2';
  salt: string;
  iterations: string;
  hasher: string;
}

interface IKdfConfig = IPBKDF2Config | ... // can include other KDFs

AES decryption may return some garbage output if the wrong password is used we will add 32 bytes to the encrypted data.
These 32 bytes will be used as a secret to check that the decryption process worked, by saving the sha256 hash of this secret we can check that the decrypted data matches.

This process may not work, but the 32 bytes of decrypted garbage would need to match the secret, this has about $2^(-256)$ chance in happening.

// > encryption
// generate a key from the password and kdf config
const key = generateKey(password, kdfConfig);
// generate a random secret
const secret = generateSecret();
// encrypt the secret + value
const data = AES.encrypt(secret+value, key);
// save the sha256 hash of the random secret
const hash = sha256(secret);


// > decryption
// generate a key using the password from user input (slow process)
const key = generateKey(password, kdfConfig);
// decrypt the data using AES
const decryptedData = AES.decrypt(data, key);
// get the random secret from the decrypted data (length must be higher than 32 bytes)
const possibleSecret = decryptedData[:32];
if (sha256(possibleSecret) === hash) {
  // decryption successfull
  const value = decryptedData[32:];
} else {
  // decryption error: wrong password
}

As stated before the storage will hold the encrypted data, the hash of a random secret and the KDF (Key Derivation Function) configuration.
With this information we can safely decrypt and detect that the correct password was used.
We will not loose any functionality for our wallets while achieving a higher security for the stored data.

The other benefit of this proposal is the flexibility to choose another KDF, change its parameters to have increased security or update the defaults while not having to migrate all wallets to the new default.

Migration process

To avoid migration issues we should leave the old standard implemented as legacy and rename the IEncryptedData to ILegacyEncryptedData, we would then be able to check the stored data and determine which method of decryption to use.

All new wallets will be created with the new standard but old wallets may use the old standard.

The desktop wallet is the only one that cannot make the migration process when unlocking the wallet because it uses a pin and password scheme where the seed is stored with the password and the derived private keys with the pin.
The best way is to have a warning label like the backup warning that once clicked will open a modal window with the migration form.

For migrations from versions v0.26.0 and previous, we cannot safely migrate to the new standard since the encrypted data has changed, so we can keep the migration to the legacy standard and show the migration warning so the user can manually migrate to the new standard.
This means that the UX for migrations on all wallets stays the same.

Reference-level explanation

Encryption Standard

function generateEncryptionKey(password: string, config: IKdfConfig): {
  match (config.kdf) {
    case 'PBKDF2':
	    return executePBKDF2(password, config);
	...
  }
}

function encryptData(value: string, password: string, config: IKdfConfig|null = null): IEncryptedData {
  const kdfConfig = getKDFConfig(config);
  const key = generateEncryptionKey(password, kdfConfig);
  const secret = CryptoJS.lib.WordArray.random(32).toString();
  const hash = sha256(secret);
  const data = CryptoJS.AES.encrypt(secret+value, password).toString();
  return {
    data,
    hash,
    kdfConfig,
  };
}

function _decryptData(data: IEncryptedData, password: string): string {
  const key = generateEncryptionKey(password, data.kdfConfig);
  const decryptedRaw = CryptoJS.AES.decrypt(data.data, key);
  return decryptedRaw.toString(CryptoJS.enc.Utf8);
}

function decryptData(data: IEncryptedData, password: string): string {
  const decrypted = _decryptData(data, password);
  if (decrypted.length < 32) {
    // Password error
    throw new Error('Wrong password');
  }
  const possibleHash = sha256(decrypted.slice(0, 32));
  if (possibleHash != data.hash) {
    throw new Error('Wrong password');
  }
  return decrypted.slice(32);
}

function checkPassword(data: IEncryptedData, password: string): boolean {
  try {
    const decrypted = decryptData(data, password);
    return true;
  } catch (_) {
    return false;
  }
}

While simple this already covers all crypto functions we aim to replace, but to allow working with the new and old standards we need to make some small changes, basically change IEncryptedData to be:

interface IEncryptedData = ILegacyEncryptedData | INewEncryptedData;

function isLegacyEncryptedData(data: IEncryptedData): data is ILegacyEncryptedData {
  // Just check for a key not present on INewEncryptedData
  return 'iterations' in data;
}

With this we can check on the exported functions if the data is legacy and use the legacy implementation.

Default KDF config

Since we will not be introducing a new KDF and will be working with the PBKDF2 we need to define a sane default config and following best practices we will use sha256 as the hasher algo.

Another security improvement we will make is have the iterations be a random number around 10 thousand, this will make rainbow table attacks more difficult by having a non-deterministic number of iterations.
Each wallet may configure the default number of iterations but for mobile we should keep the 10 thousand and for desktop we should change to 100 thousand iterations.

Migration to new standard

The mobile wallet pin can be used during the unlocking process to decrypt the entire access data and save in the new format, migration from v0.26.0 and older will be required to migrate to the legacy encryption standard first then to the new standard.

In the desktop wallet we will introduce a new localStorage key localstorage:migration to mark if the migration is done or not, this key will be used only to show the warning label.

Future possibilities

Encryption strenght slider

The AES encryption algorithm is as strong as the encryption key used with it, so the key derivation function (KDF) used is the best measure of strenght in the new encryption standard.

Usually a KDF is configured with a salt, an output key size, an input key and a difficulty level and for PBKDF2 the difficulty level is the number of iterations.

The difficulty slider will then be the number of iterations used in the config, remembering the random factor here so when 200 thousand is selected we should have the actual number of iterations be around 200 thousand $+/- 5%$.

New KDFs

There are stronger KDFs and better for our usage, for instance Scrypt or Argon2 which could be used by simply implementing a new branch on generateEncryptionKey and possibly updating the default KDF.
This would make any new wallets use the new KDF and old wallets will still use the old KDF without issues since the type saved in the access data would indicate which KDF to use when decrypting and its config.

The "encryption strength slider" mechanic should also work well with other KDFs since they also have a difficulty level input, it just may not be a number of iterations.

Guessing the best KDF config for the hardware

A KDF must be a slow/costly function to run since one of its responsibilities is mitigating brute force attacks.
A slow function in a mobile environment may not be slow in the desktop due to hardware differences, so a good way to define a local difficulty level for the current hardware is to run a sequence of KDF of varying difficulties and checking the time it took to run each of them.

This way we can choose the difficulty by the time it takes to run the KDF instead of guessing a number and have that be slow on some hardware and fast on other.

@r4mmer r4mmer self-assigned this Apr 19, 2024
@r4mmer r4mmer moved this from Todo to In Progress (WIP) in Hathor Network Apr 19, 2024
@r4mmer r4mmer moved this from In Progress (WIP) to In Progress (Done) in Hathor Network Apr 22, 2024
@r4mmer
Copy link
Member Author

r4mmer commented May 6, 2024

reviewers: @pedroferreira1 @msbrogli

@pedroferreira1
Copy link
Member

For migrations from versions v0.26.0 and previous, we cannot safely migrate to the new standard since the encrypted data has changed, so we can keep the migration to the legacy standard and show the migration warning so the user can manually migrate to the new standard.

I didn't understand this. This migration will happen after the user upgrades his wallet. Versions before v0.26.0 are the ones without the new storage scheme, then we can migrate the storage as we already do, and have the migration warning, can't we?


The checkPassword and decryptData have duplicated code. For the final version we must improve this.


Each wallet may configure the default number of iterations but for mobile we should keep the 10 thousand and for desktop we should change to 100 thousand iterations.

How long does it take to encrypt/decrypt the data with this amount of iterations?


function isLegacyEncryptedData(data: IEncryptedData): data is ILegacyEncryptedData {
  // Just check for a key not present on INewEncryptedData
  return 'iterations' in data;
}

We won't store the number of iterations in the storage anymore?

@pedroferreira1 pedroferreira1 moved this from In Progress (Done) to In Review (WIP) in Hathor Network May 7, 2024
@r4mmer
Copy link
Member Author

r4mmer commented May 7, 2024

I didn't understand this. This migration will happen after the user upgrades his wallet. Versions before v0.26.0 are the ones without the new storage scheme, then we can migrate the storage as we already do, and have the migration warning, can't we?

Yes, that is what I mean by "keep the migration to the legacy standard and show the migration warning so the user can manually migrate to the new standard".
In this context the "legacy standard" is how the new storage scheme handles encrypted access data.


How long does it take to encrypt/decrypt the data with this amount of iterations?

It depends on the hardware, on my personal machine with 100k iterations I got 600ms and on the phone with 10k iterations I got 2s.
We can adjust this and under "Future possibilities > Guessing the best KDF config for the hardware" I explain how we can have a more personalized iteration count.


We won't store the number of iterations in the storage anymore?

Yes we do, but inside the kdfConfig.
The iteration count is a PBKDF2 configuration (hence IPBKDF2Config), other KDFs will have other configuration parameters.
For instance Argon2 can have how many threads and the amount of memory each call can consume, since these are KDF specific i decided to move the KDF config to the kdfConfig and make adding other KDFs easier in the future.

@r4mmer
Copy link
Member Author

r4mmer commented May 7, 2024

The checkPassword and decryptData have duplicated code. For the final version we must improve this.

Done

@pedroferreira1
Copy link
Member

For me this is approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review (WIP)
Development

No branches or pull requests

2 participants