1044536 - Please sanity-check dropping of syncKeyBundle on error in _fetchTokenForUser

Status: VERIFIED DUPLICATE of bug 1056523

(Firefox :: Sync, defect)

Description

[Ryan Kelly [:rfkelly]]

Pursuant to the HMAC error in Bug 1042109 Comment 6, I'm trying to see if there's a way that the client might have uploaded a /crypto/keys bundle encrypted with the wrong syncKeyBundle.

One thing that stood out to me is services/sync/modules/browserid_identity.js line 531, where we have:

        """
        // Drop the sync key bundle, but still expect to have one.
        // This will arrange for us to be in the right 'currentAuthState'
        // such that UI will show the right error.
        this._shouldHaveSyncKeyBundle = true;
        Weave.Status.login = this._authFailureReason;
        Services.obs.notifyObservers(null, "weave:service:login:error", null);
        throw err;
        """

Despite the comment, it's not obvious whether this code actually does drop the syncKeyBundle or whether it might leave it set to some previous value.

If it were left to some previous value on error, it might not get updated to the correct key when a subsequent auth attempt succeeds, due to this conditional code earlier in the same function:

    """
    if (!this._syncKeyBundle) {
      // We are given kA/kB as hex.
      this._syncKeyBundle = deriveKeyBundle(Utils.hexToBytes(userData.kB));
    }
    """

Possibly a red herring, but if someone more familiar with this code could please sanity-check the key handling here, that would be awesome :-)

Comment 1

[Ryan Kelly [:rfkelly]]

Oh, looks like this was removed on purpose in Bug 982848, so I'm throwing it over to :ckarlof for consideration.  Any chance this might have contributed to key-handling funny business in Bug 1042109?

If the current behaviour is correct, we should update the comment to better reflect the code.

Comment 2

[Chris Karlof [:ckarlof]]

This would ideally get some markh head scratching, but while he's out, I'm a good a person as any to take a lot. bug 1042109 is weird. QA people have the capacity to get their accounts in states that normal users usually don't, but it is concerning.

Comment 3

[Chris Karlof [:ckarlof]]

>     if (!this._syncKeyBundle) {
>      // We are given kA/kB as hex.
>      this._syncKeyBundle = deriveKeyBundle(Utils.hexToBytes(userData.kB));
>     }

The guard around setting the syncKeyBundle there certainly is "interesting". Mark is coming back next week, and I'd would be good to get his thoughts.

Comment 4

[Chris Karlof [:ckarlof]]

Yep, looks suspicious:

https://bugzilla.mozilla.org/show_bug.cgi?id=990834#c19
https://bugzilla.mozilla.org/show_bug.cgi?id=1056523#c1

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Clearing needinfo - I'll address this in bug 1056523

Comment 7

[James Bonacci [:jbonacci]]

OK.