If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Please sanity-check dropping of syncKeyBundle on error in _fetchTokenForUser

VERIFIED DUPLICATE of bug 1056523

Status

Cloud Services
Firefox Sync: Backend
VERIFIED DUPLICATE of bug 1056523
3 years ago
3 years ago

People

(Reporter: rfkelly, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa+])

(Reporter)

Description

3 years ago
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 :-)
(Reporter)

Comment 1

3 years ago
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.
Flags: needinfo?(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.
Whiteboard: [qa+]
>     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.
Flags: needinfo?(ckarlof)
(Reporter)

Updated

3 years ago
Blocks: 1056523
(Reporter)

Updated

3 years ago
Flags: needinfo?(mhammond)
Yep, looks suspicious:

https://bugzilla.mozilla.org/show_bug.cgi?id=990834#c19
https://bugzilla.mozilla.org/show_bug.cgi?id=1056523#c1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1056523
Clearing needinfo - I'll address this in bug 1056523
Flags: needinfo?(mhammond)
OK.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.