diff --git a/chrome/content/zotero/xpcom/osKeyStore.js b/chrome/content/zotero/xpcom/osKeyStore.js index 5fce04c794..5d4c9d1d05 100644 --- a/chrome/content/zotero/xpcom/osKeyStore.js +++ b/chrome/content/zotero/xpcom/osKeyStore.js @@ -63,17 +63,22 @@ Zotero.OSKeyStore = { || Services.wm.getMostRecentWindow('navigator:browser'); }, - // Show an alert when an active write of new credentials fails (e.g., keychain unavailable) - alertSaveFailed: function () { - let win = this._getParentWindow(); - if (!win) { - return; - } - Zotero.alert( - win, - Zotero.getString('general-error'), - Zotero.getString('os-keystore-save-failed') - ); + // Offer to store credentials unencrypted after a write to the keystore fails. The keystore + // can be unusable in ways the user can't fix -- most often on Linux, where the platform + // requires a Secret Service that some managed systems don't run. + // + // Returns true if the user agrees. + confirmUnencryptedFallback: function () { + let index = Zotero.Prompt.confirm({ + window: this._getParentWindow(), + title: Zotero.getString('general-error'), + text: Zotero.getString('os-keystore-save-failed') + "\n\n" + + Zotero.getString('os-keystore-save-unencrypted'), + button0: Zotero.getString('os-keystore-save-unencrypted-button'), + button1: Zotero.Prompt.BUTTON_TITLE_CANCEL, + defaultButton: 1 + }); + return index == 0; }, // Show a one-shot alert when migration of an existing legacy plaintext entry diff --git a/chrome/content/zotero/xpcom/storage/webdav.js b/chrome/content/zotero/xpcom/storage/webdav.js index 3cae543244..ff79479c47 100644 --- a/chrome/content/zotero/xpcom/storage/webdav.js +++ b/chrome/content/zotero/xpcom/storage/webdav.js @@ -273,7 +273,23 @@ Zotero.Sync.Storage.Mode.WebDAV.prototype = { }); for (var i = 0; i < logins.length; i++) { if (logins[i].username == username) { - return Zotero.OSKeyStore.decrypt(logins[i].password); + let password = logins[i].password; + if (Zotero.OSKeyStore.isEncrypted(password)) { + return Zotero.OSKeyStore.decrypt(password); + } + // Password stored without encryption after a failed write -- encrypt it now, + // in case the keystore has since become usable + if (!this._reencryptedPassword) { + this._reencryptedPassword = true; + try { + Zotero.debug("Encrypting unencrypted WebDAV password"); + await this._writeEncryptedPassword(username, password); + } + catch (e) { + Zotero.logError(e); + } + } + return password; } } @@ -314,24 +330,7 @@ Zotero.Sync.Storage.Mode.WebDAV.prototype = { this._basicAuthHeader = false; this._digestParams = null; - try { - await this._writeEncryptedPassword(username, password); - } - catch (e) { - // If the write failed because the key database was unusable, reset the login - // manager and retry - if (!(await Zotero.Sync.Data.Local.repairLoginManager())) { - Zotero.OSKeyStore.alertSaveFailed(); - throw e; - } - try { - await this._writeEncryptedPassword(username, password); - } - catch (e) { - Zotero.OSKeyStore.alertSaveFailed(); - throw e; - } - } + await this._savePassword(username, password); // Drop any leftover plaintext entry from the legacy realm var logins = await Services.logins.searchLoginsAsync({ @@ -368,7 +367,48 @@ Zotero.Sync.Storage.Mode.WebDAV.prototype = { } }, + /** + * Store the password, retrying after a login manager repair and, if the keystore still + * can't be used, offering to store the password without it + */ + async _savePassword(username, password) { + var error; + try { + await this._writeEncryptedPassword(username, password); + return; + } + catch (e) { + Zotero.logError(e); + error = e; + } + // If the write failed because the key database was unusable, reset the login manager + // and retry + if (await Zotero.Sync.Data.Local.repairLoginManager()) { + try { + await this._writeEncryptedPassword(username, password); + return; + } + catch (e) { + Zotero.logError(e); + } + } + // A password that would read back as ciphertext can't be stored unencrypted + if (Zotero.OSKeyStore.isEncrypted(password)) { + throw error; + } + if (!Zotero.OSKeyStore.confirmUnencryptedFallback()) { + throw error; + } + await this._writePassword(username, password); + // The keystore is unusable, so don't try to encrypt again on the next read + this._reencryptedPassword = true; + }, + async _writeEncryptedPassword(username, password) { + await this._writePassword(username, password && await Zotero.OSKeyStore.encrypt(password)); + }, + + async _writePassword(username, storedValue) { // Remove any existing entries in the encrypted realm for this user var logins = await Services.logins.searchLoginsAsync({ origin: this._loginManagerHost, @@ -380,8 +420,7 @@ Zotero.Sync.Storage.Mode.WebDAV.prototype = { } } - if (password) { - let storedValue = await Zotero.OSKeyStore.encrypt(password); + if (storedValue) { let nsLoginInfo = new Components.Constructor("@mozilla.org/login-manager/loginInfo;1", Components.interfaces.nsILoginInfo, "init"); let loginInfo = new nsLoginInfo(this._loginManagerHost, null, diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js index 15effcb0d7..49c088b02a 100644 --- a/chrome/content/zotero/xpcom/sync/syncLocal.js +++ b/chrome/content/zotero/xpcom/sync/syncLocal.js @@ -88,7 +88,22 @@ Zotero.Sync.Data.Local = { } var login = await this._getAPIKeyLoginInfo(); if (login) { - return Zotero.OSKeyStore.decrypt(login.password); + if (Zotero.OSKeyStore.isEncrypted(login.password)) { + return Zotero.OSKeyStore.decrypt(login.password); + } + // Key stored without encryption after a failed write -- encrypt it now, in case + // the keystore has since become usable + if (!this._reencryptedAPIKey) { + this._reencryptedAPIKey = true; + try { + Zotero.debug("Encrypting unencrypted API key"); + await this._writeEncryptedAPIKey(login.password); + } + catch (e) { + Zotero.logError(e); + } + } + return login.password; } // If the login manager had to be reset, the stored API key is gone, so tell the user // to log in again @@ -154,24 +169,7 @@ Zotero.Sync.Data.Local = { return; } - try { - await this._writeEncryptedAPIKey(apiKey); - } - catch (e) { - // If the write failed because the key database was unusable, reset the login - // manager and retry, so that logging in works without a manual fix - if (!(await this.repairLoginManager())) { - Zotero.OSKeyStore.alertSaveFailed(); - throw e; - } - try { - await this._writeEncryptedAPIKey(apiKey); - } - catch (e) { - Zotero.OSKeyStore.alertSaveFailed(); - throw e; - } - } + await this._saveAPIKey(apiKey); // Drop any leftover plaintext entry from the legacy realm if (legacyLoginInfo) { await Services.logins.removeLoginAsync(legacyLoginInfo); @@ -181,9 +179,47 @@ Zotero.Sync.Data.Local = { }, + /** + * Store the API key, retrying after a login manager repair and, if the keystore still + * can't be used, offering to store the key without it + */ + _saveAPIKey: async function (apiKey) { + var error; + try { + await this._writeEncryptedAPIKey(apiKey); + return; + } + catch (e) { + Zotero.logError(e); + error = e; + } + // If the write failed because the key database was unusable, reset the login manager + // and retry, so that logging in works without a manual fix + if (await this.repairLoginManager()) { + try { + await this._writeEncryptedAPIKey(apiKey); + return; + } + catch (e) { + Zotero.logError(e); + } + } + if (!Zotero.OSKeyStore.confirmUnencryptedFallback()) { + throw error; + } + await this._writeAPIKey(apiKey); + // The keystore is unusable, so don't try to encrypt again on the next read + this._reencryptedAPIKey = true; + }, + + _writeEncryptedAPIKey: async function (apiKey) { + await this._writeAPIKey(await Zotero.OSKeyStore.encrypt(apiKey)); + }, + + + _writeAPIKey: async function (storedValue) { var oldLoginInfo = await this._getAPIKeyLoginInfo(); - var storedValue = await Zotero.OSKeyStore.encrypt(apiKey); var nsLoginInfo = new Components.Constructor("@mozilla.org/login-manager/loginInfo;1", Components.interfaces.nsILoginInfo, "init"); var loginInfo = new nsLoginInfo( diff --git a/chrome/locale/en-US/zotero/zotero.ftl b/chrome/locale/en-US/zotero/zotero.ftl index fa4d86c783..4414593588 100644 --- a/chrome/locale/en-US/zotero/zotero.ftl +++ b/chrome/locale/en-US/zotero/zotero.ftl @@ -1189,15 +1189,19 @@ login-manager-reset = { -app-name } was unable to read your saved login informat os-keystore-save-failed = { PLATFORM() -> [macos] { -app-name } couldn’t access the { -os-name } Keychain to securely save your credentials. Make sure your Keychain is accessible and try again. - [windows] { -app-name } couldn’t securely save your credentials. Try again or restart { -app-name }. - *[other] { -app-name } couldn’t access your { -os-name } keyring to securely save your credentials. Make sure a keyring service is running and try again. + [windows] { -app-name } couldn’t use { -os-name } Credential Manager to securely save your credentials. Try again or restart { -app-name }. + *[other] { -app-name } couldn’t access your { -os-name } keyring to securely save your credentials. Make sure a keyring service such as GNOME Keyring or KWallet is running and try again. } +os-keystore-save-unencrypted = { -app-name } can save your credentials unencrypted instead. Anyone with access to your { -app-name } profile folder would then be able to read them. + +os-keystore-save-unencrypted-button = Save Anyway + os-keystore-migrate-failed = { PLATFORM() -> [macos] { -app-name } couldn’t access the { -os-name } Keychain to encrypt your stored credentials. Your credentials remain stored unencrypted on disk. Make sure your Keychain is accessible and restart { -app-name }. [windows] { -app-name } couldn’t encrypt your stored credentials. Your credentials remain stored unencrypted on disk. Restart { -app-name } and try again. - *[other] { -app-name } couldn’t access your { -os-name } keyring to encrypt your stored credentials. Your credentials remain stored unencrypted on disk. Make sure a keyring service is running and restart { -app-name }. + *[other] { -app-name } couldn’t access your { -os-name } keyring to encrypt your stored credentials. Your credentials remain stored unencrypted on disk. Make sure a keyring service such as GNOME Keyring or KWallet is running and restart { -app-name }. } search-button = diff --git a/test/tests/syncLocalTest.js b/test/tests/syncLocalTest.js index 68643f3e47..3aec3c07c5 100644 --- a/test/tests/syncLocalTest.js +++ b/test/tests/syncLocalTest.js @@ -18,6 +18,95 @@ describe("Zotero.Sync.Data.Local", function () { await Zotero.Sync.Data.Local.setAPIKey(""); assert.strictEqual(await Zotero.Sync.Data.Local.getAPIKey(apiKey), ""); }) + + + it("should store the key without encryption if the keystore is unusable and the user agrees", async function () { + var apiKey = Zotero.Utilities.randomString(24); + var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt") + .rejects(new Error("User canceled OS unlock entry")); + var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback") + .returns(true); + try { + await Zotero.Sync.Data.Local.setAPIKey(apiKey); + assert.ok(confirmStub.called); + assert.equal(await Zotero.Sync.Data.Local.getAPIKey(), apiKey); + // The read shouldn't have retried encryption against the same broken keystore + assert.equal(encryptStub.callCount, 1); + } + finally { + encryptStub.restore(); + confirmStub.restore(); + await Zotero.Sync.Data.Local.setAPIKey(""); + } + }) + + + it("shouldn't store the key if the keystore is unusable and the user declines", async function () { + var apiKey = Zotero.Utilities.randomString(24); + var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt") + .rejects(new Error("User canceled OS unlock entry")); + var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback") + .returns(false); + try { + var e = await getPromiseError(Zotero.Sync.Data.Local.setAPIKey(apiKey)); + assert.ok(e); + assert.strictEqual(await Zotero.Sync.Data.Local.getAPIKey(), ""); + } + finally { + encryptStub.restore(); + confirmStub.restore(); + await Zotero.Sync.Data.Local.setAPIKey(""); + } + }) + + + it("should prompt before storing the key without encryption", async function () { + var apiKey = Zotero.Utilities.randomString(24); + var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt") + .rejects(new Error("User canceled OS unlock entry")); + var promptStub = sinon.stub(Zotero.Prompt, "confirm").returns(0); + try { + await Zotero.Sync.Data.Local.setAPIKey(apiKey); + assert.ok(promptStub.calledOnce); + assert.equal(await Zotero.Sync.Data.Local.getAPIKey(), apiKey); + } + finally { + encryptStub.restore(); + promptStub.restore(); + await Zotero.Sync.Data.Local.setAPIKey(""); + } + }) + + + it("should encrypt a key stored without encryption in a later session", async function () { + if (!Zotero.OSKeyStore.available) { + this.skip(); + } + var apiKey = Zotero.Utilities.randomString(24); + // Store it with an unusable keystore + var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt") + .rejects(new Error("User canceled OS unlock entry")); + var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback") + .returns(true); + try { + await Zotero.Sync.Data.Local.setAPIKey(apiKey); + } + finally { + encryptStub.restore(); + confirmStub.restore(); + } + try { + // Encryption is retried once per session, so stand in for a restart + Zotero.Sync.Data.Local._reencryptedAPIKey = false; + assert.equal(await Zotero.Sync.Data.Local.getAPIKey(), apiKey); + let login = await Zotero.Sync.Data.Local._getAPIKeyLoginInfo(); + assert.ok(Zotero.OSKeyStore.isEncrypted(login.password)); + assert.equal(await Zotero.Sync.Data.Local.getAPIKey(), apiKey); + } + finally { + await Zotero.Sync.Data.Local.setAPIKey(""); + } + }) }) diff --git a/test/tests/webdavTest.js b/test/tests/webdavTest.js index bc1018f187..b1f5bd601f 100644 --- a/test/tests/webdavTest.js +++ b/test/tests/webdavTest.js @@ -259,6 +259,40 @@ describe("Zotero.Sync.Storage.Mode.WebDAV", function () { await controller.setPassword(password); assert.equal(await controller.getPassword(), password); }) + + + it("shouldn't store a password that would read back as encrypted", async function () { + var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt") + .rejects(new Error("User canceled OS unlock entry")); + var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback") + .returns(true); + try { + var e = await getPromiseError(controller.setPassword("oskv1:example")); + assert.ok(e); + } + finally { + encryptStub.restore(); + confirmStub.restore(); + } + }) + + + it("should return a password stored without encryption after a keystore failure", async function () { + var password = "p\u00e4ssw\u20acrd"; + var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt") + .rejects(new Error("User canceled OS unlock entry")); + var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback") + .returns(true); + try { + await controller.setPassword(password); + assert.ok(confirmStub.called); + assert.equal(await controller.getPassword(), password); + } + finally { + encryptStub.restore(); + confirmStub.restore(); + } + }) }) describe("Syncing", function () {