Closed Bug 1426306 Opened 6 years ago Closed 6 years ago

Firefox Desktop should store derived key material rather than the master key `kB`

Categories

(Firefox :: Firefox Accounts, enhancement, P1)

Product:
Firefox
Component:
Firefox Accounts
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: rfkelly, Assigned: eoger)

References

Details

Attachments

(1 file)

Bug 1426306 - Store only derived keys instead of storing kB/kA.
59 bytes, text/x-review-board-request
glasserc
: review+
rfkelly
: review+
markh
: review+
Details 
This is the Desktop part of Bug 1426304.  Since we will soon start deriving non-sync-related keys from kB, sync clients should avoid storing kB directly, and instead store the minimal set of derived keys necessary to talk to sync:

* 64 bytes for the sync key bundle:

  kSync = HKDF(kB, undefined, "identity.mozilla.com/picl/v1/oldsync", 64)

* 16 bytes for the tokenserver's X-Client-State header:

  kXCS = SHA256(kB)[:16]

* 64 bytes for the webext chrome.storage.sync master key (which in restrospect, I wish we had derived from a root "sync" key rather than directly from kB...):

  kExtSync = HKDF(kB, undefined, "identity.mozilla.com/picl/v1/chrome.storage.sync", 64)
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8940809 [details]
Bug 1426306 - Store only derived keys instead of storing kB/kA.

https://reviewboard.mozilla.org/r/211074/#review216856

::: toolkit/components/extensions/ExtensionStorageSync.jsm:296
(Diff revision 1)
>      try {
>        return await super.decode(record);
>      } catch (e) {
>        if (Utils.isHMACMismatch(e)) {
> -        const currentKBHash = await getKBHash(this._fxaService);
> -        if (record.kbHash != currentKBHash) {
> +        const currentKExtSyncHash = await getKExtSyncHash(this._fxaService);
> +        if (record.kExtSyncHash != currentKExtSyncHash) {

This basically requires a difficult server-side migration to do correctly. It's possible that we could handle this correctly on the client side (wiping the server and re-uploading, the way we do on a "normal" kB change), but I'm a little nervous because we don't have tests that cover that. Could you confirm (possibly by adding a test?) that this will work correctly? Alternatively, one thing we could consider is to rename all the variables and comments, but to keep the record field names the same. This means incurring a little bit of technical debt but avoiding a migration. What do you think?

Also, I don't see any changes to the storage.sync tests. I feel like they ought to need updates as well?
Comment on attachment 8940809 [details]
Bug 1426306 - Store only derived keys instead of storing kB/kA.

https://reviewboard.mozilla.org/r/211074/#review216858

::: services/fxaccounts/FxAccounts.jsm:530
(Diff revision 1)
>     *        {
>     *          email: The user's email address
>     *          uid: The user's unique id
>     *          sessionToken: Session for the FxA server
> -   *          kA: An encryption key from the FxA server
> -   *          kB: An encryption key derived from the user's FxA password
> +   *          kSync: An encryption key for Sync
> +   *          kXCS: An encryption key for the X-Client-State header

nit: kXCS is not an "encryption key", can we call it something like "key hash" or similar to avoid any future confusion?
> This basically requires a difficult server-side migration to do correctly.

From IRC, it sounds like the alternative here will be to store sha256(kB) as an additional derived secret, to avoid the pain of a migration.
Comment on attachment 8940809 [details]
Bug 1426306 - Store only derived keys instead of storing kB/kA.

https://reviewboard.mozilla.org/r/211074/#review217006

This looks good from my perspective, thanks :eoger!  I had one question about the code that was previously doing the "chrome.storage.sync" key derivation, but other than that :thumbsup:

::: services/fxaccounts/FxAccounts.jsm:1050
(Diff revision 2)
> +   * @returns HKDF(kB, undefined, "identity.mozilla.com/picl/v1/chrome.storage.sync", 64)
> +   */
> +  _deriveWebExtSyncStoreKey(kBbytes) {
> +    return CryptoUtils.hkdf(kBbytes, undefined,
> +                            "identity.mozilla.com/picl/v1/chrome.storage.sync",
> +                            2 * 32);

I was expecting to see a corresponding removal of this "chrome.storage.sync" derivation from whatever part of the code was doing it previously, but I don't see one.  Should there have been code that was previously doing this that was removed?

(It's entirely possible that I'm not seeing it due to my unfamiliarity with reviewboad)

::: services/sync/tests/unit/test_browserid_identity.js:151
(Diff revision 2)
> +    let fxa = globalBrowseridManager._fxaService;
> +    let result = await fxa.getSignedInUser();
> +    Assert.notEqual(null, result.kSync);
> +    Assert.notEqual(null, result.kXCS);
> +    Assert.notEqual(null, result.kExtSync);
> +    Assert.notEqual(null, result.kExtKbHash);

Is it worth calculating known test vectors here, so we can assert that the calculation was done successfully rather than just "not null"?  I'd hate to accidentally set `result.kSync` to "[object Object]" or some other JS nonsense...
Attachment #8940809 - Flags: review?(rfkelly) → review+
Comment on attachment 8940809 [details]
Bug 1426306 - Store only derived keys instead of storing kB/kA.

https://reviewboard.mozilla.org/r/211074/#review217242

My r+ isn't very powerful but this looks good from my perspective.
Attachment #8940809 - Flags: review?(eglassercamp) → review+
Code aside, I'm also interested in what we should do to test this in the wild, both for new signins and for migration of existing users.  :eoger, have you given thought to an explicit test plan?
The migration test is probably the most important one:

- Install Firefox Release 57 and run it with a profile P.
- Sign-in to Sync/Verify and wait for the first sync to complete.
- Quit Firefox Release 57.
- Run Firefox Nightly 59 or above with the same profile P.
- Trigger a manual Sync.
- Verify that the migration worked correctly:
  - No "warning state" for FxA in the hamburger menu.
  - In about:sync-logs no errors detected after the first sync.
  - In Preferences -> Saved Logins -> Show Passwords the chrome://FirefoxAccounts entry password should have the kSync, kXCS, kExtSync and kExtKbHash fields in the stringified JSON.
(In reply to Edouard Oger [:eoger] from comment #10)
> The migration test is probably the most important one:

I think we also need to test what happens when the user launches 57 with a profile that's already been migrated - we know that they will have a bad time, but it should be the best bad time possible :) (ie, it should cause a reauth). We should then re-run the profile with 59 and ensure the migration happens again and kA and kB are gone (they may not be re-nuked as the profile when running on 57 might have caused us to have both the new and old keys, causing us to skip re-migrating)
Comment on attachment 8940809 [details]
Bug 1426306 - Store only derived keys instead of storing kB/kA.

https://reviewboard.mozilla.org/r/211074/#review218430

Thanks for taking this on given how gnarly our existing code is. It is looking good, but see the comments below.

::: services/fxaccounts/FxAccounts.jsm:937
(Diff revision 3)
>      let currentState = this.currentAccountState;
>      return currentState.getUserAccountData().then((userData) => {
>        if (!userData) {
>          throw new Error("Can't get keys; User is not signed in");
>        }
> -      if (userData.kA && userData.kB) {
> +      if (userData.kSync && userData.kXCS && userData.kExtSync &&

is there a reason we can't do the migration here? That would also mean we don't need to make deriveKeys public.

::: services/fxaccounts/FxAccounts.jsm:1006
(Diff revision 3)
>        unwrapBKey: null,
> +      kA: null, // we stopped storing kA and kB in bug 1426306.
> +      kB: null
>      };
>  
> -    log.debug("Keys Obtained: kA=" + !!updateData.kA + ", kB=" + !!updateData.kB);
> +    const keyNames = ["kSync", "kXCS", "kExtSync", "kExtKbHash"];

These keys should probably be in a "global" constant and used above in all places where they are duplicated?

::: services/fxaccounts/FxAccountsCommon.js:221
(Diff revision 3)
>    ["email", "verified", "authAt", "sessionToken", "uid", "oauthTokens", "profile",
>    "deviceId", "deviceRegistrationVersion", "profileCache"]);
>  
>  // Fields we store in secure storage if it exists.
>  exports.FXA_PWDMGR_SECURE_FIELDS = new Set(
> -  ["kA", "kB", "keyFetchToken", "unwrapBKey", "assertion"]);
> +  ["kA", "kB", "kSync", "kXCS", "kExtSync", "kExtKbHash", "keyFetchToken", "unwrapBKey", "assertion"]);

I'm guessing kA and kB remain here or you have trouble killing them? I think we should work out another way to nuke them so they aren't listed here (eg, maybe change storage's updateAccountData so a null value checks and deletes from all locations? Or maybe a new FXA_PWDMGR_DEPRECATED_FIELDS set?)

::: services/sync/modules/browserid_identity.js:277
(Diff revision 3)
>          this._shouldHaveSyncKeyBundle = true;
>          this.whenReadyToAuthenticate.reject("no user is logged in");
>          return;
>        }
>  
> +      if (accountData.kB) { // Bug 1426306 - Migrate from kB to derived keys.

I think this migration code should be in FxAccounts.jsm (and I'm not sure this would work correctly when the master-password is locked anyway)

::: services/sync/modules/browserid_identity.js:298
(Diff revision 3)
>        this._log.info("Waiting for user to be verified.");
>        if (!accountData.verified) {
>          telemetryHelper.maybeRecordLoginState(telemetryHelper.STATES.NOTVERIFIED);
>        }
> -      this._fxaService.whenVerified(accountData).then(accountData => {
> +      try {
> +        accountData = await this._fxaService.whenVerified(accountData);

this is no longer in the background as the comment above mentions, and it must be - validation may take a very long time.

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js:521
(Diff revision 3)
>  // Tests using this ID will share keys in local storage, so be careful.
>  const defaultExtensionId = "{13bdde76-4dc7-11e6-9bdc-54ee758d6342}";
>  const defaultExtension = {id: defaultExtensionId};
>  
> -const BORING_KB = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef";
> +const kExtSync = "63f9057577c04bbbb9f0c3fd85b5d4032b60e13edc1f8dd309bf4305d66f2cc312dde16ce46021a496f713950d0a6c566ce181521a44726e7be97cf577b31b31";
> +const BORING_KBHASH = "2350cba8fced5a2fbae3b1f180baf860f78f6542bef7be709fda96cd3e3dc800";

nit: this constant name doesn't really make sense now, as we've lost any evidence it came from a "boring" kB. Does the value actually matter? If not, make it another boring string, otherwise consider changing the name. Same as "novel" values below.
Attachment #8940809 - Flags: review?(markh)
Thank you Mark, I've made the requested changes.
I've also tested your scenario, and thankfully "running on 57 might have caused us to have both the new and old keys" does not happen, so we re-migrate with any issue.
Comment on attachment 8940809 [details]
Bug 1426306 - Store only derived keys instead of storing kB/kA.

https://reviewboard.mozilla.org/r/211074/#review218688

This is looking great, thanks - I've a few other comments (many of which are cleanups rather than about your patch), so wouldn't mind another look (and note that we should only land this post-merge)

::: services/fxaccounts/FxAccounts.jsm:938
(Diff revision 4)
>        if (!userData) {
>          throw new Error("Can't get keys; User is not signed in");
>        }
> -      if (userData.kA && userData.kB) {
> +      let promise = Promise.resolve();
> +      if (userData.kB) { // Bug 1426306 - Migrate from kB to derived keys.
> +        const kBbytes = CommonUtils.hexToBytes(userData.kB);

let's add a log.debug() call indicating we are doing the migration.

::: services/fxaccounts/FxAccounts.jsm:940
(Diff revision 4)
>        }
> -      if (userData.kA && userData.kB) {
> +      let promise = Promise.resolve();
> +      if (userData.kB) { // Bug 1426306 - Migrate from kB to derived keys.
> +        const kBbytes = CommonUtils.hexToBytes(userData.kB);
> +        userData = {
> +          ...userData,

IIRC, it shouldn't be necessary to specify userData here - existing fields shouldn't be touched by updateUserAccountData.

::: services/fxaccounts/FxAccounts.jsm:945
(Diff revision 4)
> +          ...userData,
> +          ...this._deriveKeys(userData.uid, kBbytes),
> +          kA: null, // Remove kA and kB from storage.
> +          kB: null
> +        };
> +        promise.then(() => this.updateUserAccountData(userData));

I don't quite understand this - the updateUserAccountData promise isn't consumed and I don't see why the promise.then() is needed at all. ISTM it might be clearer to make this entire method async?

::: services/fxaccounts/FxAccounts.jsm:1013
(Diff revision 4)
>      let updateData = {
> -      kA: CommonUtils.bytesAsHex(kA),
> +      ...this._deriveKeys(data.uid, kBbytes),
> -      kB: CommonUtils.bytesAsHex(kB_hex),
>        keyFetchToken: null, // null values cause the item to be removed.
>        unwrapBKey: null,
> +      kA: null, // we stopped storing kA and kB in bug 1426306.

Do we actually need to do this here? Given how things are called ISTM that it should be impossible?

If we want to be careful, maybe we could throw if they exist, as that will point to something we've missed?

(OTOH, it's cheap, so I don't really mind nulling them here, but we should at least comment that it's impossible and log a warning when it's not)

::: services/fxaccounts/tests/xpcshell/test_storage_manager.js:171
(Diff revision 4)
>    await sm._promiseStorageComplete; // storage is written in the background.
>    // "verified", "deviceId" and "deviceRegistrationVersion" are plain-text fields.
>    Assert.equal(sm.plainStorage.data.accountData.verified, true);
>    Assert.equal(sm.plainStorage.data.accountData.deviceId, "wibble");
>    Assert.equal(sm.plainStorage.data.accountData.deviceRegistrationVersion, DEVICE_REGISTRATION_VERSION);
> -  // "kA" and "foo" are secure
> +  // "kXCS" and "foo" are secure

This comment seems like it was wrong before your change - there's no "foo" - I'm guessing copy-pasta. Maybe change the comment to, say, "derived keys are secure"?

::: services/sync/modules/browserid_identity.js:287
(Diff revision 4)
> +      CommonUtils.nextTick(async () => {
> -      this._log.info("Waiting for user to be verified.");
> +        this._log.info("Waiting for user to be verified.");
> -      if (!accountData.verified) {
> +        if (!accountData.verified) {
> -        telemetryHelper.maybeRecordLoginState(telemetryHelper.STATES.NOTVERIFIED);
> +          telemetryHelper.maybeRecordLoginState(telemetryHelper.STATES.NOTVERIFIED);
> -      }
> +        }
> -      this._fxaService.whenVerified(accountData).then(accountData => {
> +        try {

not directly related to your change, but let's move this try a few lines up, just in-case the telemetryHelper call fails.

::: services/sync/modules/browserid_identity.js:520
(Diff revision 4)
>    // to successfully fetch them?
>    _canFetchKeys() {
>      let userData = this._signedInUser;
>      // a keyFetchToken means we can almost certainly grab them.
> -    // kA and kB means we already have them.
> -    return userData && (userData.keyFetchToken || (userData.kA && userData.kB));
> +    // kSync, kXCS, kExtSync and kExtKbHash means we already have them.
> +    return userData && (userData.keyFetchToken ||

I should have mentioned this, but IMO we should now move `_canFetchKeys` (probably renamed to `canGetKeys()`) into FxA itself.

::: services/sync/modules/browserid_identity.js:607
(Diff revision 4)
>      let log = this._log;
>      let client = this._tokenServerClient;
>      let fxa = this._fxaService;
>      let userData = this._signedInUser;
>  
> -    // We need kA and kB for things to work.  If we don't have them, just
> +    // We need kSync, kXCS, kExtSync and kExtKbHash for things to work.  If we don't have them, just

I think this comment could just read "We need keys for things to work..." - ie, no need to list the keys here.

::: services/sync/modules/browserid_identity.js:618
(Diff revision 4)
>      }
>  
>      let maybeFetchKeys = () => {
>        // This is called at login time and every time we need a new token - in
> -      // the latter case we already have kA and kB, so optimise that case.
> -      if (userData.kA && userData.kB) {
> +      // the latter case we already have kSync, kXCS, kExtSync and kExtKbHash, so optimise that case.
> +      if (fxAccountsCommon.DERIVED_KEYS_NAMES.every(k => userData[k])) {

And I think we can probably drop this "optimization" - getKeys() already short-circuits if the keys exist, so I don't see any value in this optimization and it leaks stuff this module shouldn't really be concerned about.

So drop both the comment above and this block, and change the log message below to something like "getting keys"

(These changes would also make this code better suited for bug 1430681, where we will probably end up deciding that if we have a keyFetchToken we always consume it, even if we *do* already have keys)
Attachment #8940809 - Flags: review?(markh)
Depends on: 1430970
Comment on attachment 8940809 [details]
Bug 1426306 - Store only derived keys instead of storing kB/kA.

https://reviewboard.mozilla.org/r/211074/#review219590

Awesome, thanks!

::: services/fxaccounts/FxAccounts.jsm:950
(Diff revision 5)
> +      let userData = await currentState.getUserAccountData();
>        if (!userData) {
>          throw new Error("Can't get keys; User is not signed in");
>        }
> -      if (userData.kA && userData.kB) {
> -        return userData;
> +      if (userData.kB) { // Bug 1426306 - Migrate from kB to derived keys.
> +        log.warn("Migrating kB to derived keys.");

I think log.info is more appropriate.
Attachment #8940809 - Flags: review?(markh) → review+
Thank you Mark, let's land this for 60.
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d0befa362c81
Store only derived keys instead of storing kB/kA. r=glasserc,markh,rfkelly
https://hg.mozilla.org/mozilla-central/rev/d0befa362c81
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
See Also: → 1479749
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: