970167 - disable password sync when master password is enabled

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Richard Newman [:rnewman]]

In the course of reviewing Bug 967047, I noticed that FxA credentials weren't stored in Password Manager.

"Huh", I thought. "So where are they?"

The answer is "in a JSON file named signedInUser.json, in the profile directory, complete with kB and every other value associated with your account".

This seems less than ideal. There are very real problems with storing credentials in Password Manager, but…


18:38:53 < jbonacci> O_O
18:39:09 < jbonacci> that feels like a bug
18:41:37 < rfkelly> wat?
18:42:17 < rfkelly> rnewman that smells bad; please file something about it
18:50:38 < jbonacci> rfkelly crap
18:50:40 < jbonacci> he is right
18:50:44 < jbonacci> I am looking right at it


Marking this as a blocker; please triage and correct if the three of us are missing some context.

Note that this doesn't affect the separate implementation on Firefox on Android, where the OS provides isolation and some security for credentials.

Comment 1

[Richard Newman [:rnewman]]

ckarlof mentioned yesterday on IRC:

20:15:35 <@ckarlof> sec review hasn't completed
20:16:11 <@ckarlof> warner's opinion and mine is that if you aren't going to do something that's secure (Which is nearly impossible in this case), don't do anything at all
20:16:50 <@ckarlof> otherwise, people will think you intended for what you are doing to be "Secure" and say they broke it
20:17:59 <@ckarlof> something to revisit, though

so presumably a decision on this is blocked by sec review. Out of the 135 dependencies of Bug 905997 I can't find the one with sec-review-needed; anyone else know where that work is being tracked?

Comment 2

[Chris Karlof [:ckarlof]]

On Mac OS, we could maybe put it in the OS keychain.

Comment 3

[Ryan Kelly [:rfkelly]]

I have a "master password" set on my firefox profile, and while I understand that's far from perfect, I'd feel better knowing that my FxA info is protected in the same way my saved passwords are protected.

Comment 4

[Chris Karlof [:ckarlof]]

(In reply to Ryan Kelly [:rfkelly] from comment #3)
> I have a "master password" set on my firefox profile, and while I understand
> that's far from perfect, I'd feel better knowing that my FxA info is
> protected in the same way my saved passwords are protected.

The most straightforward way I know of doing that is to put it in the password manager, which has all sorts of terrible consequences, including 

1) not having access to your logged in state until you unlock you password manager, which is non-obvious
2) clearing your password will clear your logged in state, which is surprising

Proper user switchable profiles would lay the groundwork for a proper solution. An alternative would be to allow the option of "force auth" on every browser start, and don't store any keys/creds things persistently in the profile. Lots of UX landmines here, though. It sounds maybe doable for Fx30-31, but not for Fx29.

Comment 5

[Richard Newman [:rnewman]]

Yeah, Sync encountered a lot of pain with MP -- we have to surface a MP dialog a few seconds after startup, or before a sync, and deal with that user nagging.

You'd have to split credentials/state between prefs and pwmgr so that you could display some UI even in MP was locked.

And yes, we get a lot of users having to set up Sync again every time they quit the browser, because they had passwords set to clear on quit (rather than just not saving passwords).

That said, if you tell Firefox that you want to protect your passwords, it seems wrong for it to not do so -- indeed, to not do so for a single credential that gives 100% access to all of the passwords you're otherwise protecting!

The 'big picture' solution here is that *Firefox* has some secured login system, outside of the profile, and you can log in to the browser -- no credentials stored in the profile at all. But that's a sizable kettle of fish.

Platform-specific solutions might be worth exploring.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

How about we just disable Sync when a master password is set?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Hard to do without strings, I suppose. Seems unlikely that we can find a great solution here in the 29 time frame.

Comment 8

[Richard Newman [:rnewman]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #6)
> How about we just disable Sync when a master password is set?

I would support that on mobile, particularly if we offer pointers to users for how to secure their data using Android's built-in mechanisms.

I don't have a clear feeling either way for desktop.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

An alternative would be to force-disable the password engine (silently :/) when a master password is present.

Comment 10

[Andrew McCreight [:mccr8]]

Is there somebody who can work on this?

Comment 11

[Richard Newman [:rnewman]]

Not really my call. Certainly not me.

Comment 12

[Chris Karlof [:ckarlof]]

While this is less than ideal, IMO, it's not a 29 blocker. 

What is the threat model you had in mind protecting against?

Comment 13

[:Gavin Sharp [email: [e-mail]]]

The most compelling threat scenario is "Firefox profile theft", where a user is expecting the master password to protect at least their passwords. Probably a relatively uncommon situation, in that attackers with local file access will often have ways to also capture or bypass the master password, e.g. by installing a keylogger or malicious add-on that steals in-memory decrypted passwords. But there have been some exploits that involve local file stealing without those extra capabilities (e.g. bug 163598). Then again our master password currently isn't very robustly protected against brute forcing (bug 973759), so maybe that expectation is already broken :/

To mitigate in the short term, I think comment 9+SUMO article is best.

http://hg.mozilla.org/mozilla-central/annotate/8095b7dd8f58/browser/components/preferences/security.js#l134 is a way to determine whether a master password is set. We can disable the password engine (and associated UI) when that returns false in 29 easily enough.

Comment 14

[Chris Karlof [:ckarlof]]

I think that comment 9 is a fine idea to make sure that Fx Sync doesn't die when MP is enabled, but it doesn't address the original issue: where to store the FxA credentials. 

If we store them in the Password manager and the user hasn't entered her MP, then we can't start Sync. We take on a lot of complexity if we store the FxA creds in the password manager, and I would argue it's not worth it. It provides nearly zero value if no MP is in use, and even if one is, it's only protected by a single round of PBKDF (bug 973759). You need a pretty strong MP to mitigate offline attacks against 1 round of PBKDF, and I suspect most users' MPs don't pass muster.

As has been suggested in many of the linked threads, we need proper "log in to profile" to have a hope of doing this correctly.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to Chris Karlof [:ckarlof] from comment #14)
> I think that comment 9 is a fine idea to make sure that Fx Sync doesn't die
> when MP is enabled, but it doesn't address the original issue: where to
> store the FxA credentials. 

What do you mean "make sure that Fx Sync doesn't die"? I was proposing it as a mitigation to the security concern - if someone steals the FxA credentials for Sync users with master passwords set, those credentials won't get them the user's passwords (since we will not have synced them). Short term mitigation while we devise a solution to the broader "better handle FxA credentials" problem, which I agree can't be solved by just storing FxA credentials in the password manager.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

This really isn't "sec-high" - it doesn't lead to any data theft without some other local-file-reading exploit. sec-moderate seems appropriate.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Tim, can you implement the solution in comment 13/comment 9 (or find someone else to!).

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-970167-Disable-passwords-engine-when-a-master-pw.patch

This doesn't add a "help icon" to the disabled checkbox because I don't think we can offer this from the modal dialog that is shown when the user chooses to customize their sync options.

Comment 19

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

Comment on attachment 8389516 [details] [diff] [review]
0001-Bug-970167-Disable-passwords-engine-when-a-master-pw.patch

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

We also agreed there would be something that allows the user to work out why it's disabled - this patch doesn't have that.

::: services/sync/Weave.js
@@ +127,5 @@
> +    if (!this.fxAccountsEnabled) {
> +      return true;
> +    }
> +
> +    let secmodDB = Cc["@mozilla.org/security/pkcs11moduledb;1"]

Weave.js already imports services-sync/util.js, which has an mpLocked() function, so this should be used.

::: services/sync/modules/engines.js
@@ +559,5 @@
>    eEngineAbortApplyIncoming: "error.engine.abort.applyincoming",
>  
>    get prefName() this.name,
>    get enabled() {
> +    let service = Cc["@mozilla.org/weave/service;1"]

I think this should go into engines/passwords.js if possible and the getter doesn't complicate things.

Comment 20

[David Chan [:dchan]]

Am I understanding correctly that this patch would prevent the new sync engine from fetching / uploading passwords if a master password is used? The goal would be to limit the impact of exposing kB since it won't have the current client's MP protected passwords. There is still the threat of kB being used to decrypt passwords uploaded by other clients.

I agree with Gavin's assessment that the primary threat is profile theft either through local attacks or device/disk theft. I don't believe there is a quick way to solve the credential storage issue mentioned above. AFAIK, one of the FxA requirements is that a user logs in with their username + password, then their data is synced in the background with minimal interaction.  The FxA clients could use Windows DPAPI or OSX Keychain to store the key material. This would allow secure storage protected by the user's machine account password. I'm not aware of a crossdistribution Linux equivalent.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(In reply to David Chan [:dchan] from comment #20)
> Am I understanding correctly that this patch would prevent the new sync
> engine from fetching / uploading passwords if a master password is used? The
> goal would be to limit the impact of exposing kB since it won't have the
> current client's MP protected passwords. There is still the threat of kB
> being used to decrypt passwords uploaded by other clients.

We disable the local password engine, but we also set the pref disabling the sync password engine, which is propagated to other clients, and should prevent them from syncing passwords as well. Two issues with that currently:

- the current patch only sets the pref from within the prefs pane initialization code. It needs to do it as soon as Sync is set up instead.

- to handle the case where you then later go and re-enable passwords syncing on your android device, the desktop password engine's "enabled" setter needs to ignore attempts to be set to true (by the meta/global update), and additionally needs to then schedule a meta/global update to "force undo" the change in the android client

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-970167-Disable-passwords-engine-when-a-master-pw.patch, v2

I added a link [1] behind the disabled passwords engine's checkbox that can point to the SUMO article. Now all we need is a real URL.

We don't need to touch the engine.enabled pref as the pw engine itself and changes to the meta global will be handled accordingly.

[1] http://cl.ly/image/093u2x002E0d

Comment 23

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

Comment on attachment 8390704 [details] [diff] [review]
0001-Bug-970167-Disable-passwords-engine-when-a-master-pw.patch, v2

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

::: browser/base/content/sync/utils.js
@@ +88,5 @@
>    },
>  
> +  openMPInfoPage: function (event) {
> +    event.stopPropagation();
> +    this._openLink(Weave.Svc.Prefs.get("fxa.mpInfoURL"));

It might be better to do something like:

Services.urlFormatter.formatURLPref("app.support.baseURL") + "sync-masterpassword";

which is how we open the sumo article with info about the old sync in aboutaccounts.js and the sync prefs pane.  I think that also requires a new bug to be opened to support the new link redirecting to the real page, but I can't find an example of an old bug that does that.

::: services/sync/services-sync.js
@@ +78,5 @@
>  pref("services.sync.tokenServerURI", "https://token.services.mozilla.com/1.0/sync/1.5");
>  
>  pref("services.sync.fxa.termsURL", "https://accounts.firefox.com/legal/terms");
>  pref("services.sync.fxa.privacyURL", "https://accounts.firefox.com/legal/privacy");
> +pref("services.sync.fxa.mpInfoURL", "http://mozilla.org/");

see previous comment - if we can use a sumo article we can avoid this pref.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

We have slug/id for our SUMO article redirection:

verdi> ttaubert: ok let's use sync-master-password

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-970167-Disable-passwords-engine-when-a-master-pw.patch, v3

Carrying over r=markh with comments addressed. Requesting review from r-newman for the sync parts.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8390901 [details] [diff] [review]
0001-Bug-970167-Disable-passwords-engine-when-a-master-pw.patch, v3

Needs rebasing after major changes to enginesync.js.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-970167-Disable-passwords-engine-when-a-master-pw.patch, v4

It probably didn't actually change that much. Back to r?(rnewman) and r=markh.

Comment 28

[Richard Newman [:rnewman]]

Comment on attachment 8390920 [details] [diff] [review]
0001-Bug-970167-Disable-passwords-engine-when-a-master-pw.patch, v4

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

Looks OK with only three concerns around correctly handling user intent and avoiding surprise/permitting recovery:

1. Need to make sure that this correctly updates meta/global when you turn on MP after sync is set up and initialized, and the reverse when you turn it off.
2. We shouldn't be setting declined here. We're doing the opposite: the user *wants* to sync passwords, but we're refusing because we can't secure your credentials.
3. I think we should tell the user at the point of enabling MP that it will/did disable password sync, and if they're already using Sync with a MP, we should somehow tell them (yellow bar?) that they need to turn off MP in order to keep syncing passwords.

::: browser/components/preferences/security.js
@@ +174,5 @@
>  
>      this._initMasterPasswordUI();
> +
> +    // We might want to hide sync's password engine.
> +    gSyncPane.updateWeavePrefs();

Did you verify that this is enough to disable password sync if it's already syncing?

::: services/sync/modules/stages/enginesync.js
@@ +274,5 @@
>          // We also here mark the engine as declined, because the pref
>          // was explicitly changed to false.
>          // This will be reflected in meta/global in the next stage.
>          this._log.trace("Engine " + engineName + " was disabled locally. Marking as declined.");
>          toDecline.add(engineName);

I suspect this line should not be invoked if we disabled syncing due to MP being enabled -- in fact, we're here because the user *explicitly* turned on password sync on a different device (the opposite of declining!) and this device is silently turning it off.

So I think this part of the clause needs to be iff the engine is both disabled and was requested to be disabled. We'll handle this "bouncing" case specially when we consume the declined set.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

(In reply to Richard Newman [:rnewman] from comment #28)
> 3. I think we should tell the user at the point of enabling MP that it
> will/did disable password sync, and if they're already using Sync with a MP,
> we should somehow tell them (yellow bar?) that they need to turn off MP in
> order to keep syncing passwords.

Can't do this in 29. The "question mark icon in prefs that links to SUMO" will need to do.

Followup (maybe several) needed to clean this up.

Comment 30

[Richard Newman [:rnewman]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #29)

> Can't do this in 29. The "question mark icon in prefs that links to SUMO"
> will need to do.

Yup, understood. Just wanted to have the concern written down!

Comment 31

[Richard Newman [:rnewman]]

Comment on attachment 8390920 [details] [diff] [review]
0001-Bug-970167-Disable-passwords-engine-when-a-master-pw.patch, v4

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

See Comment 28 for things to address.

Comment 32

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

A problem with this patch is that it checks if the master password is locked - I think we want to check if it is *enabled*, regardless of whether it is currently locked or not.

I'm also unsure how this should interact with the new "declined" concept.  If the user explicitly enables password sync on a different device, we don't want to "decline" it as mentioned above, and ISTM we also don't want to wipe the data from the server - the user may still find value in having the passwords from those other devices syncronized.

Would it be enough to leave all the prefs and server data alone, but just disable the password engine locally?

Comment 33

[Richard Newman [:rnewman]]

(In reply to Mark Hammond [:markh] from comment #32)

> Would it be enough to leave all the prefs and server data alone, but just
> disable the password engine locally?

Depends how hacky we want to be.

If the goal is "by slurping up your cleartext credentials, we can't get any of your passwords", then no: if you're concerned enough to have MP enabled anywhere, we need to stop you syncing passwords everywhere.

If the goal is "if you have MP enabled everywhere, we don't sync passwords", then yeah, that's fine.

If we don't mind the user's passwords being partitioned -- i.e., if you save it on a device with no MP, it gets synced; otherwise, it doesn't -- then that's fine too.


> I'm also unsure how this should interact with the new "declined" concept. 
> If the user explicitly enables password sync on a different device, we don't
> want to "decline" it as mentioned above,

Indeed. This certainly isn't declining.


> and ISTM we also don't want to wipe
> the data from the server - the user may still find value in having the
> passwords from those other devices syncronized.

That could lead to some inconsistency, though (turn on MP later => your passwords are still on the server).

We might be over-thinking this: it is, after all, a bit of a hack. Your form history, history entry, and other data is also valuable, and having MP enabled is not necessarily a good indicator of a user wanting their passwords to be protected more strongly from a Sync-based attack.

We know that we're not presenting very usable choices here. "MP or Sync?", "MP or password sync?" -- neither is a good tradeoff, even if both meet our own goals of not having an awful password theft event.

The effort we put into this workaround should be proportional to how long it'll take until we have a better solution.

Comment 34

[:Gavin Sharp [email: [e-mail]]]

(In reply to Richard Newman [:rnewman] from comment #33)
> We might be over-thinking this: it is, after all, a bit of a hack. Your form
> history, history entry, and other data is also valuable, and having MP
> enabled is not necessarily a good indicator of a user wanting their
> passwords to be protected more strongly from a Sync-based attack.

It's a good indicator that they want some protection from local-file-stealing password attacks. The Sync-based attack means we'll let them down.

I thought we'd settled on a clear path here. We want:
- if the user has a master password on any of their clients, disable password syncing globally
- don't mark the engine as "declined" in that case

Is the pushback a result of that being somehow too complicated at this stage? I thought Tim's patch got us almost all the way there...

Comment 35

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

Attachment: 0001-Bug-970167-Disable-passwords-engine-when-a-master-pa.patch

This new version has tests!  Other significant changes not mentioned below:

* The pref for the passwords engine now reflects the disabled state correctly - previously it was possible for the engine to report it is disabled while the pref still reflected it was enabled.
* The previous version only kicked in when the MP was locked - it now kicks in if the MP is enabled, regardless of the lock state (and full disclosure, I think it's my fault that Tim's previous patch did that!)

(In reply to Richard Newman [:rnewman] from comment #28)
> 
> 1. Need to make sure that this correctly updates meta/global when you turn
> on MP after sync is set up and initialized, and the reverse when you turn it
> off.

I believe the test checks this.

> 2. We shouldn't be setting declined here. We're doing the opposite: the user
> *wants* to sync passwords, but we're refusing because we can't secure your
> credentials.

Agreed - we now do not decline if we tried, but failed, to enable it locally.

> ::: browser/components/preferences/security.js
> @@ +174,5 @@
> >  
> >      this._initMasterPasswordUI();
> > +
> > +    // We might want to hide sync's password engine.
> > +    gSyncPane.updateWeavePrefs();
> 
> Did you verify that this is enough to disable password sync if it's already
> syncing?

I'm not sure what you mean by this - all updateWeavePrefs() does is disable the checkbox.  As soon as MP is enabled the engine will report it is disabled, regardless of whether we are in a sync or not.  This means the server will not be wiped until the following sync, but that seems OK.

> ::: services/sync/modules/stages/enginesync.js
> @@ +274,5 @@
> >          // We also here mark the engine as declined, because the pref
> >          // was explicitly changed to false.
> >          // This will be reflected in meta/global in the next stage.
> >          this._log.trace("Engine " + engineName + " was disabled locally. Marking as declined.");
> >          toDecline.add(engineName);
> 
> I suspect this line should not be invoked if we disabled syncing due to MP
> being enabled -- in fact, we're here because the user *explicitly* turned on
> password sync on a different device (the opposite of declining!) and this
> device is silently turning it off.

As above, I think this should be correct now.

Comment 36

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

mid-air!

(In reply to :Gavin Sharp (email [e-mail]) from comment #34)
> Is the pushback a result of that being somehow too complicated at this
> stage? I thought Tim's patch got us almost all the way there...

I think it is doable.

Comment 37

[Richard Newman [:rnewman]]

Comment on attachment 8394521 [details] [diff] [review]
0001-Bug-970167-Disable-passwords-engine-when-a-master-pa.patch

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

::: services/sync/modules/engines/passwords.js
@@ +54,5 @@
> +    return newVal;
> +  },
> +
> +  set enabled(val) {
> +    val = this.isAllowed && val;

Just inline `val`.

::: services/sync/tests/unit/test_password_mpenabled.js
@@ +84,5 @@
> +    // stored.
> +    let meta = {
> +      payload: {
> +        engines: {
> +          "passwords": 1,

This value should actually be an object containing version and sync ID:


    "passwords": {"version":1,"syncID":"yfBi2v7PpFO2"},

Comment 38

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

thanks!

https://hg.mozilla.org/integration/fx-team/rev/3d4165fcf077

Comment 39

[Carsten Book [:Tomcat]]

landed on central https://hg.mozilla.org/mozilla-central/rev/3d4165fcf077

Comment 40

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8394521 [details] [diff] [review]
0001-Bug-970167-Disable-passwords-engine-when-a-master-pa.patch

>diff --git a/browser/components/preferences/sync.xul b/browser/components/preferences/sync.xul

>+                  <vbox id="fxa-pweng-help">

Should probably make this hidden="true" by default?

>+                    <hbox id="fxa-pweng-help-link">
>+                      <label value=" ["/>
>+                      <label class="text-link" value="?"
>+                             onclick="gSyncUtils.openMPInfoPage(event);"/>
>+                      <label value="]"/>
>+                    </hbox>

I think this would be better off as an image, I filed bug 986636.

I also filed bug 986637 to solve the underlying issue.

Comment 41

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

Comment on attachment 8394521 [details] [diff] [review]
0001-Bug-970167-Disable-passwords-engine-when-a-master-pa.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FxA Sync
User impact if declined: Users with a master password will be exposed to any local read vulnerabilities that might exist in Firefox, allowing an attacker to gain access to their passwords.  (Note that users without a master-password are also vulnerable, but they would already be vulnerable, the attacker could gain access to their passwords without going via the FxA credentials)

The solution here is that we simply don't sync passwords for users with a MP.

Testing completed (on m-c, etc.): Landed on m-c
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1b0d28323d93
https://hg.mozilla.org/releases/mozilla-beta/rev/6ab0871e26bc

Comment 43

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Testing seems to be covered automatically... is additional Manual testing also required for this?

Comment 44

[:Gavin Sharp [email: [e-mail]]]

Yes, manual testing would be appreciated, particularly for the multiple-client scenario (e.g. ensuring that enabling master password on a desktop device disables password sync on the linked Android device).

Comment 45

[Andrei Vaida [:avaida]]

Platforms covered: Windows 7 64-bit, Mac OS X 10.9, Ubuntu 13.10 64-bit.
Versions covered: latest Beta (Build ID: 20140407135746), Aurora 30.0a2 2014-04-08 (Build ID: 20140408004001), Nightly 31.0a1 2014-04-08 (Build ID: 20140408030205).

I was able to confirm the fix for this issue using different test scenarios involving different machines, profiles and a Nexus 4 (Android version 4.4.2). Here are a few notes on the current sync behavior that involves this matter:

1. Once the user signs up for a sync account with a Firefox profile that uses a master password, the pref "services.sync.engine.passwords" is set automatically to false. The same behavior is encountered when the user signs in with an existing sync account from a profile that uses a master password. Also:
- 1.1. Options > Sync menu displays "Passwords" engine disabled.
- 1.2. Options > Sync menu displays "[ ? ]" next to "Passwords" engine, which redirects the user to the correct SUMO article.
- 1.3. I assume that "[ ? ]" will turn into an actual image when Bug 986636 (Comment 40) will be fixed.
2. If the user disconnects from the Sync account while using a Firefox profile with a master password, the pref "services.sync.engine.passwords" is automatically set back to true. Also:
- 2.1. The same behavior is encountered when using a profile that does not have a master password in place.
3. Each time a sync is performed both ways, the device or profile that does not have a master password in place will have the engine "Passwords" disabled, while still being able to enable it back from Options > Sync. Also:
- 3.1. Enabling the "Passwords" engine on device/profile B and initiating a manual sync, will result in the automatic addition of the pref "services.sync.declinedEngines" having an empty (blank) string value.
- 3.2. The pref "services.sync.engine.prefs.modified" is set to false.
- 3.3. The pref "services.sync.engine.passwords" from device/profile A remains false.
- 3.4. Once another both ways sync is performed, device/profile B will have the "Passwords" engine set again to false.

Please let me know if you have any questions or concerns on the above.

Comment 46

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

(In reply to Andrei Vaida, QA [:avaida] from comment #45)
> Platforms covered: Windows 7 64-bit, Mac OS X 10.9, Ubuntu 13.10 64-bit.
> Versions covered: latest Beta (Build ID: 20140407135746), Aurora 30.0a2
> 2014-04-08 (Build ID: 20140408004001), Nightly 31.0a1 2014-04-08 (Build ID:
> 20140408030205).

Great, thank you.

Comment 47

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

Can we remove the "Security-Sensitive Core Bug" flag?

Comment 48

[:Gavin Sharp [email: [e-mail]]]

I hesitate to do so only because of the open discussion of the suckiness of our credential storage (which we haven't fixed).

Comment 49

[:Gavin Sharp [email: [e-mail]]]

I suppose there's probably not much here that hasn't already been mentioned by rnewman in bug 995268, but opening a previously-hidden bug is much more visible to the bad guys looking for juicy details to exploit.

Comment 50

[Daniel Veditz [:dveditz]]

We should err on the side of "open", especially since there's nothing security-related here not said elsewhere. The technical details might be educational in the future.