diff --git a/.eslintrc.js b/.eslintrc.js index f371d0a7..cd0604b3 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -1,6 +1,9 @@ module.exports = { root: true, extends: ['@metamask/eslint-config'], + globals: { + window: 'readonly', + }, env: { commonjs: true, }, diff --git a/index.js b/index.js index bef532bf..465e6c30 100644 --- a/index.js +++ b/index.js @@ -4,16 +4,21 @@ const bip39 = require('@metamask/bip39'); const ObservableStore = require('obs-store'); const encryptor = require('browser-passworder'); const { normalize: normalizeAddress } = require('eth-sig-util'); +//const { sha256 } = require('ethereum-cryptography/sha256'); +//const { utf8ToBytes, toHex } = require('ethereum-cryptography/utils'); const SimpleKeyring = require('eth-simple-keyring'); const HdKeyring = require('@metamask/eth-hd-keyring'); const keyringTypes = [SimpleKeyring, HdKeyring]; +const VAULT_SEPARATOR = ':::'; + const KEYRINGS_TYPE_MAP = { HD_KEYRING: 'HD Key Tree', SIMPLE_KEYRING: 'Simple Key Pair', }; + /** * Strip the hex prefix from an address, if present * @param {string} address - The address that might be hex prefixed. @@ -125,6 +130,7 @@ class KeyringController extends EventEmitter { mnemonic: seedPhraseAsBuffer, numberOfAccounts: 1, }, + password, ); const [firstAccount] = await firstKeyring.getAccounts(); if (!firstAccount) { @@ -145,8 +151,8 @@ class KeyringController extends EventEmitter { */ async setLocked() { // set locked - this.password = null; this.memStore.updateState({ isUnlocked: false }); + delete this.encryptionKey; // remove keyrings this.keyrings = []; await this._updateMemStoreKeyrings(); @@ -168,7 +174,29 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async submitPassword(password) { + await this.verifyPassword(password); this.keyrings = await this.unlockKeyrings(password); + + // We should persist keyrings so that we can either + // (1) migrate or (2) create a new salt + await this.persistAllKeyrings(password); + + this.setUnlocked(); + this.fullUpdate(); + + return this.encryptionKey; + } + + /** + * Submit Encrypted Key + * + * Attempts to decrypt the current vault with a given encryption key + * and loads its keyrings into memory + * + * @param {string} password + */ + async submitEncryptionKey(encryptionKey) { + this.keyrings = await this.unlockKeyrings(undefined, encryptionKey); this.setUnlocked(); this.fullUpdate(); } @@ -186,7 +214,12 @@ class KeyringController extends EventEmitter { if (!encryptedVault) { throw new Error('Cannot unlock without a previous vault.'); } - await this.encryptor.decrypt(password, encryptedVault); + + const result = await this.attemptGetDecryptedVault( + encryptedVault, + password, + ); + return result; } /** @@ -202,7 +235,7 @@ class KeyringController extends EventEmitter { * @param {Object} opts - The constructor options for the keyring. * @returns {Promise} The new keyring. */ - async addNewKeyring(type, opts) { + async addNewKeyring(type, opts, password) { const Keyring = this.getKeyringClassForType(type); const keyring = new Keyring(opts); if ((!opts || !opts.mnemonic) && type === KEYRINGS_TYPE_MAP.HD_KEYRING) { @@ -214,7 +247,7 @@ class KeyringController extends EventEmitter { await this.checkForDuplicate(type, accounts); this.keyrings.push(keyring); - await this.persistAllKeyrings(); + await this.persistAllKeyrings(password); await this._updateMemStoreKeyrings(); this.fullUpdate(); @@ -491,10 +524,13 @@ class KeyringController extends EventEmitter { * @returns {Promise} - A promise that resolves if the operation was successful. */ async createFirstKeyTree(password) { - this.password = password; this.clearKeyrings(); - const keyring = await this.addNewKeyring(KEYRINGS_TYPE_MAP.HD_KEYRING); + const keyring = await this.addNewKeyring( + KEYRINGS_TYPE_MAP.HD_KEYRING, + {}, + password, + ); const [firstAccount] = await keyring.getAccounts(); if (!firstAccount) { throw new Error('KeyringController - No account found on keychain.'); @@ -516,12 +552,37 @@ class KeyringController extends EventEmitter { * @param {string} password - The keyring controller password. * @returns {Promise} Resolves to true once keyrings are persisted. */ - async persistAllKeyrings(password = this.password) { - if (typeof password !== 'string') { + async persistAllKeyrings(password) { + if (password && typeof password !== 'string') { throw new Error('KeyringController - password is not a string'); } - this.password = password; + // Since we also allow persisting without a password, + // we should require this.encryptionKey + if (password === undefined && this.encryptionKey === undefined) { + throw new Error( + 'KeyringController - a password or encryptionKey must exist to persist keyrings', + ); + } + + let salt = null; + if (password) { + // If this is a migration or new password-driven login, we should + // create or rotate the salt + salt = this.encryptor.generateSalt(); + + // Since there's a new salt, we need to generate a new encrypted key + // for use in the + this.encryptionKey = await this._generateEncryptionKey(password, salt); + } else { + const encryptedVault = this.store.getState().vault; + if (!encryptedVault) { + throw new Error('Cannot unlock without a previous vault.'); + } + // We can use an existing salt if one exists in the encrypted key + salt = this.parseVault(encryptedVault).salt; + } + const serializedKeyrings = await Promise.all( this.keyrings.map(async (keyring) => { const [type, data] = await Promise.all([ @@ -531,14 +592,50 @@ class KeyringController extends EventEmitter { return { type, data }; }), ); + const encryptedString = await this.encryptor.encrypt( - this.password, + this.encryptionKey, serializedKeyrings, ); - this.store.updateState({ vault: encryptedString }); + + if (!encryptedString || !salt) { + throw new Error( + 'Cannot persist vault without salt or encrypted vault string', + ); + } + + const newVault = [encryptedString, VAULT_SEPARATOR, salt].join(''); + + // The encrypted string gets concatenated with a separator and salt + this.store.updateState({ vault: newVault }); return true; } + async attemptGetDecryptedVault(encryptedVault, password, encryptionKey) { + if (password === undefined && encryptionKey === undefined) { + throw new Error( + 'No way to decrypt a salted vault without a password or encryption key', + ); + } + + // If the separator string is in the vault string, the user has already migrated + // from the previous password-only model + if (encryptedVault.includes(VAULT_SEPARATOR)) { + const { salt, vault } = this.parseVault(encryptedVault); + + if (encryptionKey) { + this.encryptionKey = encryptionKey; + } else { + this.encryptionKey = await this._generateEncryptionKey(password, salt); + } + + //return await this.encryptor.decrypt(this.encryptionKey, vault); + return await this.encryptor.decryptWithKey(this.encryptionKey, encryptedVault); + } + + return await this.encryptor.decrypt(password, encryptedVault); + } + /** * Unlock Keyrings * @@ -548,20 +645,71 @@ class KeyringController extends EventEmitter { * @param {string} password - The keyring controller password. * @returns {Promise>} The keyrings. */ - async unlockKeyrings(password) { + async unlockKeyrings(password, encryptionKey) { const encryptedVault = this.store.getState().vault; if (!encryptedVault) { throw new Error('Cannot unlock without a previous vault.'); } + if (password === undefined && encryptionKey === undefined) { + throw new Error( + 'No way to decrypt a salted vault without a password or encrypted key', + ); + } + await this.clearKeyrings(); - const vault = await this.encryptor.decrypt(password, encryptedVault); - this.password = password; + + const vault = await this.attemptGetDecryptedVault( + encryptedVault, + password, + encryptionKey, + ); + await Promise.all(vault.map(this._restoreKeyring.bind(this))); + await this._updateMemStoreKeyrings(); + return this.keyrings; } + /** + * Parse Vault + * + * Parses the vault string for vault and salt + * + * @param {string} encryptedVault - The stored, encrypted vault + * @returns {Boject} Contains salt and vault properties + */ + parseVault(encryptedVault) { + const [vault, salt] = encryptedVault.split(VAULT_SEPARATOR); + return { vault, salt }; + } + + /** + * Generate Encryption Key + * + * Generates an encryption key which will be used to decrypt + * the vault. + * + * @param {Object} password - The user's submitted password + * @param {Object} salt - Salt to generate the encryption key + * @returns {string} The encryption key + */ + async _generateEncryptionKey(password, salt) { + //return toHex(sha256(utf8ToBytes(password + salt))); + const key = await encryptor.keyFromPassword(password, salt); + const exportedKey = await window.crypto.subtle.exportKey('jwk', key); + const keyString = JSON.stringify(exportedKey); + console.log("key is: ", key, exportedKey); + console.log("keystring is: ", keyString); + + // recreate the key? + const restoredKey = await window.crypto.subtle.importKey('jwk', JSON.parse(keyString), 'AES-GCM', true, ['encrypt', 'decrypt']); + console.log('restored key is: ', restoredKey); + + return keyString; + } + /** * Restore Keyring * @@ -762,4 +910,4 @@ class KeyringController extends EventEmitter { } } -module.exports = KeyringController; +module.exports = KeyringController; \ No newline at end of file diff --git a/package.json b/package.json index a1953c3e..2bc4dbd5 100644 --- a/package.json +++ b/package.json @@ -36,6 +36,7 @@ "browser-passworder": "^2.0.3", "eth-sig-util": "^3.0.1", "eth-simple-keyring": "^4.2.0", + "ethereum-cryptography": "^1.1.2", "obs-store": "^4.0.3" }, "devDependencies": { diff --git a/test/index.js b/test/index.js index 35d8000e..a1611167 100644 --- a/test/index.js +++ b/test/index.js @@ -6,9 +6,14 @@ const sinon = require('sinon'); const Wallet = require('ethereumjs-wallet').default; const KeyringController = require('..'); -const mockEncryptor = require('./lib/mock-encryptor'); +const { MOCK_SALT } = require('./lib/constants'); +const generateMockEncryptor = require('./lib/mock-encryptor'); + +const PASSWORD = 'password123'; +const ENCRYPTED_PASSWORD = + '60f78b86639ce48cc7b29cbe640506cce8c1c467841aaf816dd159df6ef94857'; // toHex(sha256(utf8ToBytes(PASSWORD + MOCK_SALT))) +const mockEncryptor = generateMockEncryptor(); -const password = 'password123'; const walletOneSeedWords = 'puzzle seed penalty soldier say clay field arctic metal hen cage runway'; const walletOneAddresses = ['0xef35ca8ebb9669a35c31b5f6f249a9941a812ac1']; @@ -20,6 +25,7 @@ const walletTwoAddresses = [ '0xbbafcf3d00fb625b65bb1497c94bf42c1f4b3e78', '0x49dd2653f38f75d40fdbd51e83b9c9724c87f7eb', ]; + describe('KeyringController', function () { let keyringController; @@ -28,11 +34,13 @@ describe('KeyringController', function () { encryptor: mockEncryptor, }); - await keyringController.createNewVaultAndKeychain(password); + await keyringController.createNewVaultAndKeychain(PASSWORD); + await keyringController.submitPassword(PASSWORD); }); afterEach(function () { sinon.restore(); + sinon.resetHistory(); }); describe('setLocked', function () { @@ -45,7 +53,7 @@ describe('KeyringController', function () { await keyringController.setLocked(); - expect(keyringController.password).toBeNull(); + expect(keyringController.encryptionKey).toBeUndefined(); expect(keyringController.memStore.getState().isUnlocked).toBe(false); expect(keyringController.keyrings).toHaveLength(0); }); @@ -62,14 +70,21 @@ describe('KeyringController', function () { describe('submitPassword', function () { it('should not create new keyrings when called in series', async function () { - await keyringController.createNewVaultAndKeychain(password); - await keyringController.persistAllKeyrings(); + await keyringController.createNewVaultAndKeychain(PASSWORD); + await keyringController.persistAllKeyrings(PASSWORD); expect(keyringController.keyrings).toHaveLength(1); - await keyringController.submitPassword(`${password}a`); + await expect( + keyringController.submitPassword(`${PASSWORD}a`), + ).rejects.toThrow('Incorrect password'); expect(keyringController.keyrings).toHaveLength(1); - await keyringController.submitPassword(''); + await expect(keyringController.submitPassword('')).rejects.toThrow( + 'Incorrect password', + ); + expect(keyringController.keyrings).toHaveLength(1); + + await keyringController.submitPassword(PASSWORD); expect(keyringController.keyrings).toHaveLength(1); }); @@ -79,7 +94,7 @@ describe('KeyringController', function () { const spy = sinon.spy(); keyringController.on('unlock', spy); - await keyringController.submitPassword(password); + await keyringController.submitPassword(PASSWORD); expect(spy.calledOnce).toBe(true); }); }); @@ -89,22 +104,28 @@ describe('KeyringController', function () { keyringController.store.updateState({ vault: null }); assert(!keyringController.store.getState().vault, 'no previous vault'); - await keyringController.createNewVaultAndKeychain(password); + await keyringController.createNewVaultAndKeychain(PASSWORD); const { vault } = keyringController.store.getState(); // eslint-disable-next-line jest/no-restricted-matchers - expect(vault).toBeTruthy(); + expect(Boolean(vault)).toBe(true); }); it('should encrypt keyrings with the correct password each time they are persisted', async function () { keyringController.store.updateState({ vault: null }); assert(!keyringController.store.getState().vault, 'no previous vault'); + sinon.spy(keyringController, '_generateEncryptionKey'); + await keyringController.createNewVaultAndKeychain(PASSWORD); + + expect(keyringController._generateEncryptionKey.args[0]).toStrictEqual([ + PASSWORD, + MOCK_SALT, + ]); - await keyringController.createNewVaultAndKeychain(password); const { vault } = keyringController.store.getState(); // eslint-disable-next-line jest/no-restricted-matchers expect(vault).toBeTruthy(); keyringController.encryptor.encrypt.args.forEach(([actualPassword]) => { - expect(actualPassword).toBe(password); + expect(actualPassword).toBe(ENCRYPTED_PASSWORD); }); }); }); @@ -119,7 +140,7 @@ describe('KeyringController', function () { expect(allAccounts).toHaveLength(2); await keyringController.createNewVaultAndRestore( - password, + PASSWORD, walletOneSeedWords, ); @@ -137,7 +158,7 @@ describe('KeyringController', function () { it('throws error if mnemonic passed is invalid', async function () { await expect(() => keyringController.createNewVaultAndRestore( - password, + PASSWORD, 'test test test palace city barely security section midnight wealth south deer', ), ).rejects.toThrow('Seed phrase is invalid.'); @@ -151,7 +172,7 @@ describe('KeyringController', function () { ); await keyringController.createNewVaultAndRestore( - password, + PASSWORD, mnemonicAsArrayOfNumbers, ); @@ -303,7 +324,7 @@ describe('KeyringController', function () { describe('unlockKeyrings', function () { it('returns the list of keyrings', async function () { await keyringController.setLocked(); - const keyrings = await keyringController.unlockKeyrings(password); + const keyrings = await keyringController.unlockKeyrings(PASSWORD); expect(keyrings).toHaveLength(1); keyrings.forEach((keyring) => { expect(keyring.wallets).toHaveLength(1); @@ -451,4 +472,86 @@ describe('KeyringController', function () { ); }); }); + + describe('Manifest v3 changes', function () { + it('can still verify password', async function () { + // Log the user out + await keyringController.setLocked(); + + // Attempt to verify the password + await expect( + keyringController.verifyPassword(PASSWORD), + // eslint-disable-next-line jest/no-restricted-matchers + ).resolves.toBeTruthy(); + }); + + it('can still unlock with password after being migrated and locked', async function () { + // Log the user out + await keyringController.setLocked(); + + // Attempt to unlock the keychaing via old password + await expect( + keyringController.submitPassword(PASSWORD), + // eslint-disable-next-line jest/no-restricted-matchers + ).resolves.toBeTruthy(); + + // Ensure the new vault value has separator and salt + const { vault } = keyringController.store.getState(); + expect(vault).toContain(':::'); + }); + + // TODO: THIS SHOULD BE FAILING + it('does not unlock with incorrect encryption key', async function () { + await keyringController.submitPassword(PASSWORD); + + // Log the user out + await keyringController.setLocked(); + + // Attempt to login with an incorrect encryption key + await expect( + keyringController.submitEncryptionKey('FAKE KEY'), + ).rejects.toThrow('Incorrect password'); + }); + + it('unlocks with correct encryption key', async function () { + await keyringController.submitPassword(PASSWORD); + + const { encryptionKey } = keyringController; + + // Log the user out + await keyringController.setLocked(); + + // Attempt to login with an incorrect encryption key + await keyringController.submitEncryptionKey(encryptionKey); + + expect(Boolean(keyringController.encryptionKey)).toBe(true); + }); + + it('removes encryption key when locked', async function () { + await keyringController.setLocked(); + + expect(keyringController.encryptionKey).toBeUndefined(); + }); + + it('rotates encrypted keys between logins', async function () { + keyringController = new KeyringController({ + encryptor: generateMockEncryptor(true), + }); + + await keyringController.createNewVaultAndKeychain(PASSWORD); + + // Log in with correct password, which will trigger generation of key + await keyringController.submitPassword(PASSWORD); + + const currentKey = keyringController.encryptionKey; + + await keyringController.setLocked(); + + await keyringController.submitPassword(PASSWORD); + + const newKey = keyringController.encryptionKey; + + expect(currentKey !== newKey).toBe(true); + }); + }); }); diff --git a/test/lib/constants.js b/test/lib/constants.js new file mode 100644 index 00000000..d2b40014 --- /dev/null +++ b/test/lib/constants.js @@ -0,0 +1,3 @@ +module.exports = { + MOCK_SALT: 'WHADDASALT!', +}; diff --git a/test/lib/mock-encryptor.js b/test/lib/mock-encryptor.js index 6089a370..7614a890 100644 --- a/test/lib/mock-encryptor.js +++ b/test/lib/mock-encryptor.js @@ -1,32 +1,42 @@ const sinon = require('sinon'); +const { MOCK_SALT } = require('./constants'); const mockHex = '0xabcdef0123456789'; const mockKey = Buffer.alloc(32); let cacheVal; -module.exports = { - encrypt: sinon.stub().callsFake(function (_password, dataObj) { - cacheVal = dataObj; - return Promise.resolve(mockHex); - }), +module.exports = function generateMockEncryptor(changeSaltBetweenCalls) { + return { + encrypt: sinon.stub().callsFake(function (_password, dataObj) { + cacheVal = dataObj; + cacheVal.password = _password; + return Promise.resolve(mockHex); + }), - decrypt(_password, _text) { - return Promise.resolve(cacheVal || {}); - }, + decrypt(_password, _text) { + if (_password !== cacheVal.password) { + throw new Error(`Incorrect password: ${_password} != ${cacheVal.password}`); + } + return Promise.resolve(cacheVal || {}); + }, - encryptWithKey(key, dataObj) { - return this.encrypt(key, dataObj); - }, + encryptWithKey(key, dataObj) { + return this.encrypt(key, dataObj); + }, - decryptWithKey(key, text) { - return this.decrypt(key, text); - }, + decryptWithKey(key, text) { + return this.decrypt(key, text); + }, - keyFromPassword(_password) { - return Promise.resolve(mockKey); - }, + keyFromPassword(_password) { + return Promise.resolve(mockKey); + }, - generateSalt() { - return 'WHADDASALT!'; - }, + generateSalt() { + if (changeSaltBetweenCalls) { + return Date.now(); + } + return MOCK_SALT; + }, + }; }; diff --git a/yarn.lock b/yarn.lock index 8d6fd5be..89708440 100644 --- a/yarn.lock +++ b/yarn.lock @@ -584,6 +584,16 @@ tweetnacl "^1.0.3" tweetnacl-util "^0.15.1" +"@noble/hashes@1.1.2", "@noble/hashes@~1.1.1": + version "1.1.2" + resolved "https://registry.yarnpkg.com/@noble/hashes/-/hashes-1.1.2.tgz#e9e035b9b166ca0af657a7848eb2718f0f22f183" + integrity sha512-KYRCASVTv6aeUi1tsF8/vpyR7zpfs3FUzy2Jqm+MU+LmUKhQ0y2FpfwqkCcxSg2ua4GALJd8k2R76WxwZGbQpA== + +"@noble/secp256k1@1.6.3", "@noble/secp256k1@~1.6.0": + version "1.6.3" + resolved "https://registry.yarnpkg.com/@noble/secp256k1/-/secp256k1-1.6.3.tgz#7eed12d9f4404b416999d0c87686836c4c5c9b94" + integrity sha512-T04e4iTurVy7I8Sw4+c5OSN9/RkPlo1uKxAomtxQNLq8j1uPAqnsqG1bqvY3Jv7c13gyr6dui0zmh/I3+f/JaQ== + "@nodelib/fs.scandir@2.1.5": version "2.1.5" resolved "https://registry.yarnpkg.com/@nodelib/fs.scandir/-/fs.scandir-2.1.5.tgz#7619c2eb21b25483f6d167548b4cfd5a7488c3d5" @@ -628,6 +638,28 @@ node-gyp "^7.1.0" read-package-json-fast "^2.0.1" +"@scure/base@~1.1.0": + version "1.1.1" + resolved "https://registry.yarnpkg.com/@scure/base/-/base-1.1.1.tgz#ebb651ee52ff84f420097055f4bf46cfba403938" + integrity sha512-ZxOhsSyxYwLJj3pLZCefNitxsj093tb2vq90mp2txoYeBqbcjDjqFhyM8eUjq/uFm6zJ+mUuqxlS2FkuSY1MTA== + +"@scure/bip32@1.1.0": + version "1.1.0" + resolved "https://registry.yarnpkg.com/@scure/bip32/-/bip32-1.1.0.tgz#dea45875e7fbc720c2b4560325f1cf5d2246d95b" + integrity sha512-ftTW3kKX54YXLCxH6BB7oEEoJfoE2pIgw7MINKAs5PsS6nqKPuKk1haTF/EuHmYqG330t5GSrdmtRuHaY1a62Q== + dependencies: + "@noble/hashes" "~1.1.1" + "@noble/secp256k1" "~1.6.0" + "@scure/base" "~1.1.0" + +"@scure/bip39@1.1.0": + version "1.1.0" + resolved "https://registry.yarnpkg.com/@scure/bip39/-/bip39-1.1.0.tgz#92f11d095bae025f166bef3defcc5bf4945d419a" + integrity sha512-pwrPOS16VeTKg98dYXQyIjJEcWfz7/1YJIwxUEPFfQPtc86Ym/1sVgQ2RLoD43AazMk2l/unK4ITySSpW2+82w== + dependencies: + "@noble/hashes" "~1.1.1" + "@scure/base" "~1.1.0" + "@sinonjs/commons@^1.6.0", "@sinonjs/commons@^1.8.3": version "1.8.3" resolved "https://registry.yarnpkg.com/@sinonjs/commons/-/commons-1.8.3.tgz#3802ddd21a50a949b6721ddd72da36e67e7f1b2d" @@ -1984,6 +2016,16 @@ ethereum-cryptography@^0.1.3: secp256k1 "^4.0.1" setimmediate "^1.0.5" +ethereum-cryptography@^1.1.2: + version "1.1.2" + resolved "https://registry.yarnpkg.com/ethereum-cryptography/-/ethereum-cryptography-1.1.2.tgz#74f2ac0f0f5fe79f012c889b3b8446a9a6264e6d" + integrity sha512-XDSJlg4BD+hq9N2FjvotwUET9Tfxpxc3kWGE2AqUG5vcbeunnbImVk3cj6e/xT3phdW21mE8R5IugU4fspQDcQ== + dependencies: + "@noble/hashes" "1.1.2" + "@noble/secp256k1" "1.6.3" + "@scure/bip32" "1.1.0" + "@scure/bip39" "1.1.0" + ethereumjs-abi@^0.6.8: version "0.6.8" resolved "https://registry.yarnpkg.com/ethereumjs-abi/-/ethereumjs-abi-0.6.8.tgz#71bc152db099f70e62f108b7cdfca1b362c6fcae"