328566 - Can steal (upload) files by changing input type to file during DOMNodeInserted mutation event

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Jesse Ruderman]

For a demo, see http://www.squarefree.com/mutation-upload/.


Password manager does something like this:
1. Make sure the username field has type="text".
2. Make sure the password field has type="password".
3. Fill in the username field.
4. Fill in the password field.

During a DOMNodeInserted event triggered by step 3 (or even during a DOMNodeInserted event triggered by step 4 -- see below), I can change the password field into a file upload control and steal the file whose path&name is your password.


Exploiting this requires getting a user to save a password of your choice.  That isn't hard: an attacker could either

A) convince a user to save *some* password, with the filename in a hidden password field, or

B) use a bug 162020-like trick on the "Save password?" dialog.


There are arguably several things wrong here:

1. The web page gets a DOMNodeInserted event when the username is filled.  The originalTarget of the event is a text node in the textbox's editor, an implementation detail that web pages should never see.

2. Perhaps password manager should re-verify that the password field is really a password field just before filling it in.

3. If you change the type of the password field during the DOMNodeInserted event for the password field (rather than the username field), the exploit still works.  So fixing (2) alone wouldn't close off the hole.  I think this is because nsHTMLInputElement::SetValueInternal ensures it isn't a file upload control, then does stuff to its editor, then changes its internal value.

4. Mutation events allow web site scripts to execute at unexpected times, which makes it really hard to write correct code.  Maybe we shouldn't support them.

Comment 1

[Jesse Ruderman]

Btw, I tested using Mac trunk.

Comment 2

[Jesse Ruderman]

Here's a simpler exploit that doesn't rely on password manager, and as a result requires no user interaction at all:

http://www.squarefree.com/mutation-upload/simpler.html

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I wonder if this is a recent regression. In bug 325947 we changed the order of chaning mType and clearing the value. I actually was nervous about exactly this happening, but we didn't think it could. Apparently we were wrong.

Comment 4

[Daniel Veditz [:dveditz]]

You nominated for 1.8.0.3 -- have you tested on a branch build? If it's a regression from bug 325947 (and even if it isn't) we'd want this in 1.8.0.2.

Have you tested a 1.0.8 build? If this is because of bug 325947 then they should be affected as well. I tried this all three places on windows and can't seem to see the exploit. (trunk, 1.5.0.2 build, 1.0.8 build, 1.5.0.1 release)

Comment 5

[Jesse Ruderman]

I only tested on trunk.

The exploit in comment 2 could be a regression based on the description in comment 3, but I doubt the exploit in comment 0 is a regression.

Comment 6

[Boris Zbarsky [:bzbarsky]]

It sounds like we need a separate bug for password manager.  I'll look into the comment 2 testcase, since it's pure Gecko, but I'm not touching the toolkit password manager with a 10-foot pole.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I see this on the 1.7 branch too.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, I see the bug on the testcase in comment 2 even in builds from Feb 8 (so before the fix from bug 325947).  Investigating now.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK.  So this has nothing to do with bug 325947, actually.  What's going on here is the following:

1)  We set the value on the text input
2)  This messes with the anon nodes in the editor and causes a mutation event
    that bubbles out (first bug here!)
3)  The mutation event changes the type to file
4)  This makes us call into SetValueInternal()
5)  Since the frame owns the value (either because mType is file before bug
    325947 or because mType is text and the frame really does own the value
    after that fix), we end up setting the value in the frame.
6)  The frame ignores us trying to set the value, since it's in the middle of
    doing just that (see the mIsProcessing stuff in
    nsTextControlFrame::SetFormProperty).  If it didn't do that, we would
    probably recurse to death here instead (esp if the mutation listener were
    modified to, say, set the value to a random number).

I don't see any way of fixing this without fixing the issue in item 2.  The problem is that I'm not sure where we can cut off those events.  Do they need to propagate to the nsHTMLInputElement, for example?  Or could we just kill them based on target at the beginning of nsHTMLInputElement::HandleDOMEvent?

ccing smaug, since we'll probably need a solution on trunk too, and it'll need to be pretty different from branches given his event changes.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, and I won't be able to work on this till at least Wednesday (I need to fix bug 327078 and deal with some other commitments first).  So any help would be very very much appreciated.

Comment 11

[Jesse Ruderman]

Morphing based on comment 6.

Comment 12

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

Attachment: Prevent mutation events propagate from <input>

Would this be enough?

Comment 13

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

(In reply to comment #9)
> ccing smaug, since we'll probably need a solution on trunk too, and it'll need
> to be pretty different from branches given his event changes.
> 
For 1.9 I'll think some more generic solution for mutation event handling.

Comment 14

[Jesse Ruderman]

Why only block mutation events?

Comment 15

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

(In reply to comment #14)
> Why only block mutation events?
>
mousemoves etc. should still propagate. The originalTarget could be changed to
<input> ofc, (hoping without regressions).
Can you create any security problems with non-mutation events?
I tried, but no luck yet, fortunately.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 213254 [details] [diff] [review]
Prevent mutation events propagate from <input>

Non-mutation events can't fire from inside SetValue...

I think that should indeed be enough (if nothing else, I doubt that we have any code that depends on mutation events from the anon content).

So assuming this has gotten a bit of testing (esp. with file inputs), looks good.

Comment 17

[Daniel Veditz [:dveditz]]

Has anyone (Jesse in particular) reproduced this bug anywhere other than the trunk? On windows I can reproduce the "simpler" testcase on trunk, but not the 1.8.0, 1.8 or aviary101 branches (nor released 1.5.0.1)

I don't want to take untested patched into branches that don't need it.

Comment 18

[Boris Zbarsky [:bzbarsky]]

See comment 7 (though that tree might not have been fully up to date on that branch).

Comment 19

[Boris Zbarsky [:bzbarsky]]

So on Linux I can reproduce on the following builds (as of right now):

nightly in mozilla/nightly/latest-1.7
nightly in firefox/nightly/latest-aviary1.0.1
nightly in firefox/nightly/latest-mozilla1.8.0

I didn't bother testing 1.8 branch given that.

Comment 20

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

Hmm, I see this on (Linux) trunk only. I thought I tested this also with 1.8 
yesterday, but 1.8 seems to work ok (and now I wonder whether I tested 1.8 at 
all...). 1.8.0 is ok too, so is FF 1.0.7.
Anyway the patch does fix the problem on trunk.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I was testing with the "simpler" testcase, btw.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Er... I just realized that on 1.7 and 1.8.0 the type is not actually changing.  So the handler's not firing there, looks like.  Or something.

On trunk, the bug appeared between 2005-12-02-05 and 2005-12-03-06.  I have no idea what caused it -- perhaps bug 241518?  If I attach the listener to document instead of window, I crash with 1.7 and 1.8.0, fwiw...

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not really happy with this patch i must say (to fix the secirty issue i mean, it's good otherwise). I was nervous before about relying on no events being fired, and I am no less nervous now. I guess we can go for this for 1.8.0.2 since there are no known ways to break this.

But we need to separate mValue from mFileName on all branches. This insanity must stop.

Comment 24

[Boris Zbarsky [:bzbarsky]]

So as far as I can tell we're crashing on the branches because changing the type deletes the editor which is in the middle of doing stuff... so when we unwind back into editor code we end up referencing dead objects.  I get the same crash when I attach the listener to the node instead of the document.

With the listener attached to the node, the 2005-12-02-05 trunk build does NOT crash, and DOES show the bug.  Same for the 1.8.0 branch build.  So I would guess that bug 241518 fixed things so that mutation event listeners attached to windows started working more correctly, but the bug was present before that too.

It also looks to me like we definitely need this fix on the 1.8 branches.  Investigating the crash now.

I agree with sicking on the value thing, fwiw.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase that shows the bug on the 1.8 branches

Comment 26

[Boris Zbarsky [:bzbarsky]]

So on trunk the crash (or a crash?) went away between 2004-09-11-08 and 2004-09-13-08 (with the testcase I attached; similar testcases may still crash 1.8.0, per comment 22).

That was probably the fix for bug 257818... Not sure how relevant that is to this bug, since that bug was a trunk (not 1.7, that is) regression.  But after that point we no longer crash on that testcase....

I think if this patch fixes the 1.7 crash on the testcase I attached we should take it just because it keeps us from accessing deleted objects.

Comment 27

[Marcia Knous [:marcia]]

During the triage meeting, I was asked to try to reproduce this on the mac using the 1.0.8 branch build. I used Jesse's testcase in Comment 2, and was not able to see the vulnerability on the 1.0.8 branch. When I switched to the trunk I see it right away.  I also did not experience a crash on the trunk or the 1.0.8 branch.
 

Comment 28

[Boris Zbarsky [:bzbarsky]]

Marcia, which testcase did you use to test for the crash?

Comment 29

[Marcia Knous [:marcia]]

Tracy and I both see the crash using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.13) Gecko/20060227 Firefox/1.0.8 and running the test case that Boris cites in Comment 25.

We still need to figure out if this is enough of a problem that it should hold 1.0.8. Jesse mentioned that the crash along might be problematic since it is referencing deleted objects.

Comment 30

[Tracy Walker [:tracy]]

Crashed in the testcase on Fx 1.0.8 build from 02/27

TB ID TB15705752H

Comment 31

[Boris Zbarsky [:bzbarsky]]

Yeah, the deleted object thing is what was bothering me.  If the patch in this bug fixes that crash, I think we should take it.  If not, then I wouldn't hold for the crash fix...

Comment 32

[Marcia Knous [:marcia]]

Per dveditz's suggestion, I did try the Boris testcase (Comment 25) on the latest 1.0.8 Windows build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060227 Firefox/1.0.8). I did not get a crash. dveditz indicated we might be less concerned about this if it didn't crash on Windows.

Comment 33

[Tim Riley [:timr]]

I just got off the phone with dveditz.  How is this different from a whole bunch of other stirdom bugs cause crashes and  accessing deleted messages that we are not fixing?  At this point we are about two weeks late freezing and we have been only opening it up for publicly known security vulnerabilities or huge exposures to the same.  

We are also concerned that Jonas didn't like the patch and it is not clear of the patch is ok for the 1.7 or Aviary branches. 

Marcia just checked and the crash seems to me Mac only.

--Tim

Comment 34

[Boris Zbarsky [:bzbarsky]]

> How is this different from a whole bunch of other stirdom bugs cause crashes
> and  accessing deleted messages that we are not fixing?

We think it has a simple and reasonably safe fix (not sure whether someone has verified that this patch fixes it...).  Past that, it's no different, really.  I wouldn't cry if we push it out to the next 1.7/aviary release, or even if we don't take it on those branches at all; just wanted to make sure it was considered.

> We are also concerned that Jonas didn't like the patch

He didn't like it because it doesn't provide enough guarantees of safety.  Providing such guarantees would require the more invasive surgery comment 23 mentions at the end.

> Marcia just checked and the crash seems to me Mac only.

Well, Mac and Linux.  At least I'm getting the crash on Linux.  ;)

In any case, we should fix the password-stealing thing on 1.8 and trunk and perhaps spin the crash aspect off into a separate bug.

Comment 35

[Tim Riley [:timr]]

Based on comment #34 we should not hold 1.0.8 for this fix.  We already have what are hopefully final bits in hand and have started final testing.  If we have to re-spin, then we could take this as a ride along.  Minusing for 1.0.8/1.7.13.  "?" for 1.0.9, 1.7.14.

Comment 36

[Daniel Veditz [:dveditz]]

Confirming the file stealing in ff1.5.0.1 with Boris's testcase.

Confirming the moz1.7-branch crash on windows with a debug build. in nsRulNode::Transition() curr was 0xfdfdfdfd which is a "no man's land" boundary marker in a live object, meaning we've got a potentially exploitable memory issue. Also confirming that the patch fixes the crash.

Comment 37

[Daniel Veditz [:dveditz]]

Comment on attachment 213254 [details] [diff] [review]
Prevent mutation events propagate from <input>

approved for 1.8.0 branch, a=dveditz

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I disagree that this is just as bad as stirdom stuff we're leaving in.

For many of the stirdom crashes we don't actually have known exploits. We just see accesses of deleted memory and are concerned that it might be possible to control that memory enough to create an exploit. However doing so is fairly hard and in some cases might not even be possible. (I'm not saying that we shouldn't fix them asap of course).

For this bug it is known that the author can relativly easy steal any file from your harddrive.


Re comment 23. I'm not so much concerned that this patch will cause problems as I am that it still leaves holes. But not taking anything, which is the only option at this point, will for sure leave the hole, so that's much worse.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Sicking, the comparison to StirDOM was just about the 1.7 crash that my testcase triggers.  I think we all agree that the bug as originally files is more serious than that.

Comment 40

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

Checked in to trunk, 1.8.0 and 1.8.

Comment 41

[Daniel Veditz [:dveditz]]

changing status/keywords to match comment 40

Comment 42

[Dave Liebreich [:davel]]

Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)

Comment 43

[Daniel Veditz [:dveditz]]

Comment on attachment 213254 [details] [diff] [review]
Prevent mutation events propagate from <input>

approving for moz17/aviary101 branches per comment 35 -- we're respinning to pick up the fix for bug 317554. a=dveditz

Comment 44

[Daniel Veditz [:dveditz]]

Fix checked into aviary101 and mozilla1.7 branches

Comment 45

[Jesse Ruderman]

I'm moving testcases from http://www.squarefree.com/mutation-upload/ to this bug because of concerns that the URL might leak accidentally (e.g. through the Google Toolbar).

Comment 46

[Jesse Ruderman]

Attachment: with password manager - step 1

Comment 47

[Jesse Ruderman]

Attachment: with password manager - step 2

Comment 48

[Jesse Ruderman]

Attachment: simpler.html (without password manager)

Comment 49

[Boris Zbarsky [:bzbarsky]]

We still need a separate bug on password manager, no?  I doubt the fix for this bug fixed those testcases....

Comment 50

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Please file a new bug on the password version and assign it to me (assuming it is still happening of course)

Comment 51

[Marcia Knous [:marcia]]

verified on the 1.x branch using using the 2006-03-02-11-aviary1.0.1 Mac build. Testcase in Comment 25 looks fine. Adding relevant keyword. 

Comment 52

[Marcia Knous [:marcia]]

verified on the 1.7 branch using testcase in Comment 25. Mozilla 1.7.13
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060302. adding keyword.

Comment 53

[Jesse Ruderman]

This seems to be fixed for the password manager case too.  As long as there aren't other events for content to hook into between username filling and password filling, password manager should be safe.

Can any events be triggered by the username filling -- for example, an onmousemove if the cursor is over the filled text, or an attribute-changed event for the textbox?

Comment 54

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

mouseout and mousein might be triggered on newer branches

Comment 55

[Jesse Ruderman]

I'm not seeing mouseover and mouseout triggered between username filling and password filling on trunk, even with the cursor over the part of the username field where the username will appear.  So maybe password manager is safe -- at least on the trunk, at least for now.  It might be a good idea for it to get a belt+braces "is the password field still a password field?" check anyway.

Comment 56

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2, no exploit with any of the testcases.