From 136296aad6058c4bea8c3083e8ae553b2afcab98 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 22 Nov 2016 09:52:43 -0800 Subject: [PATCH 1/7] Began moving salt into encryptor --- app/scripts/keyring-controller.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index cf761c88c..e3705c113 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -55,7 +55,7 @@ module.exports = class KeyringController extends EventEmitter { return { seedWords: this.configManager.getSeedWords(), isInitialized: (!!wallet || !!vault), - isUnlocked: !!this.key, + isUnlocked: this.keyrings.length > 0, isDisclaimerConfirmed: this.configManager.getConfirmedDisclaimer(), // AUDIT this.configManager.getConfirmedDisclaimer(), unconfTxs: this.configManager.unconfirmedTxs(), transactions: this.configManager.getTxList(), @@ -137,7 +137,7 @@ module.exports = class KeyringController extends EventEmitter { createNewVault (password, entropy, cb) { const configManager = this.configManager - const salt = this.encryptor.generateSalt() + const salt = this.getSalt() configManager.setSalt(salt) return this.migrateAndGetKey(password) @@ -182,7 +182,7 @@ module.exports = class KeyringController extends EventEmitter { submitPassword (password, cb) { this.migrateAndGetKey(password) .then((key) => { - return this.unlockKeyrings(key) + return this.unlockKeyrings(password) }) .then((keyrings) => { this.keyrings = keyrings @@ -197,7 +197,7 @@ module.exports = class KeyringController extends EventEmitter { } loadKey (password) { - const salt = this.configManager.getSalt() || this.encryptor.generateSalt() + const salt = this.getSalt() return this.encryptor.keyFromPassword(password + salt) .then((key) => { this.key = key @@ -206,6 +206,11 @@ module.exports = class KeyringController extends EventEmitter { }) } + getSalt () { + const vault = this.configManager.getVault() + const salt = vault.salt || this.encryptor.generateSalt() + } + addNewKeyring (type, opts, cb) { const Keyring = this.getKeyringClassForType(type) const keyring = new Keyring(opts) @@ -288,7 +293,7 @@ module.exports = class KeyringController extends EventEmitter { }) } - unlockKeyrings (key) { + unlockKeyrings (password) { const encryptedVault = this.configManager.getVault() return this.encryptor.decryptWithKey(key, encryptedVault) .then((vault) => { From de8da9ddf67f36977e43d2168c4847fd9923c875 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 22 Nov 2016 15:54:51 -0800 Subject: [PATCH 2/7] Simplify Encryptor API Surface At least, the portion of it that we use. Moved salting within the encryptor, so it does not need to be managed externally. KeyringController now caches the password instead of a passwordDerivedKey, since it is ignorant of the salt. Encryptor payload is now in a JSON format, so its portions are both base64 encoded *and* labeled appropriately. The format is `{ "data": "0x0", "iv": "0x0", "salt": "string" }`. Fixes #843 Fixes #859 --- app/scripts/keyring-controller.js | 50 ++++++++-------------------- app/scripts/lib/encryptor.js | 26 ++++++++++----- test/unit/keyring-controller-test.js | 10 +++--- 3 files changed, 37 insertions(+), 49 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 00c04ea9b..b24c4bac5 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -113,34 +113,25 @@ module.exports = class KeyringController extends EventEmitter { }) } - migrateAndGetKey (password) { - let key + migrateOldVaultIfAny (password) { const shouldMigrate = !!this.configManager.getWallet() && !this.configManager.getVault() - return this.loadKey(password) - .then((derivedKey) => { - key = derivedKey - this.key = key - return this.idStoreMigrator.migratedVaultForPassword(password) - }) + return this.idStoreMigrator.migratedVaultForPassword(password) .then((serialized) => { if (serialized && shouldMigrate) { const keyring = this.restoreKeyring(serialized) this.keyrings.push(keyring) this.configManager.setSelectedAccount(keyring.getAccounts()[0]) return this.persistAllKeyrings() - .then(() => { return key }) + .then(() => { return }) } - return key + return }) } createNewVault (password, cb) { - const configManager = this.configManager - const salt = this.getSalt() - configManager.setSalt(salt) - - return this.migrateAndGetKey(password) + return this.migrateOldVaultIfAny(password) .then(() => { + this.password = password return this.persistAllKeyrings() }) .then(() => { @@ -184,8 +175,8 @@ module.exports = class KeyringController extends EventEmitter { } submitPassword (password, cb) { - this.migrateAndGetKey(password) - .then((key) => { + this.migrateOldVaultIfAny(password) + .then(() => { return this.unlockKeyrings(password) }) .then((keyrings) => { @@ -200,21 +191,6 @@ module.exports = class KeyringController extends EventEmitter { }) } - loadKey (password) { - const salt = this.getSalt() - return this.encryptor.keyFromPassword(password + salt) - .then((key) => { - this.key = key - this.configManager.setSalt(salt) - return key - }) - } - - getSalt () { - const vault = this.configManager.getVault() - const salt = vault.salt || this.encryptor.generateSalt() - } - addNewKeyring (type, opts, cb) { const Keyring = this.getKeyringClassForType(type) const keyring = new Keyring(opts) @@ -290,7 +266,7 @@ module.exports = class KeyringController extends EventEmitter { data: keyring.serialize(), } }) - return this.encryptor.encryptWithKey(this.key, serialized) + return this.encryptor.encrypt(this.password, serialized) .then((encryptedString) => { this.configManager.setVault(encryptedString) return true @@ -299,7 +275,7 @@ module.exports = class KeyringController extends EventEmitter { unlockKeyrings (password) { const encryptedVault = this.configManager.getVault() - return this.encryptor.decryptWithKey(key, encryptedVault) + return this.encryptor.decrypt(this.password, encryptedVault) .then((vault) => { vault.forEach(this.restoreKeyring.bind(this)) return this.keyrings @@ -407,7 +383,7 @@ module.exports = class KeyringController extends EventEmitter { }) } - function estimateGas(txData, blockGasLimitHex, cb) { + function estimateGas (txData, blockGasLimitHex, cb) { const txParams = txData.txParams // check if gasLimit is already specified txData.gasLimitSpecified = Boolean(txParams.gas) @@ -419,7 +395,7 @@ module.exports = class KeyringController extends EventEmitter { query.estimateGas(txParams, cb) } - function checkForGasError(txData, estimatedGasHex) { + function checkForGasError (txData, estimatedGasHex) { txData.estimatedGas = estimatedGasHex // all gas used - must be an error if (estimatedGasHex === txData.txParams.gas) { @@ -428,7 +404,7 @@ module.exports = class KeyringController extends EventEmitter { cb() } - function setTxGas(txData, blockGasLimitHex) { + function setTxGas (txData, blockGasLimitHex) { const txParams = txData.txParams // if OOG, nothing more to do if (txData.simulationFails) { diff --git a/app/scripts/lib/encryptor.js b/app/scripts/lib/encryptor.js index df72b62c0..8bacab766 100644 --- a/app/scripts/lib/encryptor.js +++ b/app/scripts/lib/encryptor.js @@ -26,10 +26,16 @@ module.exports = { // Takes a Pojo, returns cypher text. function encrypt (password, dataObj) { - return keyFromPassword(password) + const salt = this.generateSalt() + + return keyFromPassword(password + salt) .then(function (passwordDerivedKey) { return encryptWithKey(passwordDerivedKey, dataObj) }) + .then(function (payload) { + payload.salt = salt + return JSON.stringify(payload) + }) } function encryptWithKey (key, dataObj) { @@ -44,22 +50,26 @@ function encryptWithKey (key, dataObj) { var buffer = new Uint8Array(buf) var vectorStr = encodeBufferToBase64(vector) var vaultStr = encodeBufferToBase64(buffer) - return `${vaultStr}\\${vectorStr}` + return { + data: vaultStr, + iv: vectorStr, + } }) } // Takes encrypted text, returns the restored Pojo. function decrypt (password, text) { - return keyFromPassword(password) + const payload = JSON.parse(text) + const salt = payload.salt + return keyFromPassword(password + salt) .then(function (key) { - return decryptWithKey(key, text) + return decryptWithKey(key, payload) }) } -function decryptWithKey (key, text) { - const parts = text.split('\\') - const encryptedData = decodeBase64ToBuffer(parts[0]) - const vector = decodeBase64ToBuffer(parts[1]) +function decryptWithKey (key, payload) { + const encryptedData = decodeBase64ToBuffer(payload.data) + const vector = decodeBase64ToBuffer(payload.iv) return crypto.subtle.decrypt({name: 'AES-GCM', iv: vector}, key, encryptedData) .then(function (result) { const decryptedData = new Uint8Array(result) diff --git a/test/unit/keyring-controller-test.js b/test/unit/keyring-controller-test.js index 437441e0e..c32141cc6 100644 --- a/test/unit/keyring-controller-test.js +++ b/test/unit/keyring-controller-test.js @@ -82,13 +82,15 @@ describe('KeyringController', function() { }) - describe('#migrateAndGetKey', function() { + describe('#migrateOldVaultIfAny', function() { it('should return the key for that password', function(done) { - keyringController.migrateAndGetKey(password) - .then((key) => { - assert(key, 'a key is returned') + keyringController.migrateOldVaultIfAny(password) + .then(() => { done() }) + .catch((reason) => { + assert.ifError(reason) + }) }) }) From 7562d49db714f9b2e2a7a67718b5cfb1d4fae367 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 22 Nov 2016 15:56:54 -0800 Subject: [PATCH 3/7] Linted --- ui/lib/account-link.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/lib/account-link.js b/ui/lib/account-link.js index ff52d9c54..77db0851d 100644 --- a/ui/lib/account-link.js +++ b/ui/lib/account-link.js @@ -1,4 +1,4 @@ -module.exports = function(address, network) { +module.exports = function (address, network) { const net = parseInt(network) let link From 4b7b0a86d8e9688bd8a512342b99c76dc1810486 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 22 Nov 2016 15:57:48 -0800 Subject: [PATCH 4/7] Refine isInitialized derivation method --- app/scripts/keyring-controller.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index b24c4bac5..c736fa1f6 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -55,7 +55,7 @@ module.exports = class KeyringController extends EventEmitter { return { seedWords: this.configManager.getSeedWords(), isInitialized: (!!wallet || !!vault), - isUnlocked: this.keyrings.length > 0, + isUnlocked: Boolean(this.password), isDisclaimerConfirmed: this.configManager.getConfirmedDisclaimer(), // AUDIT this.configManager.getConfirmedDisclaimer(), unconfTxs: this.configManager.unconfirmedTxs(), transactions: this.configManager.getTxList(), From 607a474c3fef10aa6a50b27015263d874d25aa69 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 22 Nov 2016 16:05:37 -0800 Subject: [PATCH 5/7] Improve vault migration unit test --- test/unit/keyring-controller-test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/unit/keyring-controller-test.js b/test/unit/keyring-controller-test.js index c32141cc6..2d1064f40 100644 --- a/test/unit/keyring-controller-test.js +++ b/test/unit/keyring-controller-test.js @@ -83,9 +83,10 @@ describe('KeyringController', function() { }) describe('#migrateOldVaultIfAny', function() { - it('should return the key for that password', function(done) { + it('should return and init a new vault', function(done) { keyringController.migrateOldVaultIfAny(password) .then(() => { + assert(Boolean(localStorage['vault'])) done() }) .catch((reason) => { From 358440384c769c82fec148e7a24e3e4f6366b4b4 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 22 Nov 2016 16:07:35 -0800 Subject: [PATCH 6/7] Fix vault migration unit test --- test/unit/keyring-controller-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/keyring-controller-test.js b/test/unit/keyring-controller-test.js index 2d1064f40..a58043c7a 100644 --- a/test/unit/keyring-controller-test.js +++ b/test/unit/keyring-controller-test.js @@ -86,7 +86,7 @@ describe('KeyringController', function() { it('should return and init a new vault', function(done) { keyringController.migrateOldVaultIfAny(password) .then(() => { - assert(Boolean(localStorage['vault'])) + assert(keyringController.configManager.getVault(), 'now has a vault') done() }) .catch((reason) => { From 6ebdebc0a5287bce18947fff3e7812bcac43ce36 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 22 Nov 2016 16:18:18 -0800 Subject: [PATCH 7/7] Remove line of cruft --- app/scripts/keyring-controller.js | 1 - 1 file changed, 1 deletion(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index c736fa1f6..4fa2b4ee8 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -122,7 +122,6 @@ module.exports = class KeyringController extends EventEmitter { this.keyrings.push(keyring) this.configManager.setSelectedAccount(keyring.getAccounts()[0]) return this.persistAllKeyrings() - .then(() => { return }) } return })