From c01bc1c36373a3bfcba3049bc960cd1e9d6af4a0 Mon Sep 17 00:00:00 2001 From: Laurent Cozic Date: Wed, 22 Jan 2020 22:01:58 +0000 Subject: [PATCH] All: Added new, more secure encryption methods, so that they can be switched to at a later time --- CliClient/tests/encryption.js | 72 ++++++++- CliClient/tests/synchronizer.js | 4 +- CliClient/tests/test-utils.js | 10 +- .../lib/services/EncryptionService.js | 152 ++++++++++++------ 4 files changed, 180 insertions(+), 58 deletions(-) diff --git a/CliClient/tests/encryption.js b/CliClient/tests/encryption.js index be53a9e84b..fc04fb5261 100644 --- a/CliClient/tests/encryption.js +++ b/CliClient/tests/encryption.js @@ -41,7 +41,7 @@ describe('Encryption', function() { }; const encodedHeader = service.encodeHeader_(header); - const decodedHeader = service.decodeHeader_(encodedHeader); + const decodedHeader = service.decodeHeaderBytes_(encodedHeader); delete decodedHeader.length; expect(objectsEqual(header, decodedHeader)).toBe(true); @@ -54,14 +54,14 @@ describe('Encryption', function() { let hasThrown = false; try { - await service.decryptMasterKey(masterKey, 'wrongpassword'); + await service.decryptMasterKey_(masterKey, 'wrongpassword'); } catch (error) { hasThrown = true; } expect(hasThrown).toBe(true); - const decryptedMasterKey = await service.decryptMasterKey(masterKey, '123456'); + const decryptedMasterKey = await service.decryptMasterKey_(masterKey, '123456'); expect(decryptedMasterKey.length).toBe(512); })); @@ -69,7 +69,7 @@ describe('Encryption', function() { let masterKey = await service.generateMasterKey('123456'); masterKey = await MasterKey.save(masterKey); - await service.loadMasterKey(masterKey, '123456', true); + await service.loadMasterKey_(masterKey, '123456', true); const cipherText = await service.encryptString('some secret'); const plainText = await service.decryptString(cipherText); @@ -87,11 +87,37 @@ describe('Encryption', function() { expect(plainText2 === veryLongSecret).toBe(true); })); + it('should decrypt various encryption methods', asyncTest(async () => { + let masterKey = await service.generateMasterKey('123456'); + masterKey = await MasterKey.save(masterKey); + await service.loadMasterKey_(masterKey, '123456', true); + + { + const cipherText = await service.encryptString('some secret', { + encryptionMethod: EncryptionService.METHOD_SJCL_2, + }); + const plainText = await service.decryptString(cipherText); + expect(plainText).toBe('some secret'); + const header = await service.decodeHeaderString(cipherText); + expect(header.encryptionMethod).toBe(EncryptionService.METHOD_SJCL_2); + } + + { + const cipherText = await service.encryptString('some secret', { + encryptionMethod: EncryptionService.METHOD_SJCL_3, + }); + const plainText = await service.decryptString(cipherText); + expect(plainText).toBe('some secret'); + const header = await service.decodeHeaderString(cipherText); + expect(header.encryptionMethod).toBe(EncryptionService.METHOD_SJCL_3); + } + })); + it('should fail to decrypt if master key not present', asyncTest(async () => { let masterKey = await service.generateMasterKey('123456'); masterKey = await MasterKey.save(masterKey); - await service.loadMasterKey(masterKey, '123456', true); + await service.loadMasterKey_(masterKey, '123456', true); const cipherText = await service.encryptString('some secret'); @@ -107,7 +133,7 @@ describe('Encryption', function() { let masterKey = await service.generateMasterKey('123456'); masterKey = await MasterKey.save(masterKey); - await service.loadMasterKey(masterKey, '123456', true); + await service.loadMasterKey_(masterKey, '123456', true); let cipherText = await service.encryptString('some secret'); cipherText += 'ABCDEFGHIJ'; @@ -120,7 +146,7 @@ describe('Encryption', function() { it('should encrypt and decrypt notes and folders', asyncTest(async () => { let masterKey = await service.generateMasterKey('123456'); masterKey = await MasterKey.save(masterKey); - await service.loadMasterKey(masterKey, '123456', true); + await service.loadMasterKey_(masterKey, '123456', true); let folder = await Folder.save({ title: 'folder' }); let note = await Note.save({ title: 'encrypted note', body: 'something', parent_id: folder.id }); @@ -151,7 +177,7 @@ describe('Encryption', function() { it('should encrypt and decrypt files', asyncTest(async () => { let masterKey = await service.generateMasterKey('123456'); masterKey = await MasterKey.save(masterKey); - await service.loadMasterKey(masterKey, '123456', true); + await service.loadMasterKey_(masterKey, '123456', true); const sourcePath = `${__dirname}/../tests/support/photo.jpg`; const encryptedPath = `${__dirname}/data/photo.crypted`; @@ -164,4 +190,34 @@ describe('Encryption', function() { expect(fileContentEqual(sourcePath, decryptedPath)).toBe(true); })); + // it('should upgrade master key encryption mode', asyncTest(async () => { + // let masterKey = await service.generateMasterKey('123456', { + // encryptionMethod: EncryptionService.METHOD_SJCL_2, + // }); + // masterKey = await MasterKey.save(masterKey); + // Setting.setObjectKey('encryption.passwordCache', masterKey.id, '123456'); + // Setting.setValue('encryption.activeMasterKeyId', masterKey.id); + + // await sleep(0.01); + + // await service.loadMasterKeysFromSettings(); + + // masterKeyNew = await MasterKey.load(masterKey.id); + + // // Check that the master key has been upgraded + + // expect(masterKeyNew.created_time).toBe(masterKey.created_time); + // expect(masterKeyNew.updated_time === masterKey.updated_time).toBe(false); + // expect(masterKeyNew.content === masterKey.content).toBe(false); + // expect(masterKeyNew.encryption_method === masterKey.encryption_method).toBe(false); + // expect(masterKeyNew.checksum === masterKey.checksum).toBe(false); + // expect(masterKeyNew.encryption_method).toBe(service.defaultMasterKeyEncryptionMethod_); + + // // Check that encryption still works + + // const cipherText = await service.encryptString('some secret'); + // const plainText = await service.decryptString(cipherText); + // expect(plainText).toBe('some secret'); + // })); + }); diff --git a/CliClient/tests/synchronizer.js b/CliClient/tests/synchronizer.js index 52bab66e4d..b0dce87935 100644 --- a/CliClient/tests/synchronizer.js +++ b/CliClient/tests/synchronizer.js @@ -539,7 +539,7 @@ describe('Synchronizer', function() { let context2 = await synchronizer().start(); if (withEncryption) { const masterKey_2 = await MasterKey.load(masterKey.id); - await encryptionService().loadMasterKey(masterKey_2, '123456', true); + await encryptionService().loadMasterKey_(masterKey_2, '123456', true); let t = await Tag.load(tag.id); await Tag.decrypt(t); } @@ -743,7 +743,7 @@ describe('Synchronizer', function() { expect(masterKey_2.content).toBe(masterKey.content); expect(masterKey_2.checksum).toBe(masterKey.checksum); // Now load the master key we got from client 1 and try to decrypt - await encryptionService().loadMasterKey(masterKey_2, '123456', true); + await encryptionService().loadMasterKey_(masterKey_2, '123456', true); // Get the decrypted items back await Folder.decrypt(folder1_2); await Note.decrypt(note1_2); diff --git a/CliClient/tests/test-utils.js b/CliClient/tests/test-utils.js index 274759e0bf..7aed20f552 100644 --- a/CliClient/tests/test-utils.js +++ b/CliClient/tests/test-utils.js @@ -299,7 +299,7 @@ async function loadEncryptionMasterKey(id = null, useExisting = false) { masterKey = masterKeys[0]; } - await service.loadMasterKey(masterKey, '123456', true); + await service.loadMasterKey_(masterKey, '123456', true); return masterKey; } @@ -370,8 +370,12 @@ function asyncTest(callback) { try { await callback(); } catch (error) { - console.error(error); - expect('good').toBe('not good', 'Test has thrown an exception - see above error'); + if (error.constructor && error.constructor.name === 'ExpectationFailed') { + // OK - will be reported by Jasmine + } else { + console.error(error); + expect(0).toBe(1, 'Test has thrown an exception - see above error'); + } } finally { done(); } diff --git a/ReactNativeClient/lib/services/EncryptionService.js b/ReactNativeClient/lib/services/EncryptionService.js index 3a4896bf56..fd81250938 100644 --- a/ReactNativeClient/lib/services/EncryptionService.js +++ b/ReactNativeClient/lib/services/EncryptionService.js @@ -29,10 +29,13 @@ class EncryptionService { this.loadedMasterKeys_ = {}; this.activeMasterKeyId_ = null; this.defaultEncryptionMethod_ = EncryptionService.METHOD_SJCL; + this.defaultMasterKeyEncryptionMethod_ = EncryptionService.METHOD_SJCL_2; this.logger_ = new Logger(); this.headerTemplates_ = { + // Template version 1 1: { + // Fields are defined as [name, valueSize, valueType] fields: [['encryptionMethod', 2, 'int'], ['masterKeyId', 32, 'hex']], }, }; @@ -98,13 +101,20 @@ class EncryptionService { this.logger().info(`Trying to load ${masterKeys.length} master keys...`); for (let i = 0; i < masterKeys.length; i++) { - const mk = masterKeys[i]; + let mk = masterKeys[i]; const password = passwords[mk.id]; if (this.isMasterKeyLoaded(mk.id)) continue; if (!password) continue; try { - await this.loadMasterKey(mk, password, activeMasterKeyId === mk.id); + // if (mk.encryption_method != this.defaultMasterKeyEncryptionMethod_) { + // const newMkContent = await this.generateMasterKeyContent_(password); + // mk = Object.assign({}, mk, newMkContent); + // await MasterKey.save(mk); + // this.logger().info(`Master key ${mk.id} is using a deprectated encryption method. It has been upgraded to the new method.`); + // } + + await this.loadMasterKey_(mk, password, activeMasterKeyId === mk.id); } catch (error) { this.logger().warn(`Cannot load master key ${mk.id}. Invalid password?`, error); } @@ -147,9 +157,9 @@ class EncryptionService { return !!this.loadedMasterKeys_[id]; } - async loadMasterKey(model, password, makeActive = false) { + async loadMasterKey_(model, password, makeActive = false) { if (!model.id) throw new Error('Master key does not have an ID - save it first'); - this.loadedMasterKeys_[model.id] = await this.decryptMasterKey(model, password); + this.loadedMasterKeys_[model.id] = await this.decryptMasterKey_(model, password); if (makeActive) this.setActiveMasterKeyId(model.id); } @@ -219,29 +229,35 @@ class EncryptionService { .join(''); } - async generateMasterKey(password) { + async generateMasterKeyContent_(password, options = null) { + options = Object.assign({}, { + encryptionMethod: this.defaultMasterKeyEncryptionMethod_, + }, options); + const bytes = await shim.randomBytes(256); - const hexaBytes = bytes - .map(a => { - return hexPad(a.toString(16), 2); - }) - .join(''); + const hexaBytes = bytes.map(a => hexPad(a.toString(16), 2)).join(''); const checksum = this.sha256(hexaBytes); - const encryptionMethod = EncryptionService.METHOD_SJCL_2; - const cipherText = await this.encrypt(encryptionMethod, password, hexaBytes); - const now = Date.now(); + const cipherText = await this.encrypt(options.encryptionMethod, password, hexaBytes); return { - created_time: now, - updated_time: now, - source_application: Setting.value('appId'), - encryption_method: encryptionMethod, checksum: checksum, + encryption_method: options.encryptionMethod, content: cipherText, }; } - async decryptMasterKey(model, password) { + async generateMasterKey(password, options = null) { + const model = await this.generateMasterKeyContent_(password, options); + + const now = Date.now(); + model.created_time = now; + model.updated_time = now; + model.source_application = Setting.value('appId'); + + return model; + } + + async decryptMasterKey_(model, password) { const plainText = await this.decrypt(model.encryption_method, password, model.content); const checksum = this.sha256(plainText); if (checksum !== model.checksum) throw new Error('Could not decrypt master key (checksum failed)'); @@ -250,7 +266,7 @@ class EncryptionService { async checkMasterKeyPassword(model, password) { try { - await this.decryptMasterKey(model, password); + await this.decryptMasterKey_(model, password); } catch (error) { return false; } @@ -264,12 +280,13 @@ class EncryptionService { const sjcl = shim.sjclModule; + // 2020-01-23: Deprecated and no longer secure due to the use og OCB2 mode - do not use. if (method === EncryptionService.METHOD_SJCL) { try { // Good demo to understand each parameter: https://bitwiseshiftleft.github.io/sjcl/demo/ return sjcl.json.encrypt(key, plainText, { v: 1, // version - iter: 1000, // Defaults to 10000 in sjcl but since we're running this on mobile devices, use a lower value. Maybe review this after some time. https://security.stackexchange.com/questions/3959/recommended-of-iterations-when-using-pkbdf2-sha256 + iter: 1000, // Defaults to 1000 in sjcl but since we're running this on mobile devices, use a lower value. Maybe review this after some time. https://security.stackexchange.com/questions/3959/recommended-of-iterations-when-using-pkbdf2-sha256 ks: 128, // Key size - "128 bits should be secure enough" ts: 64, // ??? mode: 'ocb2', // The cipher mode is a standard for how to use AES and other algorithms to encrypt and authenticate your message. OCB2 mode is slightly faster and has more features, but CCM mode has wider support because it is not patented. @@ -282,7 +299,8 @@ class EncryptionService { } } - // Same as first one but slightly more secure (but slower) to encrypt master keys + // 2020-01-23: Deprectated - see above. + // Was used to encrypt master keys if (method === EncryptionService.METHOD_SJCL_2) { try { return sjcl.json.encrypt(key, plainText, { @@ -299,6 +317,41 @@ class EncryptionService { } } + if (method === EncryptionService.METHOD_SJCL_3) { + try { + // Good demo to understand each parameter: https://bitwiseshiftleft.github.io/sjcl/demo/ + return sjcl.json.encrypt(key, plainText, { + v: 1, // version + iter: 1000, // Defaults to 1000 in sjcl. Since we're running this on mobile devices we need to be careful it doesn't affect performances too much. Maybe review this after some time. https://security.stackexchange.com/questions/3959/recommended-of-iterations-when-using-pkbdf2-sha256 + ks: 128, // Key size - "128 bits should be secure enough" + ts: 64, // ??? + mode: 'ccm', // The cipher mode is a standard for how to use AES and other algorithms to encrypt and authenticate your message. OCB2 mode is slightly faster and has more features, but CCM mode has wider support because it is not patented. + // "adata":"", // Associated Data - not needed? + cipher: 'aes', + }); + } catch (error) { + // SJCL returns a string as error which means stack trace is missing so convert to an error object here + throw new Error(error.message); + } + } + + // Same as above but more secure (but slower) to encrypt master keys + if (method === EncryptionService.METHOD_SJCL_4) { + try { + return sjcl.json.encrypt(key, plainText, { + v: 1, + iter: 10000, + ks: 256, + ts: 64, + mode: 'ccm', + cipher: 'aes', + }); + } catch (error) { + // SJCL returns a string as error which means stack trace is missing so convert to an error object here + throw new Error(error.message); + } + } + throw new Error(`Unknown encryption method: ${method}`); } @@ -307,23 +360,22 @@ class EncryptionService { if (!key) throw new Error('Encryption key is required'); const sjcl = shim.sjclModule; + if (!this.isValidEncryptionMethod(method)) throw new Error(`Unknown decryption method: ${method}`); - if (method === EncryptionService.METHOD_SJCL || method === EncryptionService.METHOD_SJCL_2) { - try { - return sjcl.json.decrypt(key, cipherText); - } catch (error) { - // SJCL returns a string as error which means stack trace is missing so convert to an error object here - throw new Error(error.message); - } + try { + return sjcl.json.decrypt(key, cipherText); + } catch (error) { + // SJCL returns a string as error which means stack trace is missing so convert to an error object here + throw new Error(error.message); } - - throw new Error(`Unknown decryption method: ${method}`); } async encryptAbstract_(source, destination, options = null) { - if (!options) options = {}; + options = Object.assign({}, { + encryptionMethod: this.defaultEncryptionMethod(), + }, options); - const method = this.defaultEncryptionMethod(); + const method = options.encryptionMethod; const masterKeyId = this.activeMasterKeyId(); const masterKeyPlainText = this.loadedMasterKey(masterKeyId); @@ -357,13 +409,7 @@ class EncryptionService { async decryptAbstract_(source, destination, options = null) { if (!options) options = {}; - const identifier = await source.read(5); - if (!this.isValidHeaderIdentifier(identifier)) throw new JoplinError(`Invalid encryption identifier. Data is not actually encrypted? ID was: ${identifier}`, 'invalidIdentifier'); - const mdSizeHex = await source.read(6); - const mdSize = parseInt(mdSizeHex, 16); - if (isNaN(mdSize) || !mdSize) throw new Error(`Invalid header metadata size: ${mdSizeHex}`); - const md = await source.read(parseInt(mdSizeHex, 16)); - const header = this.decodeHeader_(identifier + mdSizeHex + md); + const header = await this.decodeHeaderSource_(source); const masterKeyPlainText = this.loadedMasterKey(header.masterKeyId); let doneSize = 0; @@ -501,11 +547,6 @@ class EncryptionService { await cleanUp(); } - decodeHeaderVersion_(hexaByte) { - if (hexaByte.length !== 2) throw new Error(`Invalid header version length: ${hexaByte}`); - return parseInt(hexaByte, 16); - } - headerTemplate(version) { const r = this.headerTemplates_[version]; if (!r) throw new Error(`Unknown header version: ${version}`); @@ -523,7 +564,22 @@ class EncryptionService { return `JED01${encryptionMetadata}`; } - decodeHeader_(headerHexaBytes) { + async decodeHeaderString(cipherText) { + const source = this.stringReader_(cipherText); + return this.decodeHeaderSource_(source); + } + + async decodeHeaderSource_(source) { + const identifier = await source.read(5); + if (!this.isValidHeaderIdentifier(identifier)) throw new JoplinError(`Invalid encryption identifier. Data is not actually encrypted? ID was: ${identifier}`, 'invalidIdentifier'); + const mdSizeHex = await source.read(6); + const mdSize = parseInt(mdSizeHex, 16); + if (isNaN(mdSize) || !mdSize) throw new Error(`Invalid header metadata size: ${mdSizeHex}`); + const md = await source.read(parseInt(mdSizeHex, 16)); + return this.decodeHeaderBytes_(identifier + mdSizeHex + md); + } + + decodeHeaderBytes_(headerHexaBytes) { const reader = this.stringReader_(headerHexaBytes, true); const identifier = reader.read(3); const version = parseInt(reader.read(2), 16); @@ -561,6 +617,10 @@ class EncryptionService { return /JED\d\d/.test(id); } + isValidEncryptionMethod(method) { + return [EncryptionService.METHOD_SJCL, EncryptionService.METHOD_SJCL_2, EncryptionService.METHOD_SJCL_3, EncryptionService.METHOD_SJCL_4].indexOf(method) >= 0; + } + async itemIsEncrypted(item) { if (!item) throw new Error('No item'); const ItemClass = BaseItem.itemClass(item); @@ -578,6 +638,8 @@ class EncryptionService { EncryptionService.METHOD_SJCL = 1; EncryptionService.METHOD_SJCL_2 = 2; +EncryptionService.METHOD_SJCL_3 = 3; +EncryptionService.METHOD_SJCL_4 = 4; EncryptionService.fsDriver_ = null;