From e520f78577c60a715a47c9971a1aea1f6ca9a0d1 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Sat, 14 Jun 2014 14:33:20 +1000 Subject: [PATCH] Bug 1013064 (part 2) - Store sensitive FxA credentials in the login manager. r=ckarlof From 4a92f9ee1ba35989f82a24bba18806f8616a5be8 Mon Sep 17 00:00:00 2001 --- services/fxaccounts/FxAccounts.jsm | 170 ++++++++++++++++++ services/fxaccounts/FxAccountsCommon.js | 13 ++ services/fxaccounts/moz.build | 5 +- .../tests/xpcshell/test_loginmgr_storage.js | 196 +++++++++++++++++++++ services/fxaccounts/tests/xpcshell/xpcshell.ini | 2 + services/sync/modules/util.js | 12 +- 6 files changed, 396 insertions(+), 2 deletions(-) create mode 100644 services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js --- services/fxaccounts/FxAccounts.jsm | 170 +++++++++++++++ services/fxaccounts/FxAccountsCommon.js | 13 ++ services/fxaccounts/moz.build | 5 +- .../tests/xpcshell/test_loginmgr_storage.js | 196 ++++++++++++++++++ .../fxaccounts/tests/xpcshell/xpcshell.ini | 2 + services/sync/modules/util.js | 12 +- 6 files changed, 396 insertions(+), 2 deletions(-) create mode 100644 services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js diff --git a/services/fxaccounts/FxAccounts.jsm b/services/fxaccounts/FxAccounts.jsm index 5fb78a72ff7f..3379c456d05d 100644 --- a/services/fxaccounts/FxAccounts.jsm +++ b/services/fxaccounts/FxAccounts.jsm @@ -303,7 +303,12 @@ function FxAccountsInternal() { // We don't reference |profileDir| in the top-level module scope // as we may be imported before we know where it is. + // We only want the fancy new LoginManagerStorage on desktop. +#if defined(MOZ_B2G) this.signedInUserStorage = new JSONStorage({ +#else + this.signedInUserStorage = new LoginManagerStorage({ +#endif filename: DEFAULT_STORAGE_FILENAME, baseDir: OS.Constants.Path.profileDir, }); @@ -901,6 +906,171 @@ JSONStorage.prototype = { } }; +/** + * LoginManagerStorage constructor that creates instances that may set/get + * from a combination of a clear-text JSON file and stored securely in + * the nsILoginManager. + * + * @param options { + * filename: of the plain-text file to write to + * baseDir: directory where the file resides + * } + * @return instance + */ + +function LoginManagerStorage(options) { + // we reuse the JSONStorage for writing the plain-text stuff. + this.jsonStorage = new JSONStorage(options); +} + +LoginManagerStorage.prototype = { + // The fields in the credentials JSON object that are stored in plain-text + // in the profile directory. All other fields are stored in the login manager, + // and thus are only available when the master-password is unlocked. + + // a hook point for testing. + get _isLoggedIn() { + return Services.logins.isLoggedIn; + }, + + // Clear any data from the login manager. Returns true if the login manager + // was unlocked (even if no existing logins existed) or false if it was + // locked (meaning we don't even know if it existed or not.) + _clearLoginMgrData: Task.async(function* () { + try { // Services.logins might be third-party and broken... + yield Services.logins.initializationPromise; + if (!this._isLoggedIn) { + return false; + } + let logins = Services.logins.findLogins({}, FXA_PWDMGR_HOST, null, FXA_PWDMGR_REALM); + for (let login of logins) { + Services.logins.removeLogin(login); + } + return true; + } catch (ex) { + log.error("Failed to clear login data: ${}", ex); + return false; + } + }), + + set: Task.async(function* (contents) { + if (!contents) { + // User is signing out - write the null to the json file. + yield this.jsonStorage.set(contents); + + // And nuke it from the login manager. + let cleared = yield this._clearLoginMgrData(); + if (!cleared) { + // just log a message - we verify that the email address matches when + // we reload it, so having a stale entry doesn't really hurt. + log.info("not removing credentials from login manager - not logged in"); + } + return; + } + + // We are saving actual data. + // Split the data into 2 chunks - one to go to the plain-text, and the + // other to write to the login manager. + let toWriteJSON = {version: contents.version}; + let accountDataJSON = toWriteJSON.accountData = {}; + let toWriteLoginMgr = {version: contents.version}; + let accountDataLoginMgr = toWriteLoginMgr.accountData = {}; + for (let [name, value] of Iterator(contents.accountData)) { + if (FXA_PWDMGR_PLAINTEXT_FIELDS.indexOf(name) >= 0) { + accountDataJSON[name] = value; + } else { + accountDataLoginMgr[name] = value; + } + } + yield this.jsonStorage.set(toWriteJSON); + + try { // Services.logins might be third-party and broken... + // and the stuff into the login manager. + yield Services.logins.initializationPromise; + // If MP is locked we silently fail - the user may need to re-auth + // next startup. + if (!this._isLoggedIn) { + log.info("not saving credentials to login manager - not logged in"); + return; + } + // write the rest of the data to the login manager. + let loginInfo = new Components.Constructor( + "@mozilla.org/login-manager/loginInfo;1", Ci.nsILoginInfo, "init"); + let login = new loginInfo(FXA_PWDMGR_HOST, + null, // aFormSubmitURL, + FXA_PWDMGR_REALM, // aHttpRealm, + contents.accountData.email, // aUsername + JSON.stringify(toWriteLoginMgr), // aPassword + "", // aUsernameField + "");// aPasswordField + + let existingLogins = Services.logins.findLogins({}, FXA_PWDMGR_HOST, null, + FXA_PWDMGR_REALM); + if (existingLogins.length) { + Services.logins.modifyLogin(existingLogins[0], login); + } else { + Services.logins.addLogin(login); + } + } catch (ex) { + log.error("Failed to save data to the login manager: ${}", ex); + } + }), + + get: Task.async(function* () { + // we need to suck some data from the .json file in the profile dir and + // some other from the login manager. + let data = yield this.jsonStorage.get(); + if (!data) { + // no user logged in, nuke the storage data incase we couldn't remove + // it previously and then we are done. + yield this._clearLoginMgrData(); + return null; + } + + // if we have encryption keys it must have been saved before we + // used the login manager, so re-save it. + if (data.accountData.kA || data.accountData.kB || data.keyFetchToken) { + log.info("account data needs migration to the login manager."); + yield this.set(data); + } + + try { // Services.logins might be third-party and broken... + // read the data from the login manager and merge it for return. + yield Services.logins.initializationPromise; + + if (!this._isLoggedIn) { + log.info("returning partial account data as the login manager is locked."); + return data; + } + + let logins = Services.logins.findLogins({}, FXA_PWDMGR_HOST, null, FXA_PWDMGR_REALM); + if (logins.length == 0) { + // This could happen if the MP was locked when we wrote the data. + log.info("Can't find the rest of the credentials in the login manager"); + return data; + } + let login = logins[0]; + if (login.username == data.accountData.email) { + let lmData = JSON.parse(login.password); + if (lmData.version == data.version) { + // Merge the login manager data + copyObjectProperties(lmData.accountData, data.accountData); + } else { + log.info("version field in the login manager doesn't match - ignoring it"); + yield this._clearLoginMgrData(); + } + } else { + log.info("username in the login manager doesn't match - ignoring it"); + yield this._clearLoginMgrData(); + } + } catch (ex) { + log.error("Failed to get data from the login manager: ${}", ex); + } + return data; + }), + +} + // A getter for the instance to export XPCOMUtils.defineLazyGetter(this, "fxAccounts", function() { let a = new FxAccounts(); diff --git a/services/fxaccounts/FxAccountsCommon.js b/services/fxaccounts/FxAccountsCommon.js index d3a2756f2f9c..267e5ecdb7af 100644 --- a/services/fxaccounts/FxAccountsCommon.js +++ b/services/fxaccounts/FxAccountsCommon.js @@ -178,5 +178,18 @@ SERVER_ERRNO_TO_ERROR[ERRNO_INCORRECT_EMAIL_CASE] = ERROR_INCORRECT_EM SERVER_ERRNO_TO_ERROR[ERRNO_SERVICE_TEMP_UNAVAILABLE] = ERROR_SERVICE_TEMP_UNAVAILABLE; SERVER_ERRNO_TO_ERROR[ERRNO_UNKNOWN_ERROR] = ERROR_UNKNOWN; +// FxAccounts has the ability to "split" the credentials between a plain-text +// JSON file in the profile dir and in the login manager. +// These constants relate to that. + +// The fields we save in the plaintext JSON. +// See bug 1013064 comments 23-25 for why the sessionToken is "safe" +this.FXA_PWDMGR_PLAINTEXT_FIELDS = ["email", "verified", "authAt", + "sessionToken", "uid"]; +// The pseudo-host we use in the login manager +this.FXA_PWDMGR_HOST = "chrome://FirefoxAccounts"; +// The realm we use in the login manager. +this.FXA_PWDMGR_REALM = "Firefox Accounts credentials"; + // Allow this file to be imported via Components.utils.import(). this.EXPORTED_SYMBOLS = Object.keys(this); diff --git a/services/fxaccounts/moz.build b/services/fxaccounts/moz.build index f95971425a67..1bb8c0916b22 100644 --- a/services/fxaccounts/moz.build +++ b/services/fxaccounts/moz.build @@ -10,11 +10,14 @@ TEST_DIRS += ['tests'] EXTRA_JS_MODULES += [ 'Credentials.jsm', - 'FxAccounts.jsm', 'FxAccountsClient.jsm', 'FxAccountsCommon.js' ] +EXTRA_PP_JS_MODULES += [ + 'FxAccounts.jsm', +] + # For now, we will only be using the FxA manager in B2G. if CONFIG['MOZ_B2G']: EXTRA_JS_MODULES += ['FxAccountsManager.jsm'] diff --git a/services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js b/services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js new file mode 100644 index 000000000000..297b25691708 --- /dev/null +++ b/services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js @@ -0,0 +1,196 @@ +/* Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/publicdomain/zero/1.0/ */ + +"use strict"; + +// Tests for FxAccounts, storage and the master password. + +// Stop us hitting the real auth server. +Services.prefs.setCharPref("identity.fxaccounts.auth.uri", "http://localhost"); + +Cu.import("resource://gre/modules/Services.jsm"); +Cu.import("resource://gre/modules/FxAccounts.jsm"); +Cu.import("resource://gre/modules/FxAccountsClient.jsm"); +Cu.import("resource://gre/modules/FxAccountsCommon.js"); +Cu.import("resource://gre/modules/osfile.jsm"); +Cu.import("resource://services-common/utils.js"); +Cu.import("resource://gre/modules/FxAccountsCommon.js"); + +initTestLogging("Trace"); +// See verbose logging from FxAccounts.jsm +Services.prefs.setCharPref("identity.fxaccounts.loglevel", "DEBUG"); + +function run_test() { + run_next_test(); +} + +function getLoginMgrData() { + let logins = Services.logins.findLogins({}, FXA_PWDMGR_HOST, null, FXA_PWDMGR_REALM); + if (logins.length == 0) { + return null; + } + Assert.equal(logins.length, 1, "only 1 login available"); + return logins[0]; +} + +add_task(function test_simple() { + let fxa = new FxAccounts({}); + + let creds = { + email: "test@example.com", + sessionToken: "sessionToken", + kA: "the kA value", + kB: "the kB value", + verified: true + }; + yield fxa.setSignedInUser(creds); + + // This should have stored stuff in both the .json file in the profile + // dir, and the login dir. + let path = OS.Path.join(OS.Constants.Path.profileDir, "signedInUser.json"); + let data = yield CommonUtils.readJSON(path); + + Assert.strictEqual(data.accountData.email, creds.email, "correct email in the clear text"); + Assert.strictEqual(data.accountData.sessionToken, creds.sessionToken, "correct sessionToken in the clear text"); + Assert.strictEqual(data.accountData.verified, creds.verified, "correct verified flag"); + + Assert.ok(!("kA" in data.accountData), "kA not stored in clear text"); + Assert.ok(!("kB" in data.accountData), "kB not stored in clear text"); + + let login = getLoginMgrData(); + Assert.strictEqual(login.username, creds.email, "email matches"); + let loginData = JSON.parse(login.password); + Assert.strictEqual(loginData.version, data.version, "same version flag in both places"); + Assert.strictEqual(loginData.accountData.kA, creds.kA, "correct kA in the login mgr"); + Assert.strictEqual(loginData.accountData.kB, creds.kB, "correct kB in the login mgr"); + + Assert.ok(!("email" in loginData), "email not stored in the login mgr json"); + Assert.ok(!("sessionToken" in loginData), "sessionToken not stored in the login mgr json"); + Assert.ok(!("verified" in loginData), "verified not stored in the login mgr json"); + + yield fxa.signOut(/* localOnly = */ true); + Assert.strictEqual(getLoginMgrData(), null, "login mgr data deleted on logout"); +}); + +add_task(function test_MPLocked() { + let fxa = new FxAccounts({}); + + let creds = { + email: "test@example.com", + sessionToken: "sessionToken", + kA: "the kA value", + kB: "the kB value", + verified: true + }; + + // tell the storage that the MP is locked. + fxa.internal.signedInUserStorage.__defineGetter__("_isLoggedIn", function() false); + yield fxa.setSignedInUser(creds); + + // This should have stored stuff in the .json, and the login manager stuff + // will not exist. + let path = OS.Path.join(OS.Constants.Path.profileDir, "signedInUser.json"); + let data = yield CommonUtils.readJSON(path); + + Assert.strictEqual(data.accountData.email, creds.email, "correct email in the clear text"); + Assert.strictEqual(data.accountData.sessionToken, creds.sessionToken, "correct sessionToken in the clear text"); + Assert.strictEqual(data.accountData.verified, creds.verified, "correct verified flag"); + + Assert.ok(!("kA" in data.accountData), "kA not stored in clear text"); + Assert.ok(!("kB" in data.accountData), "kB not stored in clear text"); + + Assert.strictEqual(getLoginMgrData(), null, "login mgr data doesn't exist"); + yield fxa.signOut(/* localOnly = */ true) +}); + +add_task(function test_consistentWithMPEdgeCases() { + let fxa = new FxAccounts({}); + + let creds1 = { + email: "test@example.com", + sessionToken: "sessionToken", + kA: "the kA value", + kB: "the kB value", + verified: true + }; + + let creds2 = { + email: "test2@example.com", + sessionToken: "sessionToken2", + kA: "the kA value2", + kB: "the kB value2", + verified: false, + }; + + // Log a user in while MP is unlocked. + yield fxa.setSignedInUser(creds1); + + // tell the storage that the MP is locked - this will prevent logout from + // being able to clear the data. + fxa.internal.signedInUserStorage.__defineGetter__("_isLoggedIn", function() false); + + // now set the second credentials. + yield fxa.setSignedInUser(creds2); + + // We should still have creds1 data in the login manager. + let login = getLoginMgrData(); + Assert.strictEqual(login.username, creds1.email); + // and that we do have the first kA in the login manager. + Assert.strictEqual(JSON.parse(login.password).accountData.kA, creds1.kA, + "stale data still in login mgr"); + + // Make a new FxA instance (otherwise the values in memory will be used.) + // Because we haven't overridden _isLoggedIn for this new instance it will + // treat the MP as unlocked. + let fxa = new FxAccounts({}); + + let accountData = yield fxa.getSignedInUser(); + Assert.strictEqual(accountData.email, creds2.email); + // we should have no kA at all. + Assert.strictEqual(accountData.kA, undefined, "stale kA wasn't used"); + yield fxa.signOut(/* localOnly = */ true) +}); + +add_task(function test_migration() { + // manually write out the full creds data to the JSON - this will look like + // old data that needs migration. + let creds = { + email: "test@example.com", + sessionToken: "sessionToken", + kA: "the kA value", + kB: "the kB value", + verified: true + }; + let toWrite = { + version: 1, + accountData: creds, + }; + + let path = OS.Path.join(OS.Constants.Path.profileDir, "signedInUser.json"); + let data = yield CommonUtils.writeJSON(toWrite, path); + + // Create an FxA object - and tell it to load the data. + let fxa = new FxAccounts({}); + data = yield fxa.getSignedInUser(); + + Assert.deepEqual(data, creds, "we should have everything available"); + + // now sniff the data on disk - it should have been magically migrated. + data = yield CommonUtils.readJSON(path); + + Assert.strictEqual(data.accountData.email, creds.email, "correct email in the clear text"); + Assert.strictEqual(data.accountData.sessionToken, creds.sessionToken, "correct sessionToken in the clear text"); + Assert.strictEqual(data.accountData.verified, creds.verified, "correct verified flag"); + + Assert.ok(!("kA" in data.accountData), "kA not stored in clear text"); + Assert.ok(!("kB" in data.accountData), "kB not stored in clear text"); + + // and it should magically be in the login manager. + let login = getLoginMgrData(); + Assert.strictEqual(login.username, creds.email); + // and that we do have the first kA in the login manager. + Assert.strictEqual(JSON.parse(login.password).accountData.kA, creds.kA, + "kA was migrated"); + + yield fxa.signOut(/* localOnly = */ true) +}); diff --git a/services/fxaccounts/tests/xpcshell/xpcshell.ini b/services/fxaccounts/tests/xpcshell/xpcshell.ini index 9d9bffe12544..4448d76a8f4a 100644 --- a/services/fxaccounts/tests/xpcshell/xpcshell.ini +++ b/services/fxaccounts/tests/xpcshell/xpcshell.ini @@ -5,6 +5,8 @@ tail = [test_accounts.js] [test_client.js] [test_credentials.js] +[test_loginmgr_storage.js] +skip-if = appname == 'b2g' # login manager storage only used on desktop. [test_manager.js] run-if = appname == 'b2g' reason = FxAccountsManager is only available for B2G for now diff --git a/services/sync/modules/util.js b/services/sync/modules/util.js index 194e0b285ca9..4691428613f3 100644 --- a/services/sync/modules/util.js +++ b/services/sync/modules/util.js @@ -19,6 +19,13 @@ Cu.import("resource://gre/modules/XPCOMUtils.jsm", this); Cu.import("resource://gre/modules/osfile.jsm", this); Cu.import("resource://gre/modules/Task.jsm", this); +// FxAccountsCommon.js doesn't use a "namespace", so create one here. +XPCOMUtils.defineLazyGetter(this, "FxAccountsCommon", function() { + let FxAccountsCommon = {}; + Cu.import("resource://gre/modules/FxAccountsCommon.js", FxAccountsCommon); + return FxAccountsCommon; +}); + /* * Utility functions */ @@ -594,8 +601,11 @@ this.Utils = { return this._syncCredentialsHosts; } let result = new Set(); - // the legacy sync host. + // the legacy sync host result.add(PWDMGR_HOST); + // the FxA host + result.add(FxAccountsCommon.FXA_PWDMGR_HOST); + // // The FxA hosts - these almost certainly all have the same hostname, but // better safe than sorry... for (let prefName of ["identity.fxaccounts.remote.force_auth.uri",