1430681 - Firefox stores and never consumes a keyFetchToken on FxA password change.

Status: NEW

(Firefox :: Firefox Accounts, defect)

Description

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

STR:
* Sign in to Sync and have it complete once.
* Change the account password on that device.
* Look at the FxA credentials stored in the password manager.

Actual:
* It has kA, kB, keyFetchToken and unwrapBKey

Expected:
* It should have *either* kA/kB *or* keyFetchToken/unwrapBKey, but not both.

This is particularly a problem given bug 1426304 (after which, it will be derived keys instead of kB) - it will be possible to re-fetch kB even after we've derived additional keys and discarded it)

The underlying issue is that the webchannel message we get from FxA telling us about the password change gives us the keyFetchToken and unwrapBKey - but as we already have kA and kB, we never consume and discard them.

There are 3 possible solutions:

* FxA should not give us this data. However, this would mean that a device which chooses to discard the keys as soon as it sees an auth error would then be unable to re-fetch them when the password changes.

* Desktop should re-fetch and re-derive keys in that case, even though the new keys will be expected to be identical to the ones we already hold. This will have the benefit of ensuring the keyFetchToken is consumed.

* Desktop should just discard keyFetchToken and unwrapBKey - this will mean the keyFetchToken is never consumed, which might not be ideal for the FxA server.

Bunch of needinfo's:
* Ryan, can you please check if there is anything above that warrants an FxA issue? Assuming we decide the FxA server shouldn't change, which of the 2 desktop focused solutions seems best from your POV?

* Grisha and James: can you please try and determine if Android/iOS might have the same issue, and if so, open the appropriate bugs?

Comment 1

[Ryan Kelly [:rfkelly]]

Our docs say that the fxaccounts:change_password includes the same data as the login message, because "A change password causes a user's unwrapBKey to change":

  https://github.com/mozilla/fxa-content-server/blob/master/docs/relier-communication-protocols/fx-webchannel.md#fxaccountchange_password

The browser shouldn't need to know this, because it shouldn't need to persist unwrapBKey long-term.  The one case I can think of where this might be useful would be something like

* User signs in to Firefox, but doesn't confirm the login, so the browser can't fetch keys yet
* User changes their password, causing their unwrapBKey and wrapKb to change
* User confirms the original login, Firefox uses its keyFetchToken to fetch wrapKb, but derives the wrong key because it's using an older value of unwrapBKey

But even then, as we learned in Bug 1419552, a password change will currently disconnect all signed-in devices anyway, making the above impossible.

So it seems that we do send these fields on purpose, but it's not clear to me that we should.  I actually wonder whether it was added simply to allow the client to use the same logic for login as for password change.  I filed an FxA bug to see if we can stop doing this:

  https://github.com/mozilla/fxa-content-server/issues/5832


:markh, assuming we *do* stop sending these fields, will older desktop browser (and android, and ios) cope with it correctly.  IIUC the problem is that they're not currently consuming these fields at all, so hopefully the fields can just go away without any bustage.

> * Desktop should re-fetch and re-derive keys in that case, even though the new keys will
> be expected to be identical to the ones we already hold. This will have the benefit of
> ensuring the keyFetchToken is consumed.

I think I prefer this option, it seems to have the least ways things could go wrong, and the load on the server should be minimal given that password change is not a frequent operation.  How terrible would it be to do this anyway, regardless of it and when we change the behaviour of FxA here?

Comment 2

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

(In reply to Ryan Kelly [:rfkelly] from comment #1)
> * User signs in to Firefox, but doesn't confirm the login, so the browser
> can't fetch keys yet
> * User changes their password, causing their unwrapBKey and wrapKb to change
> * User confirms the original login, Firefox uses its keyFetchToken to fetch
> wrapKb, but derives the wrong key because it's using an older value of
> unwrapBKey

Ah yes, this seems a real possibility.

> But even then, as we learned in Bug 1419552, a password change will
> currently disconnect all signed-in devices anyway, making the above
> impossible.

Unless they change their password on the device they just created it on? (OTOH, I guess they can't change the password in this scenario?)

> :markh, assuming we *do* stop sending these fields, will older desktop
> browser (and android, and ios) cope with it correctly.  IIUC the problem is
> that they're not currently consuming these fields at all, so hopefully the
> fields can just go away without any bustage.

I can't speak for Android or iOS, but desktop has logic in a getKeys() method that says, basically:

* If I have kA/kB (soon, k*), return them immediately.
* Otherwise, if I have a keyFetchToken, fetch and unwrap, then discard keyFetchToken/unwrapBKey
* Otherwise we are screwed, so logout forcing a reauth.

So it's really an accident of implementation - the most common scenario here is that we do have k*, so don't consume keyFetchToken - but it's hard to rationalize that it's 100% impossible that we will ever get this message when we don't already have k*. Another perfectly valid implementation of getKeys() would be slightly inverted:

* If we have keyFetchToken, it means we need keys, so fetch them.
* If we have k*, return them.
* Otherwise we are screwed, so logout forcing a reauth.

That inverted implementation would avoid the bug, but it clearly means more work that's unnecessary if we already have the keys.

> > * Desktop should re-fetch and re-derive keys in that case, even though the new keys will
> > be expected to be identical to the ones we already hold. This will have the benefit of
> > ensuring the keyFetchToken is consumed.
> 
> I think I prefer this option, it seems to have the least ways things could
> go wrong, and the load on the server should be minimal given that password
> change is not a frequent operation.  How terrible would it be to do this
> anyway, regardless of it and when we change the behaviour of FxA here?

I think that's trivial and as I sketched above (and I'm fine with the extra work too). I'm *slightly* nervous about changing FxA, simply as I'm not immediately certain we are guaranteed to already have the keys in that case - but apart from your "unverified user changing their password" scenario above, I can't think of a case when that would happen in practice - and even if it did, it would just mean the user needs to reauthenticate (which IIUC, would already be true today if somehow the webchannel message was lost - we wouldn't have a new session token)

Comment 3

[Ryan Kelly [:rfkelly]]

> Unless they change their password on the device they just created it on?

IIRC password change deletes all session tokens, and then immediately generates a fresh sessionToken which we send out to the device on which you did the password change.  So it shouldn't be possible regardless of device.

> I'm not immediately certain we are guaranteed to already have the keys in that case

Yeah, me neither.  But I think if you are in a position to listen to change_password events, but somehow don't have keys, then the right thing to do is freak out and force a re-auth, not magically recover to a hopefully-good-now state by virtue of the password change.

Comment 4

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

(In reply to Ryan Kelly [:rfkelly] from comment #3)
> > Unless they change their password on the device they just created it on?
> 
> IIRC password change deletes all session tokens, and then immediately
> generates a fresh sessionToken which we send out to the device on which you
> did the password change.  So it shouldn't be possible regardless of device.

What I meant to ask was "unless they can't change their password before verifying[/they have keys]?"

If that is possible, and FxA stopped sending kft/ubk, then it would be in a state where it had no keys and no way of obtaining them. So would...

> the right thing to do is freak out and force a re-auth...

do exactly that - but we might as well avoid it :) 

So I believe we are agreeing that (a) as above, if we have both keys and a kft, discard keys and re-fetch and (b) this implies FxA must continue to send those fields, meaning that PR is WONTFIX/INVALID (however that translates to github speak ;)?

Comment 5

[Ryan Kelly [:rfkelly]]

> What I meant to ask was "unless they can't change their password before verifying[/they have keys]?"

Hmm, yes you may be right - changing the password involves FxA web content fetching the keys, which I don't think it can do until after the account is verified.

The other thing your comment made me realize: a password change also deletes any outstanding keyFetchTokens.  Not entirely sure how this affects the state-machine here...

> (a) as above, if we have both keys and a kft, discard keys and re-fetch and 
> (b) this implies FxA must continue to send those fields

I agree with (a). Does it imply (b) by virtue of "we're not certain the browser will always have keys in this scenario"?

Comment 6

[:Grisha Kruglov]

On password change, Android's state machine is switched back to the initial "engaged" state, discarding any persisted derived keys. As the state machine is pumped at the start of a subsequent sync, keyFetchToken is consumed to fetch wrapkB, and new key material is derived via unwrapkB.

AFAICT, STRs as described in Comment 1 aren't applicable to Android.

Comment 7

[Ryan Kelly [:rfkelly]]

Comment 6 suggests that we should WONTFIX [1] because Android depends on the message containing a keyFetchToken.

[1] https://github.com/mozilla/fxa-content-server/issues/5832

Comment 8

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

(In reply to Ryan Kelly [:rfkelly] from comment #5)
> > (a) as above, if we have both keys and a kft, discard keys and re-fetch and 
> > (b) this implies FxA must continue to send those fields
> 
> I agree with (a). Does it imply (b) by virtue of "we're not certain the
> browser will always have keys in this scenario"?

That's where I was coming from, yeah.

(In reply to Ryan Kelly [:rfkelly] from comment #7)
> Comment 6 suggests that we should WONTFIX [1] because Android depends on the
> message containing a keyFetchToken.
> 
> [1] https://github.com/mozilla/fxa-content-server/issues/5832

So yeah, for better or worse, I agree that settles it.