Skip to content

Commit

Permalink
refactor: remove crypto-js which is now deprecated (#5866)
Browse files Browse the repository at this point in the history
### Description

This removes [crypto-js](https://github.com/brix/crypto-js) which is now
discontinued:
> Active development of CryptoJS has been discontinued. This library is
no longer maintained.
>
> Nowadays, NodeJS and modern browsers have a native Crypto module. The
latest version of CryptoJS already uses the native Crypto module for
random number generation, since Math.random() is not crypto-safe.
Further development of CryptoJS would result in it only being a wrapper
of native Crypto. Therefore, development and maintenance has been
discontinued, it is time to go for the native crypto module.

Bonus, because it now uses the native crypto module (via
[react-native-quick-crypto](https://github.com/margelo/react-native-quick-crypto))
AES encryption/decryption should be faster. Though the gains are
probably not noticeable given the small use we have.

Note: there's an important change for tests too, they now all use real
encryption.
- I feel this is less surprising than what we used to do.
- I was able to refactor tests that expected mocked encryption without
too much trouble

### Test plan

- Updated tests (ensuring backward compat with CryptoJS encrypted
strings)
- Manually tested existing keychain items encrypted by CryptoJS can
still be decrypted in Valora

### Related issues

- Part of RET-1185

### Backwards compatibility

Yes

### Network scalability

If a new NetworkId and/or Network are added in the future, the changes
in this PR will:

- [x] Continue to work without code changes, OR trigger a compilation
error (guaranteeing we find it when a new network is added)
  • Loading branch information
jeanregisser authored Sep 3, 2024
1 parent 737cfd5 commit d4171f3
Show file tree
Hide file tree
Showing 14 changed files with 145 additions and 66 deletions.
8 changes: 0 additions & 8 deletions __mocks__/crypto-js.ts

This file was deleted.

3 changes: 3 additions & 0 deletions babel.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ if (!process.env.JEST_WORKER_ID) {
{
root: ['.'],
alias: {
crypto: 'react-native-quick-crypto',
stream: 'readable-stream',
buffer: '@craftzdog/react-native-buffer',
'^src/(.+)$': './src/\\1',
'^locales$': './locales',
},
Expand Down
2 changes: 0 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@
"bignumber.js": "^9.1.2",
"clevertap-react-native": "^2.2.1",
"country-data": "^0.0.31",
"crypto-js": "^4.2.0",
"date-fns": "^3.6.0",
"dot-prop-immutable": "^2.1.1",
"fast-levenshtein": "^3.0.0",
Expand Down Expand Up @@ -223,7 +222,6 @@
"@testing-library/jest-native": "^5.4.3",
"@testing-library/react-native": "^12.4.5",
"@types/country-data": "^0.0.5",
"@types/crypto-js": "^4.1.1",
"@types/fast-levenshtein": "^0.0.2",
"@types/fs-extra": "^9.0.13",
"@types/hoist-non-react-statics": "^3.3.5",
Expand Down
7 changes: 3 additions & 4 deletions src/backup/utils.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import CryptoJS from 'crypto-js'
import { useAsync } from 'react-async-hook'
import { showError } from 'src/alert/actions'
import AppAnalytics from 'src/analytics/AppAnalytics'
Expand All @@ -9,6 +8,7 @@ import { useDispatch, useSelector } from 'src/redux/hooks'
import { removeStoredItem, retrieveStoredItem, storeItem } from 'src/storage/keychain'
import Logger from 'src/utils/Logger'
import { CELO_DERIVATION_PATH_BASE, generateKeys } from 'src/utils/account'
import { aesDecrypt, aesEncrypt } from 'src/utils/aes'
import { ETHEREUM_DERIVATION_PATH } from 'src/web3/consts'
import { currentAccountSelector } from 'src/web3/selectors'

Expand Down Expand Up @@ -95,10 +95,9 @@ export function isValidBackupPhrase(phrase: string) {
}

export async function encryptMnemonic(phrase: string, password: string) {
return CryptoJS.AES.encrypt(phrase, password).toString()
return aesEncrypt(phrase, password)
}

export async function decryptMnemonic(encryptedMnemonic: string, password: string) {
const bytes = CryptoJS.AES.decrypt(encryptedMnemonic, password)
return bytes.toString(CryptoJS.enc.Utf8)
return aesDecrypt(encryptedMnemonic, password)
}
8 changes: 6 additions & 2 deletions src/keylessBackup/keychain.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { getSECP256k1PrivateKey, storeSECP256k1PrivateKey } from 'src/keylessBackup/keychain'
import { getPassword } from 'src/pincode/authentication'
import { retrieveStoredItem, storeItem } from 'src/storage/keychain'
import { encryptPrivateKey } from 'src/web3/KeychainAccounts'
import { generatePrivateKey } from 'viem/accounts'

jest.mock('src/pincode/authentication')
Expand Down Expand Up @@ -33,8 +34,11 @@ describe(getSECP256k1PrivateKey, () => {

it('gets the private key from the keychain', async () => {
jest.mocked(getPassword).mockResolvedValue('password')
jest.mocked(retrieveStoredItem).mockResolvedValue(mockPrivateKey)
await getSECP256k1PrivateKey('0x1234')
jest
.mocked(retrieveStoredItem)
.mockResolvedValue(await encryptPrivateKey(mockPrivateKey, 'password'))
const privateKey = await getSECP256k1PrivateKey('0x1234')
expect(privateKey).toBe(mockPrivateKey)
expect(getPassword).toHaveBeenCalledWith('0x1234')
expect(retrieveStoredItem).toHaveBeenCalledWith('secp256k1PrivateKey')
})
Expand Down
3 changes: 3 additions & 0 deletions src/missingGlobals.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Without this, one will see a confusing error
// similar to https://imgur.com/a/7rnLIh5
import { install } from 'react-native-quick-crypto'
import 'react-native-url-polyfill/auto'

export interface Global {
Expand All @@ -15,3 +16,5 @@ if (typeof global.self === 'undefined') {
}
global.btoa = require('Base64').btoa
require('crypto')

install()
51 changes: 41 additions & 10 deletions src/pincode/authentication.test.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import CryptoJS from 'crypto-js'
import * as Keychain from 'react-native-keychain'
import { expectSaga } from 'redux-saga-test-plan'
import { select } from 'redux-saga/effects'
import { PincodeType } from 'src/account/reducer'
import { pincodeTypeSelector } from 'src/account/selectors'
import AppAnalytics from 'src/analytics/AppAnalytics'
import { AuthenticationEvents } from 'src/analytics/Events'
import { encryptMnemonic } from 'src/backup/utils'
import { storedPasswordRefreshed } from 'src/identity/actions'
import { navigate, navigateBack } from 'src/navigator/NavigationService'
import {
Expand Down Expand Up @@ -33,10 +33,11 @@ import {
} from 'src/pincode/authentication'
import { store } from 'src/redux/store'
import Logger from 'src/utils/Logger'
import { aesDecrypt, aesEncrypt } from 'src/utils/aes'
import { ensureError } from 'src/utils/ensureError'
import { getWalletAsync } from 'src/web3/contracts'
import { getMockStoreData } from 'test/utils'
import { mockAccount } from 'test/values'
import { mockAccount, mockMnemonic } from 'test/values'

jest.mock('src/web3/contracts')
jest.unmock('src/pincode/authentication')
Expand All @@ -51,6 +52,16 @@ jest.mock('src/utils/sleep', () => ({
sleep: jest.fn().mockResolvedValue(true),
}))

jest.mock('src/utils/aes', () => {
const originalAes = jest.requireActual('src/utils/aes')

return {
...originalAes,
aesEncrypt: jest.fn().mockImplementation((...args) => originalAes.aesEncrypt(...args)),
aesDecrypt: jest.fn().mockImplementation((...args) => originalAes.aesDecrypt(...args)),
}
})

const loggerErrorSpy = jest.spyOn(Logger, 'error')
const mockPepper = {
username: 'some username',
Expand All @@ -75,6 +86,18 @@ const expectPincodeEntered = () => {
expect(navigateBack).toHaveBeenCalled()
}

// Keep initial mocked implementation from __mocks__/react-native-keychain.ts
const originalGetGenericPassword = mockedKeychain.getGenericPassword.getMockImplementation()
const originalSetGenericPassword = mockedKeychain.setGenericPassword.getMockImplementation()
const originalResetGenericPassword = mockedKeychain.resetGenericPassword.getMockImplementation()

beforeEach(async () => {
// Reset to the original mocked implementation to get better isolation between tests
mockedKeychain.getGenericPassword.mockImplementation(originalGetGenericPassword)
mockedKeychain.setGenericPassword.mockImplementation(originalSetGenericPassword)
mockedKeychain.resetGenericPassword.mockImplementation(originalResetGenericPassword)
})

describe(getPasswordSaga, () => {
beforeEach(() => {
jest.clearAllMocks()
Expand Down Expand Up @@ -338,18 +361,22 @@ describe(updatePin, () => {
const newPasswordHash = 'd9bb2d77ec27dc8bf4269a6241daaa0388e8908518458f6ce0314380d11411cd'
// expectedAccountHash generated from normalizeAddress(mockAccount)
const accountHash = 'PASSWORD_HASH-0000000000000000000000000000000000007e57'
let encryptedMnemonicOldPin: string

beforeEach(() => {
beforeEach(async () => {
jest.clearAllMocks()
clearPasswordCaches()

encryptedMnemonicOldPin = await encryptMnemonic(mockMnemonic, oldPassword)

mockedKeychain.getGenericPassword.mockImplementation((options) => {
if (options?.service === 'PEPPER') {
return Promise.resolve(mockPepper)
}
if (options?.service === 'mnemonic') {
return Promise.resolve({
username: 'some username',
password: 'mockEncryptedValue',
password: encryptedMnemonicOldPin,
service: 'some service',
storage: 'some string',
})
Expand Down Expand Up @@ -390,13 +417,11 @@ describe(updatePin, () => {
expect(mockedKeychain.setGenericPassword).toHaveBeenNthCalledWith(
2,
'CELO',
'mockEncryptedValue',
expect.any(String), // Encrypted mnemonic new pin
expect.objectContaining({ service: 'mnemonic' })
)
// as we are mocking the outcome of encryption/decryption of mnemonic, check
// that they are called with the expected params
expect(CryptoJS.AES.decrypt).toHaveBeenCalledWith('mockEncryptedValue', oldPassword)
expect(CryptoJS.AES.encrypt).toHaveBeenCalledWith('mockDecryptedValue', newPassword)
expect(jest.mocked(aesDecrypt)).toHaveBeenCalledWith(encryptedMnemonicOldPin, oldPassword)
expect(jest.mocked(aesEncrypt)).toHaveBeenCalledWith(mockMnemonic, newPassword)
})

it('should update the cached pin, stored password, store mnemonic, and stored pin if biometry is enabled', async () => {
Expand All @@ -415,7 +440,7 @@ describe(updatePin, () => {
expect(mockedKeychain.setGenericPassword).toHaveBeenNthCalledWith(
3,
'CELO',
'mockEncryptedValue',
expect.any(String), // Encrypted mnemonic new pin
expect.objectContaining({ service: 'mnemonic' })
)
expect(mockedKeychain.setGenericPassword).toHaveBeenNthCalledWith(
Expand All @@ -432,7 +457,13 @@ describe(updatePin, () => {
})

describe(removeStoredPin, () => {
beforeEach(() => {
jest.clearAllMocks()
clearPasswordCaches()
})

it('should remove the item from keychain', async () => {
expect(mockedKeychain.resetGenericPassword).toHaveBeenCalledTimes(0)
mockedKeychain.resetGenericPassword.mockResolvedValueOnce(true)
await removeStoredPin()

Expand Down
36 changes: 36 additions & 0 deletions src/utils/aes.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// This test was initially copied from https://github.com/RaisinTen/aes-crypto-js/blob/2978af8e004d47539d767e751def003fe134b6e2/test.js
// And adapted for this project
import { aesDecrypt, aesEncrypt } from './aes'

describe.each([
[
'normal characters',
'Hello, world!',
'umm, shhh ...',
'U2FsdGVkX1+W9o0WI1QJGehALRoGMaRfoN2YH36BGTk=',
],
[
'weird characters in secret',
'Hello, world!',
'umm, šhhh ... 😀D◌̇랆탆𝐿 𑒹◌̴◌𑒺',
'U2FsdGVkX1/Eq6lXayqOFwfqTdefS3Zqi7LqOeWKrtA=',
],
[
'bytes corresponding to a single character that are split between two buffers',
'\u{30a8}\u{30b9}\u{30af}\u{30fc}\u{30c8}\u{3099}',
'umm, shhh ...',
'U2FsdGVkX18JW+58n/s+37y5831hmabBUuwtVf+JkaDZjeVyRNDHc+I/1w8kpAEA',
],
])('AES encryption and decryption: %s', (scenario, plainText, secret, encryptedByCryptoJS) => {
it('decrypts strings encrypted by crypto-js', () => {
// Note: encryptedByCryptoJS is the result of CryptoJS.AES.encrypt(plainText, secret).toString()
const decrypted = aesDecrypt(encryptedByCryptoJS, secret)
expect(decrypted).toBe(plainText)
})

it('decrypts strings encrypted with encryptAES', () => {
const encrypted = aesEncrypt(plainText, secret)
const decrypted = aesDecrypt(encrypted, secret)
expect(decrypted).toBe(plainText)
})
})
46 changes: 46 additions & 0 deletions src/utils/aes.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// This file was copied from https://github.com/RaisinTen/aes-crypto-js/blob/2978af8e004d47539d767e751def003fe134b6e2/index.js
// and modified slightly for TS compatibility.
import crypto from 'crypto'

// Refs: https://github.com/brix/crypto-js/issues/468#issuecomment-2060562277
export function aesEncrypt(plainText: string, secret: string) {
const salt = crypto.randomBytes(8)
const password = Buffer.concat([Buffer.from(secret), salt])
const hash = []
let digest = password
for (let i = 0; i < 3; i++) {
hash[i] = crypto.createHash('md5').update(digest).digest()
digest = Buffer.concat([hash[i], password])
}
const keyDerivation = Buffer.concat(hash)
const key = keyDerivation.subarray(0, 32)
const iv = keyDerivation.subarray(32)
const cipher = crypto.createCipheriv('aes-256-cbc', key, iv)
return Buffer.concat([
Buffer.from('Salted__', 'utf8'),
salt,
cipher.update(plainText),
cipher.final(),
]).toString('base64')
}

// Refs: https://github.com/brix/crypto-js/issues/468#issuecomment-1783351942
export function aesDecrypt(encryptedText: string, secret: string) {
// From https://gist.github.com/schakko/2628689?permalink_comment_id=3321113#gistcomment-3321113
// From https://gist.github.com/chengen/450129cb95c7159cb05001cc6bdbf6a1
const cypher = Buffer.from(encryptedText, 'base64')
const salt = cypher.slice(8, 16)
const password = Buffer.concat([Buffer.from(secret), salt])
const md5Hashes = []
let digest = password
for (let i = 0; i < 3; i++) {
md5Hashes[i] = crypto.createHash('md5').update(digest).digest()
digest = Buffer.concat([md5Hashes[i], password])
}
const key = Buffer.concat([md5Hashes[0], md5Hashes[1]])
const iv = md5Hashes[2]
const contents = cypher.slice(16)
const decipher = crypto.createDecipheriv('aes-256-cbc', key, iv)

return Buffer.concat([decipher.update(contents), decipher.final()]).toString('utf8')
}
3 changes: 0 additions & 3 deletions src/viem/getLockableWallet.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@ import {
} from 'viem/actions'
import { celoAlfajores, goerli as ethereumGoerli, sepolia as ethereumSepolia } from 'viem/chains'

// Use real encryption
jest.unmock('crypto-js')

jest.mock('src/viem', () => {
return {
viemTransports: {
Expand Down
3 changes: 0 additions & 3 deletions src/web3/KeychainAccounts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@ import {
mockPrivateKey,
} from 'test/values'

// Use real encryption
jest.unmock('crypto-js')

const MOCK_DATE = new Date('2016-12-21T23:36:07.071Z')

beforeEach(() => {
Expand Down
7 changes: 3 additions & 4 deletions src/web3/KeychainAccounts.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { isValidAddress, normalizeAddress, normalizeAddressWith0x } from '@celo/utils/lib/address'
import CryptoJS from 'crypto-js'
import { ErrorMessages } from 'src/app/ErrorMessages'
import { generateKeysFromMnemonic, getStoredMnemonic } from 'src/backup/utils'
import {
Expand All @@ -9,6 +8,7 @@ import {
storeItem,
} from 'src/storage/keychain'
import Logger from 'src/utils/Logger'
import { aesDecrypt, aesEncrypt } from 'src/utils/aes'
import { ViemKeychainAccount, keychainAccountToAccount } from 'src/viem/keychainAccountToAccount'
import { Hex } from 'viem'
import { Address, privateKeyToAddress } from 'viem/accounts'
Expand Down Expand Up @@ -39,13 +39,12 @@ function accountStorageKey(account: KeychainAccount) {
}

export async function encryptPrivateKey(privateKey: string, password: string) {
return CryptoJS.AES.encrypt(privateKey, password).toString()
return aesEncrypt(privateKey, password)
}

export async function decryptPrivateKey(encryptedPrivateKey: string, password: string) {
try {
const bytes = CryptoJS.AES.decrypt(encryptedPrivateKey, password)
return bytes.toString(CryptoJS.enc.Utf8)
return aesDecrypt(encryptedPrivateKey, password)
} catch (e) {
// decrypt can sometimes throw if the inputs are incorrect (encryptedPrivateKey or password)
Logger.warn(TAG, 'Failed to decrypt private key', e)
Expand Down
7 changes: 2 additions & 5 deletions src/web3/KeychainWallet.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ import { normalizeAddressWith0x, privateKeyToPublicKey } from '@celo/utils/lib/a
import { Encrypt } from '@celo/utils/lib/ecies'
import { verifySignature } from '@celo/utils/lib/signatureUtils'
import { recoverTransaction, verifyEIP712TypedDataSigner } from '@celo/wallet-base'
import CryptoJS from 'crypto-js'
import MockDate from 'mockdate'
import * as Keychain from 'react-native-keychain'
import { trimLeading0x } from 'src/utils/address'
import { aesEncrypt } from 'src/utils/aes'
import { UNLOCK_DURATION } from 'src/web3/consts'
import { KeychainAccounts } from 'src/web3/KeychainAccounts'
import { KeychainWallet } from 'src/web3/KeychainWallet'
Expand All @@ -20,9 +20,6 @@ import {
mockPrivateKey2,
} from 'test/values'

// Use real encryption
jest.unmock('crypto-js')

const CHAIN_ID = 44378

const ONE_CELO_IN_WEI = '1000000000000000000'
Expand Down Expand Up @@ -401,7 +398,7 @@ describe('KeychainWallet', () => {
// Setup mocked keychain content with a private key without the 0x prefix
mockedKeychain.setItems({
'account--2021-01-10T11:14:50.298Z--1be31a94361a391bbafb2a4ccd704f57dc04d4bb': {
password: await CryptoJS.AES.encrypt(mockPrivateKey, 'password').toString(),
password: await aesEncrypt(mockPrivateKey, 'password').toString(),
},
})

Expand Down
Loading

0 comments on commit d4171f3

Please sign in to comment.