1280294 - (CVE-2016-5260) Session Manager can sometimes store Firefox Accounts Password in plain text

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Mike Kaply [:mkaply]]

We received a report from a partner that showed a sessionstore.js with their password in plain text.

On further investigation, we noticed that it was a case where the particular page (In this case, Firefox accounts) provided the ability to show the password and when this was done, because the field was now just a text field, the password was written to sessionstore.js.

We should probably be smarter in this case and somehow maintain that the field was a password field at some point in time so that we don't ever save the password for that field to disk.

Comment 1

[:Gijs (he/him)]

(In reply to Mike Kaply [:mkaply] from comment #0)
> We should probably be smarter in this case and somehow maintain that the
> field was a password field at some point in time so that we don't ever save
> the password for that field to disk.

Does the Firefox accounts website actually reuse the same element? I suspect plenty of websites who implement this kind of functionality use a separate textbox (or hidden form fields in which they copy that information - okta used to do this) in which case even that mitigation doesn't really work.

Comment 2

[Mike Kaply [:mkaply]]

> Does the Firefox accounts website actually reuse the same element?

Yep.

> I suspect plenty of websites who implement this kind of functionality use a separate textbox (or hidden form fields in which they copy that information - okta used to do this) in which case even that mitigation doesn't really work.

That seems like extra work because then you would have to have two different named password fields. I'm not sure how common that would be...

Comment 3

[:Gijs (he/him)]

(In reply to Mike Kaply [:mkaply] from comment #2)
> > Does the Firefox accounts website actually reuse the same element?
> 
> Yep.
> 
> > I suspect plenty of websites who implement this kind of functionality use a separate textbox (or hidden form fields in which they copy that information - okta used to do this) in which case even that mitigation doesn't really work.
> 
> That seems like extra work because then you would have to have two different
> named password fields. I'm not sure how common that would be...

So... I *think* there used to be browser engines that would clear the contents of the form field if you changed the type. I forget whether it was IE or chrome. Chrome definitely doesn't (anymore? - I checked). Dunno about IE/Edge. But that would be one reason to use two elements.

Comment 4

[Matthew N. [:MattN]]

IME changing the type of the input is more common than moving a value around.

I also don't think we need a perfect solution for this bug as that is really bug 502258 since sites wouldn't need to implement this themselves at that point.

@autocomplete=off should also still prevent storage in session history so FxA could toggle that when toggling .type or when the document becomes hidden (maybe with browser-specific rules).

type=password and autocomplete=off are both the ways to prevent storage in session history across browsers (IIUC) so is there any reason we can't treat this as an FxA web bug?

Comment 5

[Mike Kaply [:mkaply]]

>Is there any reason we can't treat this as an FxA web bug?

I don't think so (and that would probably make it easier to get done.)

I wasn't sure when I brought up the issue where it would end up.

Comment 6

[Matthew N. [:MattN]]

Chris, who could fix the FxA login page to set autocomplete=off when the type is set to text?

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #4)
> type=password and autocomplete=off are both the ways to prevent storage in
> session history across browsers (IIUC) so is there any reason we can't treat
> this as an FxA web bug?

I didn't know about autocomplete=off as a way to ignore this... Mike Connor and I have exchanged emails about this issue and I proposed new proprietary attributes to signal session store/ XPathGenerator.jsm to explicitly ignore a form field.
Matt, I'm curious, where is this implemented?

Comment 8

[Matthew N. [:MattN]]

I didn't test it but I believe session store uses the state key method which uses IsAutocompleteOff:
https://dxr.mozilla.org/mozilla-central/search?q=IsAutocompleteOff&redirect=false

Comment 9

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #8)
> I didn't test it but I believe session store uses the state key method which
> uses IsAutocompleteOff:
> https://dxr.mozilla.org/mozilla-central/
> search?q=IsAutocompleteOff&redirect=false

I don't think it does... the only callers are generatestatekey, from which I only see layout callpaths. sessionstore collects formdata using FormData.jsm, http://searchfox.org/mozilla-central/source/browser/components/sessionstore/content/content-sessionStore.js#436 and that uses an xpath expression that doesn't take autocomplete=off into account, AFAICT.

Comment 10

[Matthew N. [:MattN]]

Maybe it's only used for bfcache then[1]? It seems like session store should match bfcache for this (I thought it did at one point) and look for autocomplete=off.

Comment 11

[Mike Kaply [:mkaply]]

So at this point, do we believe there is anything that can be done server side to mitigate this?

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #10)
> Maybe it's only used for bfcache then[1]? It seems like session store should
> match bfcache for this (I thought it did at one point) and look for
> autocomplete=off.

I'd be happy to implement this, but first I'd like to learn what autocomplete=off means exactly?

(In reply to Mike Kaply [:mkaply] from comment #11)
> So at this point, do we believe there is anything that can be done server
> side to mitigate this?

Remove the 'Show password' option from the FxA page? I'll hopefully be able to implement using 'autocomplete=off' for Fx 50, but I don't know if that's quick enough.

Comment 13

[Mike Kaply [:mkaply]]

I think at this point we're going to recommend to the partner that they change the filter for browser.sessionstore.privacy_level to 2.

We'll lose the storing of all form elements, but it will solve the problem for now.

I don't think the FxA team would go for removing the show password.

Comment 14

[Chris Karlof [:ckarlof]]

I'm adding some relevant folks from the FxA team to help dig in. Ryan, Shane, can you please take a look?

Comment 15

[Ryan Kelly [:rfkelly]]

We have a similar-sounding issue on our radar already: https://github.com/mozilla/fxa-content-server/issues/3799

Leaving ni? myself to dig in deeper

Comment 16

[Ryan Kelly [:rfkelly]]

So yes, it sounds like this is the same issue as I linked above.  We'll be very happy to make the necessary changes on the FxA side for it to behave better here.

I believe the current fix we're looking at on the FxA side, is to just turn the field back into a type="password" field before actually submitting the form.  Should we also look at toggling autocomplete="off" as an additional measure?

Comment 17

[Ryan Kelly [:rfkelly]]

Adding Sai, who is working on the current fix on the FxA side.

It sounds like there's some time-sensitivity to this due to the involvement of a partner, :mkaply is there any more context you can share around that?

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: add autocomplete='off' to list of ignored inputs of the session store formdata collector

Matt, what do you think of this?

Comment 19

[Mike de Boer [:mikedeboer]]

(In reply to Ryan Kelly [:rfkelly] from comment #16)
> I believe the current fix we're looking at on the FxA side, is to just turn
> the field back into a type="password" field before actually submitting the
> form.  Should we also look at toggling autocomplete="off" as an additional
> measure?

I don't think that'd be a fix... the browsers' session store will still collect the form data while the password is showing in a regular input field. Turning the input back to type=password won't change that behavior.

Comment 20

[Shane Tomlinson [:stomlinson]]

Clearing the ni, we already know about this problem and are working on an FxA side fix too.

Comment 21

[Justin Dolske [:Dolske]]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #4)
> IME changing the type of the input is more common than moving a value around.
> 
> I also don't think we need a perfect solution for this bug as that is really
> bug 502258 since sites wouldn't need to implement this themselves at that
> point.

Seems like a more robust way to fix this (without requiring sites to change behavior, which is a losing game), would be to have a way to mark inputs as tainted if they've ever been type=password.

EG:

0) start with <input type=text>
1) input.mozWasOnceAPassword === false
2) input.type = "password"
3) input.mozWasOnceAPassword === true
4) input.type = "color"
5) input.mozWasOnceAPassword === true

Obviously this would require a DOM change, but I should think it would be trivial. Then session store (and form manager?) should decline to save input that have been so tainted.

Comment 22

[Ryan Kelly [:rfkelly]]

> the browsers' session store will still collect the form data while the password is showing
> in a regular input field. Turning the input back to type=password won't change that behavior.

Sai, Shane, I want to highlight this explicitly ^.  IIUC the problem doesn't manifest only on form submit, but can occur at any stage due to saving data for session restore.

Comment 23

[Shane Tomlinson [:stomlinson]]

(In reply to Ryan Kelly [:rfkelly] from comment #22)
> Sai, Shane, I want to highlight this explicitly ^.  IIUC the problem doesn't
> manifest only on form submit, but can occur at any stage due to saving data
> for session restore.

In light of this, it seems to me we should disable the "Show password" feature
until the Firefox change is ready. I did not realize the subtleties involved in 
how Firefox saves form data, and the possibility of saving any user's password
in plain text just by pressing the FxA "Show" button makes me uncomfortable.

:rfeeley?

Comment 24

[Ryan Feeley [:rfeeley]]

IMO, this is a matter of striking a balance between security and convenience. Through user testing we know that users like the way our password manager works because it make logging into sites easier. We are currently running an experiment to see if users who show the password experience less friction when submitting a password. My hunch is that it makes like easier. Note that users on Fennec are far more likely to show passwords.

Do they exist in sessionstore.js as long as History persists?
Are they as easily accessed by a local attacker as saved passwords are?
Does sessionstore.js sync?

Comment 25

[Sai Pc]

I would like to propose something stupid: Why not have an overlay span/div with the password text when Show button is clicked?? that way, we can hide/show password without having to worry about the browser saving it for session restore, etc. and we can avoid changing the types of the input field.
:stomlinson

Comment 26

[Ryan Feeley [:rfeeley]]

Attachment: windows.gif

So only showing mouse is down? Like Windows does with this eye?

Comment 27

[Sai Pc]

:rfeeley Either that, or just overlay on a click and keep updating text of the overlay as the user types password. The focus is still bound to the password field, so the password gets updated and on submitting the form, the password is submitted as a password field. We can still have the toggle effect we have now with `Show/Hide` button.

Comment 28

[Mike de Boer [:mikedeboer]]

(In reply to Ryan Feeley [:rfeeley] from comment #24)
> Do they exist in sessionstore.js as long as History persists?

Stupid question: does that matter?

> Are they as easily accessed by a local attacker as saved passwords are?
> Does sessionstore.js sync?

If our saved passwords are stored in plaintext, unencrypted, then yes. And the attack vector is the same.
Not all of sessionstore.js is synced and the saved form data is not part of that, AFAIK.

Shane's option from comment 23 looks quite valid to me TBH, even though the functionality is very useful. (I don't want to see it go, so that's why I'm also trying to find a workable solution)

Comment 29

[Ryan Kelly [:rfkelly]]

This "syncing a <span> to the password field" thing sounds like a tremendous amount of complexity, let's not get too far ahead of ourselves.

IIUC the "show password" button is controlled by our A/B infrastructure, meaning it's pretty straightforward for us to switch it off and equally straightforward for us to switch it back on (yay web content!).  I propose that we just switch it off while we figure out a path forward here.

> Note that users on Fennec are far more likely to show passwords.

It's not obvious to me whether fennec will also have this behaviour, and if it should get a similar patch for ignoring fields with "autocomplete=off".

Comment 30

[Ryan Kelly [:rfkelly]]

> IIUC the "show password" button is controlled by our A/B infrastructure [...]
> I propose that we just switch it off while we figure out a path forward here.

:vladikoff, you're best placed to understand the intricacies of the show-password experiment, is this a reasonable thing to do?

Comment 31

[Ryan Feeley [:rfeeley]]

It's very likely that the users FxA credentials will exist in their Saved Passwords already, so I don't feel as if they are being exposed to additional risk.

When users authenticate to Sync Firefox automatically saves a credentials blob (not the actual password) but does ask the user if they want to save their FxA password as well.

More detail: https://bugzilla.mozilla.org/show_bug.cgi?id=1248765

Comment 32

[Mike Kaply [:mkaply]]

Timing for this is Firefox 48. If we can do something server side, that's even better.

Comment 33

[Ryan Kelly [:rfkelly]]

> It's very likely that the users FxA credentials will exist in their Saved Passwords already,

Saved passwords can be encrypted with the profile's master password, if you enable one, while I assume session-restore data is not.  In fact this is the very reason we store the FxA tokens and keys in the password-manager, because it's the most secure local storage bucket that we have.

Comment 34

[Matthew N. [:MattN]]

Comment on attachment 8765946 [details] [diff] [review]
Patch v1: add autocomplete='off' to list of ignored inputs of the session store formdata collector

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

Thanks Mike. r=me since this will match bfcache and implement the suggestion of the HTML spec with the one fix noted below.


(In reply to Mike de Boer [:mikedeboer] from comment #12)
> (In reply to Matthew N. [:MattN] (behind on reviews) from comment #10)
> > Maybe it's only used for bfcache then[1]? It seems like session store should
> > match bfcache for this (I thought it did at one point) and look for
> > autocomplete=off.
> 
> I'd be happy to implement this, but first I'd like to learn what
> autocomplete=off means exactly?

https://html.spec.whatwg.org/multipage/forms.html#autofill-processing-model:attr-fe-autocomplete-off-3

> When an element's autofill field name is "off", the user agent should not remember the control's data, and should not offer past values to the user.

and

> In addition, when an element's autofill field name is "off", values are reset when traversing the history.


Dolske's suggestion would also fix this specific FxA issue but wouldn't fix the inconsistency between bfcache and sessionstore so I think it can be filed as a separate improvement to both of those components.

::: toolkit/modules/sessionstore/XPathGenerator.jsm
@@ +109,2 @@
>      let formNodesXPath = "//textarea|//select|//xhtml:textarea|//xhtml:select|" +
>        "//input[" + ignore + "]|//xhtml:input[" + ignore + "]";

@autocomplete=off should also be handled on <textarea> and <select>. Could you add those two elements to the/a test as well?

Note that we may not want to land with the test to avoid disclosing this bug.

Comment 35

[Matthew N. [:MattN]]

(In reply to Ryan Kelly [:rfkelly] from comment #33)
> > It's very likely that the users FxA credentials will exist in their Saved Passwords already,
> 
> Saved passwords can be encrypted with the profile's master password, if you
> enable one

Saved passwords are never written to disk in plaintext regardless of whether a master password is setup.

Comment 36

[Ryan Kelly [:rfkelly]]

In meeting today, :vladikoff and :rfeeley mentioned they're going to review the current experiments we have in play around Show Password, which will help inform our course of action here; setting ni? for them to summarize the outcome back in this bug.

Comment 37

[Vlad Filippov (:vladikoff)]

From the data we gathered in the past 2-3 weeks, we can say that:

- the "Show password" improvement form submission by ~2%-5% depending on the view (we compared force_auth, sign in and sign up)
- in terms of checking for number of errors we can say that showing or hiding the input field does not change the number of errors users get by much. In some cases the "Show" password bus has less errors by 0.5%.

Comment 38

[Mike de Boer [:mikedeboer]]

(In reply to Matthew N. [:MattN] (PTO Jun. 29 - Jul. 1) from comment #34)
> Note that we may not want to land with the test to avoid disclosing this bug.

I think you meant 'generally', because autocomplete=off doesn't have _anything_ to do with this particular bug, at least not from the surface.

Comment 39

[Mike de Boer [:mikedeboer]]

I take that back. I'll land this without test and sans commit message.

Comment 40

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/ba5ff277a3cd1a7f655f8e02a130a395e8f2a225
Bug 1280294. r=MattN

Comment 41

[Mike Kaply [:mkaply]]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

I'm assuming this will get uplifted anyway, but want to make sure we track to FF48.

Comment 42

[Mike Kaply [:mkaply]]

[Tracking Requested - why for this release]: Needed for partner release on  Android.

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ba5ff277a3cd

Comment 44

[Matthew N. [:MattN]]

(In reply to Mike de Boer [:mikedeboer] from comment #40)
> https://hg.mozilla.org/integration/fx-team/rev/
> ba5ff277a3cd1a7f655f8e02a130a395e8f2a225
> Bug 1280294. r=MattN

Mike, it seems like you are handling @type for textarea/select when I only pointed out that you need to handle @autcomplete for those two elements. Was this intentional?

Comment 45

[Ryan Kelly [:rfkelly]]

I didn't see it mentioned explicitly in this bug, so I wanted to clarify that:

> who could fix the FxA login page to set autocomplete=off when the type is set to text?

FxA does in fact already do this, and IIUC this is the correct cross-browser way to indicate that the field shouldn't be persisted in form or session data. 

So here's a summary of my take on all this from the FxA side:

* The "show password" button seems to increase login success by around 5%, so we want to keep it long-term.
* We already use autocomplete="off", so the bug should be fixed for FxA as soon as the patch here ships in the browser.
* If you'd like us to, we can disable the "show password" button server-side until the fix is shipped; just ask

Comment 46

[Mike de Boer [:mikedeboer]]

(In reply to Matthew N. [:MattN] (PTO Jun. 29 - Jul. 1) from comment #44)
> Mike, it seems like you are handling @type for textarea/select when I only
> pointed out that you need to handle @autcomplete for those two elements. Was
> this intentional?

Yes, because I didn't want to complicate the code even further by making another distinction/ branch. The @type addition doesn't have any side-effects AFAIK, thus keeping it seemed valid.

Comment 47

[Mike de Boer [:mikedeboer]]

Comment on attachment 8765946 [details] [diff] [review]
Patch v1: add autocomplete='off' to list of ignored inputs of the session store formdata collector

Approval Request Comment
[Feature/regressing bug #]: no bug, inconsistency between session store and bfcache
[User impact if declined]: input fields that have 'autocomplete=off' set were still saved in the session store form fields cache, even though the spec tells us we shouldn't. The patch fixes this behavior.
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass.
[Risks and why]: minor.
[String/UUID change made/needed]: n/a.

Comment 48

[Mike de Boer [:mikedeboer]]

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

Comment 49

[Mike Kaply [:mkaply]]

Is someone on the hook to update the Firefox Accounts page to use autocomplete=off (or does it already)?

Comment 50

[Mike de Boer [:mikedeboer]]

(In reply to Mike Kaply [:mkaply] from comment #49)
> Is someone on the hook to update the Firefox Accounts page to use
> autocomplete=off (or does it already)?

Not sure. N-i Ryan to make sure.

Comment 51

[:Gijs (he/him)]

(In reply to Mike Kaply [:mkaply] from comment #49)
> Is someone on the hook to update the Firefox Accounts page to use
> autocomplete=off (or does it already)?

comment #45 says it does this already...

(In reply to Ryan Kelly [:rfkelly] from comment #45)
> I didn't see it mentioned explicitly in this bug, so I wanted to clarify
> that:
> 
> > who could fix the FxA login page to set autocomplete=off when the type is set to text?
> 
> FxA does in fact already do this, and IIUC this is the correct cross-browser
> way to indicate that the field shouldn't be persisted in form or session
> data.

Comment 52

[Matthew N. [:MattN]]

(In reply to Mike de Boer [:mikedeboer] from comment #46)
> (In reply to Matthew N. [:MattN] (PTO Jun. 29 - Jul. 1) from comment #44)
> > Mike, it seems like you are handling @type for textarea/select when I only
> > pointed out that you need to handle @autcomplete for those two elements. Was
> > this intentional?
> 
> Yes, because I didn't want to complicate the code even further by making
> another distinction/ branch. The @type addition doesn't have any
> side-effects AFAIK, thus keeping it seemed valid.

It does have side effects if an author uses @type on <textarea> or <select> even though those aren't standard attributes.

Comment 53

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8765946 [details] [diff] [review]
Patch v1: add autocomplete='off' to list of ignored inputs of the session store formdata collector

We want password info to behave as users would expect, let's uplift to aurora and beta.

Comment 54

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #52)
> (In reply to Mike de Boer [:mikedeboer] from comment #46)
> > (In reply to Matthew N. [:MattN] (PTO Jun. 29 - Jul. 1) from comment #44)
> > > Mike, it seems like you are handling @type for textarea/select when I only
> > > pointed out that you need to handle @autcomplete for those two elements. Was
> > > this intentional?
> > 
> > Yes, because I didn't want to complicate the code even further by making
> > another distinction/ branch. The @type addition doesn't have any
> > side-effects AFAIK, thus keeping it seemed valid.
> 
> It does have side effects if an author uses @type on <textarea> or <select>
> even though those aren't standard attributes.

Can we have a non-confidential followup bug to address this?

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6484e936f2c2

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/781c5864339c

Comment 57

[Mike Kaply [:mkaply]]

What is bug 1288721? Is it related to this issue or somethign different?

Comment 58

[:Gijs (he/him)]

(In reply to Mike Kaply [:mkaply] from comment #57)
> What is bug 1288721? Is it related to this issue or somethign different?

I've CC'd you.

Comment 59

[Matthew N. [:MattN]]

(In reply to :Gijs (slow, PTO recovery mode) from comment #54)
> (In reply to Matthew N. [:MattN] from comment #52)
> > (In reply to Mike de Boer [:mikedeboer] from comment #46)
> > > (In reply to Matthew N. [:MattN] (PTO Jun. 29 - Jul. 1) from comment #44)
> > > > Mike, it seems like you are handling @type for textarea/select when I only
> > > > pointed out that you need to handle @autcomplete for those two elements. Was
> > > > this intentional?
> > > 
> > > Yes, because I didn't want to complicate the code even further by making
> > > another distinction/ branch. The @type addition doesn't have any
> > > side-effects AFAIK, thus keeping it seemed valid.
> > 
> > It does have side effects if an author uses @type on <textarea> or <select>
> > even though those aren't standard attributes.
> 
> Can we have a non-confidential followup bug to address this?

Filed bug 1415739