Closed Bug 1044536 Opened 10 years ago Closed 10 years ago

Please sanity-check dropping of syncKeyBundle on error in _fetchTokenForUser


(Firefox :: Sync, defect)

Not set





(Reporter: rfkelly, Unassigned)



(Whiteboard: [qa+])

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 :-)
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)
Blocks: 1056523
Flags: needinfo?(mhammond)
Closed: 10 years ago
Resolution: --- → DUPLICATE
Clearing needinfo - I'll address this in bug 1056523
Flags: needinfo?(mhammond)
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.