[Crypto]: Update TrezorCrypto/scrypt

[satoshiotomakan]

Description

Fixes: #3391
The problem in the mentioned issue is related to an incorrect working of the scrypt function.
When we compile WalletCore using Xcode 14.3.1 in Release mode, scrypt starts computing weird password hashes. Moreover, it seems to change a global context, so each function call with the same arguments returns a predictable but different hash.

For example,

Data derivedKey1(scryptParams->defaultDesiredKeyLength);
Data derivedKey2(scryptParams->defaultDesiredKeyLength);
scrypt(password.data(), password.size(), scryptParams->salt.data(),
               scryptParams->salt.size(), scryptParams->n, scryptParams->r, scryptParams->p, derivedKey1.data(),
               scryptParams->defaultDesiredKeyLength);

scrypt(password.data(), password.size(), scryptParams->salt.data(),
                           scryptParams->salt.size(), scryptParams->n, scryptParams->r, scryptParams->p, derivedKey2.data(),
                           scryptParams->defaultDesiredKeyLength);

std::cout << ">>> EncryptedPayload::decrypt() password= " << hex(password) << std::endl
                  << "scryptParams.salt=" << hex(scryptParams->salt) << std::endl
                  << "scryptParams.n=" << scryptParams->n << std::endl
                  << "scryptParams.r=" << scryptParams->r << std::endl
                  << "scryptParams.p=" << scryptParams->p << std::endl
                  << "scryptParams.defaultDesiredKeyLength=" << scryptParams->defaultDesiredKeyLength << std::endl
                  << "derivedKey1=" << hex(derivedKey1) << std::endl
                  << "derivedKey2=" << hex(derivedKey2) << std::endl;

Output:

password=70617373776f7264
scryptParams.salt=
scryptParams.n=16384
scryptParams.r=8
scryptParams.p=4
scryptParams.defaultDesiredKeyLength=32
derivedKey1=209a063ae3dacc86439fefd54eb6f2bde20cd3d4b977fb77799dd787c14101ec
derivedKey2=9ad6714baaa47fa87d463af94ce9683857f0107b52e005fb369b9ac972a70910

To fix the issue, the TrezorCrypto/crypto/scrypt.c file needs to be updated with the latest Tarsnap/scrypt/lib/crypto/crypto_scrypt-ref.c.

How to test

Run C++, Android, iOS in Debug and Release modes.

Types of changes

Checklist

• Create pull request as draft initially, unless its complete.
• Add tests to cover changes as needed.
• Update documentation as needed.
• If there is a related Issue, mention it in the description.

If you're adding a new blockchain

• I have read the guidelines for adding a new blockchain.

[hewigovens]

You can use Beyond Compare to view the diff, the changes look ok to me

[hewigovens]

Let's use code from https://github.com/Tarsnap/scrypt/releases/tag/1.3.2? easier to track and diff later

[hewigovens]

I believe the original scrypt code is from https://github.com/technion/libscrypt/blob/master/crypto_scrypt-nosse.c

[satoshiotomakan]

I ran the tests in Debug and Release modes to compare performance of different implementations: RustCrypto/scrypt, updated Tarsnap/scrypt/lib/crypto/crypto_scrypt-ref.c and original WalletCore/TrezorCrypto/scrypt.c.

Critical bug in WalletCore/TrezorCrypto/scrypt.c

I found a critical bug related to the current implementation: WalletCore/TrezorCrypto/scrypt.c function derives different keys in -00/-03 and -0s optimisation levels using the same parameters - at least on my M1 local machine and when I run iOS tests (in debug and release).
The main problem is that it works correctly only with -00/-01/-02/-03 optimisation. I compared the implementation with RustCrypto/scrypt and Tarsnap/scrypt: they both compute the same keys in both Debug and Release modes.
CMake flags to compile Wallet Core tests with the -0s optimisation level:

set(CMAKE_BUILD_TYPE Release)
set(CMAKE_CXX_FLAGS "-Wall -Wextra")
set(CMAKE_CXX_FLAGS_DEBUG "-Os")
set(CMAKE_CXX_FLAGS_RELEASE "-Os")

set(CMAKE_C_FLAGS "-Wall -Wextra")
set(CMAKE_C_FLAGS_DEBUG "-Os")
set(CMAKE_C_FLAGS_RELEASE "-Os")

However, I used TrustWallet/ios/TrustKeystoreTests::testImportMnemonicKeystore and TrustWallet/android/TestKeyStoreAdapter::testExportPrivateKey tests to figure out which optimisation level is used in our Wallet Core release artefacts.
Conclusion - Wallet Core Android and iOS release artefacts (libTrustWalletCore.a) computes valid keys. Unfortunately, I couldn't figure out which optimisation level is used on iOS and Android release, but the level is not enough to make scrypt function working incorrectly.

Given the fact that the current implementation is not robust to the optimization level, I'd strongly recommend to switch to a new scrypt implementation.

Alternative Solutions

RustCrypto/scrypt

RustCrypto/scrypt is a Pure Rust implementation of the scrypt key derivation function.
I prepared a Proof Of Concept in the #3489 Pull Request.
You can find performance metrics below.
There are two cons with this approach:

• It requires modifications in the RustCrypto/scrypt crate as it does not allow kdf parameters such as log(2, n) >= r * 16: https://github.com/RustCrypto/password-hashes/blob/a737bef1f992368f165face097d621bb1e76eba4/scrypt/src/params.rs#L67-L72
• The original RustCrypto/scrypt and RustCrypto/pbkdf2 most likely not audited. Moreover, RustCrypto/salsa20 not audited and has a usage warning.

Tarsnap/scrypt

Tarsnap/scrypt/lib/crypto/crypto_scrypt-ref.c is represented in this Pull Request.
The cons of this approach:

• It's significantly slower than the original WalletCore/TrezorCrypto/scrypt.c and RustCrypto/scrypt. See the metrics below.

Performance Metrics

Test suit	RustCrypto/scrypt	Tarsnap/scrypt	WalletCore/TrezorCrypto/scrypt.c
WASM tests (-0s)	7s	11s	6s
WASM tests (./tools/wasm-build)	7s	49s	19s
C++ tests (-0s)	22.072s	48.976s	38.401s
C++ TWStoredKey (-0s)	4.379s	12.506s	9.406s
Xcode tests (release)	14.770s	39.935s	16.020s
Xcode KeyStoreTests (release)	9.451s	27.556s	11.793s

Please also note that:

1. WASM tests (-0s) are build with -Os optimisation level.
2. ./tools/wasm-build compiles WC C++ part in Debug mode. IMPORTANT we build C++ part in WASM in Debug mode too. Note that Rust part is always built in Release mode for ALL targets.
3. Xcode tests fail with the old WalletCore/TrezorCrypto/scrypt.c in Release mode.