1110557 - (CVE-2015-0822) Arbitrary File Read Vulnerability via Form Autocomplete

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Armin Ebert [:arminius]]

Attachment: auto.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141201171754

Steps to reproduce:

- Create a <input type="text"> field.
- Make sure, that the field's autocompletion will suggest "/etc/passwd" when triggered.
- Open the completion dropdown menu (which should solely suggest "/etc/passwd") by clicking in the text box or pressing the appropriate keys.
- With the menu still open, change the input type from "text" to "file".
- Although the input type changed, the completion menu does not dissapear and you will still be able to accept the filename as an autocompletion value.
- After completion the file picker has changed its value to "/etc/passwd" and will thus immediately make the file contents available to the DOM.

Confirmed on 3.17.4-1-ARCH x86_64 GNU/Linux.


Actual results:

If you try to change the value of a <input> field with type=file, you will get a security violation error. This restriction does not apply to chrome UI elements, though. The autocompletion box can therefore be abused the change otherwise inaccessible values and bypass the file upload dialogue.

The attached proof of concept outlines a possible real-life scenario, which tries to minimize the necessary user interactions to a simple double click. (Just for the record, there should be several ways to conceal the whole autocompletion process even more, making the attack much less obvious than depicted here, but probably resulting in a less stable and bloated PoC.)


Expected results:

The attempt of autocompleting a file upload form field should trigger a security error.

Comment 1

[Mats Palmgren (inactive)]

I can't reproduce it from the Bugzilla attachment, but it works locally using http://

Is it possible to cancel an ongoing form fill in HTMLInputElement::HandleTypeChange ?
Perhaps the formfiller code could check that @type is the same as when it started ?

Comment 2

[Mats Palmgren (inactive)]

Attachment: wip

This seems to fix it for me (on Linux).

We also have some formfill code (in JS) here:
http://mxr.mozilla.org/mozilla-central/find?string=toolkit%2Fcomponents%2Fformautofill%2F&tree=mozilla-central

I don't know what that is used for.

Comment 3

[Mats Palmgren (inactive)]

Exfiltrating arbitrary files that the user have read access to with a simple double-click
looks like a sec-high rating per:  https://wiki.mozilla.org/Security_Severity_Ratings

Comment 4

[Boris Zbarsky [:bzbarsky]]

I don't know much about the form fill code, sorry.

Comment 5

[Mats Palmgren (inactive)]

CC:ing some folks from the revision log who might know about the JS form fill code...
Is the code in toolkit/components/formautofill/ also vulnerable?

Comment 6

[:Paolo Amadini]

(In reply to Mats Palmgren (:mats) from comment #5)
> Is the code in toolkit/components/formautofill/ also vulnerable?

The code in this folder is still preliminary work, it can run only on Nightly when the about:config preference for the feature is enabled, so there is no exploitable vulnerability there.

When finished, the code will implement the requestAutocomplete API. According to the specification, modifications to the input value will be prevented in case the input field has mutated. The work to implement the required checks is tracked by bug 1020466.

Comment 7

[Mats Palmgren (inactive)]

OK, thanks Paolo.

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix

Dunno who can review this code. Olli, can you do it or suggest someone who can? Thanks.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8536251 [details] [diff] [review]
fix

(if gavin doesn't have time for review, feel free to put this back to my queue)

Comment 10

[:Gijs (out for now; he/him)]

Comment on attachment 8536251 [details] [diff] [review]
fix

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

I'm not a toolkit reviewer, but this change makes sense to me. Adding Marco in case that helps speed this up.

Comment 11

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

Comment on attachment 8536251 [details] [diff] [review]
fix

>diff --git a/toolkit/components/satchel/nsFormFillController.cpp b/toolkit/components/satchel/nsFormFillController.cpp

> nsFormFillController::AttributeChanged(nsIDocument* aDocument,
>                                        mozilla::dom::Element* aElement,
>                                        int32_t aNameSpaceID,
>                                        nsIAtom* aAttribute, int32_t aModType)

>+    if (mController) {
>+      StopControllingInput();

Why are you null-checking mController here? Should you instead null-check mFocusedInput?

Instead of always stopping/starting control of the input here, why not rename MaybeStartControllingInput -> ShouldStartControllingInput and have it return a bool, then only call StopControllingInput here if it returns false?

Comment 12

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #11)
> Instead of always stopping/starting control of the input here, why not
> rename MaybeStartControllingInput -> ShouldStartControllingInput and have it
> return a bool, then only call StopControllingInput here if it returns false?

I assume you mean something like:

if (ShouldStartControllingInput) {
  StartControllingInput();
} else {
  StopControllingInput();
}

cause if ShouldStartControllingInput would still invoke StartControllingInput() internally, the new name would not be great.

Comment 13

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

Attachment: alternate patch

I meant something like this (untested).

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 8537337 [details] [diff] [review]
alternate patch

I intentionally didn't do it like this because one ShouldControlInput()
state isn't necessarily the same as another ShouldControlInput() state.
I think it would be safer to be conservative here and call
StopControllingInput() *unconditionally* to reset the current setup,
and then re-initialize.

(In reply to :Gavin Sharp [email: [e-mail]] from comment #11)
> Why are you null-checking mController here?

The null-check is to avoid crashing in StopControllingInput().
MOZ_ASSERT is an alternative if you think it can never be null here.

Comment 15

[Marco Bonardo [:mak]]

(In reply to Mats Palmgren (:mats) from comment #14)
> I intentionally didn't do it like this because one ShouldControlInput()
> state isn't necessarily the same as another ShouldControlInput() state.

Afaict, ShouldControlInput runs checks only on the passed-into input, if any attribute was changed, it will read the updated attributes, so it should always return a bool that is only related to the passed-into input.
Thus, for readonly and autocomplete attribute changes the patch looks ok.

BUT, I'm not sure regarding type attribute change, and I don't think the suggested patch solves the security bug depicted here.
When type changes from text to file, looks like ShouldControlInput would keep returning true and we'd not close the suggestions popup (this is likely done by the controller when we unset input, I didn't verify though)

So, probably what Mats suggests is effectively the safer way in front of a security bug.
My if/else would also have worked cause we were either stopping or restarting (startControlling calls stopControlling internally)

> The null-check is to avoid crashing in StopControllingInput().
> MOZ_ASSERT is an alternative if you think it can never be null here.

mController can be null only if the controller instantiation fails in the constructor. I'd probably put a MOZ_ASSERT in the constructor and make ShouldStartControllingInput return false if mController is null.

Comment 16

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

(In reply to Mats Palmgren (:mats) from comment #14)
> I think it would be safer to be conservative here and call
> StopControllingInput() *unconditionally* to reset the current setup,
> and then re-initialize.

It seems wasteful (and potentially problematic) to reset things completely when we don't need to.

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #15)
> (In reply to Mats Palmgren (:mats) from comment #14)
> > I intentionally didn't do it like this because one ShouldControlInput()
> > state isn't necessarily the same as another ShouldControlInput() state.
> 
> Afaict, ShouldControlInput runs checks only on the passed-into input, if any
> attribute was changed, it will read the updated attributes, so it should
> always return a bool that is only related to the passed-into input.
> Thus, for readonly and autocomplete attribute changes the patch looks ok.

Right.

> BUT, I'm not sure regarding type attribute change, and I don't think the
> suggested patch solves the security bug depicted here.
> When type changes from text to file, looks like ShouldControlInput would
> keep returning true and we'd not close the suggestions popup (this is likely
> done by the controller when we unset input, I didn't verify though)

IsSingleLineTextControl should return false after the type attribute change, shouldn't it? So we'd call StopControllingInput.

It looks like maybe StopControllingInput doesn't close the popup, but it at least makes SetTextValue fail by clearing mFocusedInput (meaning we won't autocomplete anything into the input). But in any case it seems like Mats' patch would have the same issue...

Comment 17

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

Comment on attachment 8537337 [details] [diff] [review]
alternate patch

I hope you will reconsider given comment 15/comment 16.

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 8537337 [details] [diff] [review]
alternate patch

(In reply to :Gavin Sharp [email: [e-mail]] from comment #16)
> It seems wasteful ...

I don't see a performance issue here.  Dynamic changes of these
attributes on the focused input seems like it would be extremely rare;
and even when it does occur, the added code doesn't seem particularly
slow to me.

> (and potentially problematic) to reset things completely
> when we don't need to.

How is it potentially problematic?

> IsSingleLineTextControl should return false after the type attribute change,
> shouldn't it?

That's not the case I'm concerned about.  I'm concerned about the case
where ShouldControlInput() was true and is still true, in which case
your alternative fix does nothing.  Thus you assume that all the state
in nsFormFillController and nsAutoCompleteController is valid for the
new attribute values.  I think that's an unnecessary gamble and not
future-proof.

> It looks like maybe StopControllingInput doesn't close the popup [...]

StopControllingInput() calls mController->SetInput(nullptr):
http://hg.mozilla.org/mozilla-central/annotate/cb8ad2251c09/toolkit/components/satchel/nsFormFillController.cpp#l1182

which closes the popup here:
http://hg.mozilla.org/mozilla-central/annotate/cb8ad2251c09/toolkit/components/autocomplete/nsAutoCompleteController.cpp#l97

(In reply to :Gavin Sharp [email: [e-mail]] from comment #17)
> I hope you will reconsider given comment 15/comment 16.

I considered your comments, but I still think it's safer to call
StopControllingInput() unconditionally and then re-initialize the state
based on the new values.

Comment 19

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

(In reply to Mats Palmgren (:mats) from comment #18)
> I'm concerned about the case
> where ShouldControlInput() was true and is still true, in which case
> your alternative fix does nothing.

... which is what I would expect. Changing the type of an input from "text" to "search" should not have any effect with regard to autocomplete. In that case, your patch just does unnecessary work of tearing things down and then just re-adding them, which has no benefit (security or otherwise).

>  Thus you assume that all the state
> in nsFormFillController and nsAutoCompleteController is valid for the
> new attribute values.  I think that's an unnecessary gamble and not
> future-proof.

Why wouldn't that be a safe assumption? The only input-related state that nsFormFillController / nsAutoCompleteController care about is "can we autocomplete this field or not", which is precisely what ShouldControlInput answers.

Comment 20

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

Maybe it would be useful if you laid out some potential future change that would cause problems with my patch, but not with yours.

Comment 21

[Marco Bonardo [:mak]]

Imo, the likely someone changes code inadvertitely making ShouldControlInput not properly cut off across types with different security implication is not that ignorable.
First because we can't predict future singleLineTextControls (future types), so we can't predict the security implications of moving one type to another.
My feeling (that I guess is similar to Mats') is that we are relying on code "somewhere else" to do the right thing, rather than being explicit. And we are not even providing a test covering that "somewhere else" code...
And I agree with Mats that we should not be overzealous about a possible perf issue that is so rare that it won't actually make a difference. How many times someone needs to change the type attribute of a form field? I can't think of more than a couple rare use-cases.

Could we maybe split the attributes checks so we always stopControlling for type (that way we know any type could be added in future, we'll restart), while we take Gavin's suggested path for the other attributes?

Comment 22

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

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #21)
> Imo, the likely someone changes code inadvertitely making ShouldControlInput
> not properly cut off across types with different security implication is not
> that ignorable.

I don't understand - if ShouldControlInput starts returning true for a new input type that is not safe to autocomplete, then that's a problem regardless of which of these patches we take, because the non-type-switching OnFocus path will call StartControllingInput, and these patches won't save us.

Comment 23

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #22)
> I don't understand - if ShouldControlInput starts returning true for a new
> input type that is not safe to autocomplete, then that's a problem
> regardless of which of these patches we take

The problem, as I see it, it's not that something is safe or not to autocomplete, but that autocomplete values for one type could not be safe for another type. Anything should manage it's own autocomplete safety, but stopping on type change would also ensure values don't cross the boundary between different types.

Comment 24

[Mats Palmgren (inactive)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #21)
> How many times someone needs to change the type attribute of a form field?

NB. the _focused_ form field.

Comment 25

[Mats Palmgren (inactive)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #19)
> Changing the type of an input from "text" to "search" should not have 
> any effect with regard to autocomplete.

Perhaps today, but we shouldn't assume that it will always be the case.
I don't see why "search" and "text" must necessarily use the exact same
nsAutoCompleteController instance, or class even.  We also shouldn't
assume that "text" and "search" are the only possible values that makes
IsSingleLineTextControl true (there are many new @type values in the
HTML spec, and likely more coming).  Nor should we assume that
IsSingleLineTextControl()==false implies ShouldControlInput()==false,
there may be some exotic new @type in the future that requires new
fields in nsFormFillController and a whole different setup for the
nsAutoCompleteController.

> In that case, your patch just does unnecessary work of tearing things 
> down and then just re-adding them, which has no benefit (security or 
> otherwise).

You're missing the point.  I am treating ShouldControlInput() as a black
box - not making any assumptions on what it contains.  My patch will do
the right thing even after future changes in this code, which is good
for security and avoiding regressions in general.

> Why wouldn't that be a safe assumption? The only input-related state 
> that nsFormFillController / nsAutoCompleteController care about is "can
> we autocomplete this field or not", which is precisely what 
> ShouldControlInput answers.

Because the object state may be different between two elements that
both have ShouldControlInput()==true.

You are making the assumption that all future versions of these two
classes will have the exact same state they have today.  Assumptions
like that are bad, not just for security, but in general.  It's safer
to make the opposite assumption: that the state *is* invalid after
@type has changed.  (This isn't the first bug of this nature, BTW.)

There are circumstances where I'd be willing to compromise.  For
example by adding extensive tests and MOZ_ASSERTs to catch any unsafe
change in the future.  In this case though, there is no cost in just
doing the right thing to begin with.

Comment 26

[Mats Palmgren (inactive)]

Attachment: fix

Comment 27

[Marco Bonardo [:mak]]

Comment on attachment 8543290 [details] [diff] [review]
fix

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

As I said, I'm fine with the approach, considered it's hit only by rare events.

Though, I'm really not willing to bypass Gavin, so r=me provided he doesn't care that much.

PS: I'm still sad we don't have a chrome test here.

::: toolkit/components/satchel/nsFormFillController.cpp
@@ +121,5 @@
> +  if ((aAttribute == nsGkAtoms::type || aAttribute == nsGkAtoms::readonly ||
> +       aAttribute == nsGkAtoms::autocomplete) &&
> +      aNameSpaceID == kNameSpaceID_None) {
> +    nsCOMPtr<nsIDOMHTMLInputElement> focusedInput(mFocusedInput);
> +    // Reset the current state of the controller, unconditionally.

this comment could clarify the reason we do that, too, that is something like "to avoid leaking unsafe autocomplete values across different input types."

Comment 28

[Mats Palmgren (inactive)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #27)
> this comment could clarify the reason we do that, too, that is something
> like "to avoid leaking unsafe autocomplete values across different input
> types."

I don't think we should spell that out so explicitly in this patch.
I can file a followup bug to add a more detailed code comment once
this bug is public.

Comment 29

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

I hadn't noticed the mListNode reference in nsFormFillController - that seems like an existing issue (not supporting dynamic changes to the "list" attribute on a controlled input). I suppose we could address it by adding "list" to the set of attributes we watch for in your patch.

I don't have any objection to landing that patch.

Comment 30

[Mats Palmgren (inactive)]

That's a good point, but it doesn't seem like a security issue so perhaps it would
be better to address it in a separate bug?  I'd be happy to add 'list' to the
attributes to check for, but I don't think it's as simple as that.  We would also
need to detect changes to what the list ID refers to.  If that element is removed,
or a matching element added, or an element changing its ID so that it now matches,
etc.  I'd rather keep this patch simple, since it needs to be uploaded to branches.

Comment 31

[Mats Palmgren (inactive)]

Attachment: fix

Then again, I guess it doesn't hurt to also check 'list' even though it's
an incomplete solution for that problem.  Interdiff:

< +       aAttribute == nsGkAtoms::autocomplete) &&
---
> +       aAttribute == nsGkAtoms::autocomplete || aAttribute == nsGkAtoms::list) &&

Comment 32

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

Comment on attachment 8544555 [details] [diff] [review]
fix

I think the "list" fix should be split off to another bug to be dealt with properly (not a security issue).

Comment 33

[Armin Ebert [:arminius]]

Comment on attachment 8544555 [details] [diff] [review]
fix

I have been successfully unable to reproduce the vulnerability or find a workaround after applying patch #8544555. So, taking my limited understanding of the entire form fill logic into consideration, the bug looks fixed from my perspective.


BTW I strongly agree with the idea of an unconditional reset - not solely from a security point of view.

As far as I have observed, the completion box is laid out extremely inflexible and "persistent" once open:
It does not scroll with the content or respond to resizing; if you remove() the corresponding <input> element, it will still be present and if you globally document.write('something') to overwrite the whole DOM tree I even manage to respawn the box in the top left corner outside the browser window. In some cases, the box also persisted on the screen when switching to a different workspace of my Linux WM.

So, talking about user experience and since the formfill is unresponsive to any layout changes, I would consider visual style changes a more likely event and more signifcant issue than the minor disturbance of a closed completion box whenever the type attribute has changed.

Therefore you might event want to be more restrictive and reset the controller on additional occasions (e.g. on deletion of corresponding element).

Comment 34

[Mats Palmgren (inactive)]

Comment on attachment 8544555 [details] [diff] [review]
fix

(In reply to :Gavin Sharp [email: [e-mail]] from comment #32)
> I think the "list" fix should be split off to another bug to be dealt with
> properly (not a security issue).

OK.

Comment 35

[Mats Palmgren (inactive)]

Attachment: fix, v2

So, I pushed the last patch to Try and it triggered some failures. 
browser/base/content/test/general/test_contextmenu_input.html
hangs for example.  I can reproduce that locally and the problem
is that the Stop/StartControllingInput re-registers the mutation
observer, so when we do that from within AttributeChanged itself
we'll get another call for the change we're currently being notified
about, ad nauseam.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7353011b206e

So to avoid that I delayed the MaybeStartControllingInput call
by dispatching it in a Runnable, which seems to work:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e04e06b305cf

Comment 36

[Marco Bonardo [:mak]]

Comment on attachment 8547201 [details] [diff] [review]
fix, v2

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

::: toolkit/components/satchel/nsFormFillController.cpp
@@ +147,5 @@
> +    // Then restart based on the new values.  We have to delay this
> +    // to avoid ending up in an endless loop due to re-registering our
> +    // mutation observer (which would notify us again for *this* event).
> +    NS_DispatchToCurrentThread(
> +      new MaybeStartControllingInputRunnable(this, focusedInput));

I think you could avoid the explicit runnable definition and do something like

nsCOMPtr<nsIRunnable> event =
   NS_NewRunnableMethodWithArg<nsCOMPtr<nsIDOMHTMLInputElement> >
     (this, &nsFormFillController::MaybeStartControllingInput, focusedInput);
NS_DispatchToCurrentThread(event);

(provided you make MaybeStartControllingInput return an nsresult)??

Comment 37

[Mats Palmgren (inactive)]

Attachment: fix, v3

Yes, that's better, thanks.

Comment 38

[Mats Palmgren (inactive)]

Comment on attachment 8549708 [details] [diff] [review]
fix, v3

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Probably not that hard for someone with experience in such things.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The code reveals it has something to do with "FormFillController" and changing @type or @readonly or @autocomplete.  I think most people would guess that @type is the sensitive one.

Which older supported branches are affected by this flaw?

All, as far as I know.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I expect the patch will work fine on branches as is.

How likely is this patch to cause regressions; how much testing does it need?

Low risk.

Comment 39

[Mats Palmgren (inactive)]

Comment on attachment 8549708 [details] [diff] [review]
fix, v3

Approval Request Comment
[Feature/regressing bug #]: since the introduction of the form fill fetaure?
[User impact if declined]: user can be tricked to leak local file contents with a simple double-click
[Describe test coverage new/current, TBPL]: latest patch is only tested locally
[Risks and why]: low risk
[String/UUID change made/needed]: none

Comment 40

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8549708 [details] [diff] [review]
fix, v3

sec-approval+ for checkin on January 26, which is two weeks into the current six week cycle. Once it is green on trunk, we can checkin on Aurora and Beta. I'd like to see this fixed on ESR31 as well.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f0738c201eb

Needs an esr31 approval request from the looks of it.

Comment 42

[Mats Palmgren (inactive)]

Comment on attachment 8549708 [details] [diff] [review]
fix, v3

[Approval Request Comment]
see comment 38 / 39

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f0738c201eb

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/85d74ddebbcc
https://hg.mozilla.org/releases/mozilla-beta/rev/3f71dcaa9396

Comment 45

[Mats Palmgren (inactive)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #32)
> I think the "list" fix should be split off to another bug to be dealt with
> properly (not a security issue).

Filed bug 1125912.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/85d74ddebbcc

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/891f9e72b4c2

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/853006ae64d5
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/8e9744a33b76
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/62aa97491525