1211606 - Only fetch keys when Sync is enabled via "services.sync.enabled" pref changed from Gaia

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Sync, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

We need this to allow the FxA client to fetch Sync keys.

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8669930 [details] [diff] [review]
v1

Actually, thinking about this twice we should probably not depend on this pref and simply always retrieve the sync keys along with the user account, just like Desktop and Android does.

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33f4a824e96e

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8671213 [details] [diff] [review]
v2

Change of plans again. Will keep this pref and will set it from the SyncStateMachine in Gaia. We will also need to implement a way to refetch Sync keys on demand.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v3

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] ferjm:bug1211606.sync.enabled.pref > mozilla-b2g:master

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8672936 [details] [diff] [review]
v3

We are finally setting this pref from Gaia and we will be refreshing the Sync keys if they are not available when this pref changes to enabled. That's going to happen on bug 1214105.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v4

Hopefully, the last change...

Michiel, could you take a look at the FxA code change, please? Basically, we always need to get the key fetch token when login into FxA [1] as there is no way to do that afterwards without logging out from FxA and asking the user to login back again when we want to actually get the keys (that's a pretty bad UX).

Fabrice, could you take a look at settings.js changes, please?

Thanks!

[1] https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#post-v1accountlogin

Comment 10

[Michiel de Jong [:michielbdejong]]

IIUC, FxOS devices fall into four categories wrt Sync:
1. those that do not support sync (because Gaia was built without the FIREFOX_SYNC flag).
2. those on which sync is supported, but was never enabled (because the user never went into Settings to do that).
3. those on which the user did go into Settings, but for whatever reason made the explicit choice to disable sync.
4. those on which sync is enabled.

Until now, we had a security barrier where only devices in group 4 will obtain kB.
This change removes that barrier in order to improve the UX at the moment of a user
entering into group 4. The barrier is not only removed for groups 2 and 3 (with which
I agree, the UX gain is worth it), but also for group 1. Is there a way to avoid that?

Since aEmail and aPassword are already coming from Gaia (right?), would it be
possible to make Gaia also pass the value of (effectively) the FIREFOX_SYNC build
flag as the third aKeyFetchToken argument?

Initially, group 1 will consist of all 2.5 devices except for the smart TV, and the
reason for them not to support sync is that we will have insufficient QA and
sec-review before 2.5RA. This group will disappear in 2.5+, but in the future
there might be other devices in group 1 (e.g. some IoT devices that don't have
the browser app installed?). So it might be worthwhile to make this option follow
the Gaia build flag instead of setting it to always true?

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Michiel de Jong [:michielbdejong] from comment #10)
> IIUC, FxOS devices fall into four categories wrt Sync:
> 1. those that do not support sync (because Gaia was built without the
> FIREFOX_SYNC flag).
> 2. those on which sync is supported, but was never enabled (because the user
> never went into Settings to do that).
> 3. those on which the user did go into Settings, but for whatever reason
> made the explicit choice to disable sync.
> 4. those on which sync is enabled.
> 
> Until now, we had a security barrier where only devices in group 4 will
> obtain kB.
> This change removes that barrier in order to improve the UX at the moment of
> a user
> entering into group 4. The barrier is not only removed for groups 2 and 3
> (with which
> I agree, the UX gain is worth it), but also for group 1. Is there a way to
> avoid that?
> 

We don't remove that barrier. We still don't get the Sync keys unless the user enabled Sync [1]. What we changed with this patch is that we always get the key fetch token required to obtain the Sync keys when the user logs in with her FxA credentials [2] as there is no way to obtain that afterwards.

[1] https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsManager.jsm#592
[2] https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsManager.jsm#130

> Since aEmail and aPassword are already coming from Gaia (right?), would it be
> possible to make Gaia also pass the value of (effectively) the FIREFOX_SYNC
> build
> flag as the third aKeyFetchToken argument?
> 

Yes, we can do that as well.

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v5

With this patch we don't get the fetch key token if FIREFOX_SYNC is not enabled on Gaia.

We also don't fetch Sync keys unless the 'services.sync.enabled' pref is true, which is set from Gaia based on user choice.

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8673005 [details] [review]
[gaia] ferjm:bug1211606.sync.enabled.pref > mozilla-b2g:master

Michiel, I had to update the PR to address your suggestion about not getting the key fetch token if FIREFOX_SYNC=0.

Comment 14

[Michiel de Jong [:michielbdejong]]

> > Since aEmail and aPassword are already coming from Gaia (right?), would it be
> > possible to make Gaia also pass the value of (effectively) the FIREFOX_SYNC
> > build
> > flag as the third aKeyFetchToken argument?
> 
> Yes, we can do that as well.

OK, I think I would prefer that, just so we only affect the behavior of devices that support sync.

(another remark, copied from our irc conversation): On [1] it says: 

> "During the /account/login?keys=true call, the server extracts a second value from its HKDF call named
> wrapwrapKey. It uses wrapwrapKey to unwrap wrap(wrap(kB)) from its database (with a simple XOR) to derive
> wrap(kB) and holds this temporarily in memory (not in the database)."
[...]
> "Note that keyFetchToken and wrap(kB) are not stored in the database. The goal is to make sure that
> nothing stored in the database enables an easy brute-force attack on the user's password: all saved
> values must include the long scrypt stretch in their derivation or protection."

We should also check what the impact on fxa-auth-server is if we leave the keyFetchToken unused for 
months or years instead of "temporarily". This seems to weaken the "temporarily, not in the database" argument used in [1].

If there no security advantage in storing keyFetchToken instead of kB, then we might as well retrieve kB immediately, that way at least it doesn't hang around on the fxa-auth-server for so long (which seems to be the way onepw protocol was intended to work).

[1] https://github.com/mozilla/fxa-auth-server/wiki/onepw-protocol#fetching-sync-keys

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Ryan, Chris, could you give us your feedback about comment 14, please? Thanks!

Comment 16

[Ryan Kelly [:rfkelly]]

Thanks for flagging this.  keyFetchTokens are designed to be short-lived so you're better off fetching kB than trying to keep a long-lived keyFetchToken just in case you want to fetch kB one day.  I don't think we guarantee a lifetime for keyFetchTokens, so future versions of the server might purge outstanding tokens after a day or two.

> It uses wrapwrapKey to unwrap wrap(wrap(kB)) from its database (with a simple XOR) to derive
> wrap(kB) and holds this temporarily in memory (not in the database).

This may be a little out of date or misleading; keyFetchToken details are in fact stored in the database, although we don't store a plaintext wrap(kB) alongside them.  We store wrap(kB) encrypted with a secret derived from the keyFetchToken, so that only the client can recover it.

I don't believe the contents of the DB constitute an easier brute-force attack on the password though.

> If there no security advantage in storing keyFetchToken instead of kB

It doesn't change the security story on the server-side, but I wouldn't count on the keyFetchTokens sticking around for months or years.  I don't see a huge security advantage on the client-side, since you must be storing the (keyFetchToken, wrap(kB)) pair necessary in order to get to kB.  So I'd suggest keeping it simple and just fetching kB up-front.

Comment 17

[Ryan Kelly [:rfkelly]]

> This may be a little out of date or misleading;

FWIW I tweaked that page slightly to try to make it clearer what the flow is.  "holds it temporarily in memory" means "performs some operations on it and then throws it away" rather than e.g. stashing it into some memory db for use by future requests.

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Thank you Ryan! I'll just fetch kB up-front then.

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v6

Fabrice, for the B2G part, on this patch I am adding the 'services.sync.enabled' setting -> pref binding. I also added the ability to subscribe to preference changes notifications to avoid race conditions on the Gaia side (this is used at [1]) and also fixed the fact that we were creating a function within a for loop.

Michiel, this patch is mostly the same as v5, but now we are always fetch the Sync key once we get the key fetch token. We are still not fetching anything Sync related if the 'services.sync.enabled' pref is disabled.

[1] https://github.com/mozilla-b2g/gaia/pull/32440/files#diff-42fffece5a65f7b105bc158d0d60701fR278

Comment 20

[Michiel de Jong [:michielbdejong]]

Comment on attachment 8673682 [details] [diff] [review]
v6

In `@@ -159,16 +151,25 @@` it seems you are discarding the result of the call to `this._fxAccounts.getKeys();`, and not waiting for it to finish. Maybe add a code comment there to say what will happen if that call fails, or if an attempt is made to use the keys before they have been fetched.

Maybe also add test coverage for this line in the unit test.

Comment 21

[[:fabrice] Fabrice Desré]

Comment on attachment 8673682 [details] [diff] [review]
v6

Review of attachment 8673682 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment addressed

::: b2g/chrome/content/settings.js
@@ +37,5 @@
>                                     "@mozilla.org/pac-generator;1",
>                                     "nsIPACGenerator");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "SystemAppProxy",
> +                                  "resource://gre/modules/SystemAppProxy.jsm");

That means you should remove https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#39

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6c9fca5c2cc

Comment 23

[[:fabrice] Fabrice Desré]

Comment on attachment 8673682 [details] [diff] [review]
v6

Review of attachment 8673682 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment addressed

::: b2g/chrome/content/settings.js
@@ +37,5 @@
>                                     "@mozilla.org/pac-generator;1",
>                                     "nsIPACGenerator");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "SystemAppProxy",
> +                                  "resource://gre/modules/SystemAppProxy.jsm");

That means you should remove https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#39

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39694c3bc236bc806c08ee34bf08633c75a788ef
Bug 1211606 - Only fetch keys when Sync is enabled via 'services.sync.enabled' pref changed from Gaia. r=fabrice

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

https://github.com/mozilla-b2g/gaia/commit/8999f0ba6326d815c8366e3c1155b7e4e9763b40

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/39694c3bc236