Last Comment Bug 328566 - Can steal (upload) files by changing input type to file during DOMNodeInserted mutation event
: Can steal (upload) files by changing input type to file during DOMNodeInserte...
Status: RESOLVED FIXED
[sg:high] file stealing on post-1.8 t...
: fixed1.8.1, testcase, verified1.7.13, verified1.8.0.2
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug] (reviewing overload)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-02-25 04:36 PST by Jesse Ruderman
Modified: 2006-07-11 00:18 PDT (History)
11 users (show)
dveditz: blocking1.7.13+
dveditz: blocking‑aviary1.0.8+
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Prevent mutation events propagate from <input> (1.32 KB, patch)
2006-02-26 12:04 PST, Olli Pettay [:smaug] (reviewing overload)
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval‑aviary1.0.8+
dveditz: approval1.7.13+
bzbarsky: approval‑branch‑1.8.1+
dveditz: approval1.8.0.2+
Details | Diff | Splinter Review
Testcase that shows the bug on the 1.8 branches (559 bytes, text/html)
2006-02-27 11:35 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
with password manager - step 1 (454 bytes, text/html)
2006-03-02 00:30 PST, Jesse Ruderman
no flags Details
with password manager - step 2 (830 bytes, text/html)
2006-03-02 00:30 PST, Jesse Ruderman
no flags Details
simpler.html (without password manager) (573 bytes, text/html)
2006-03-02 00:31 PST, Jesse Ruderman
no flags Details

Description Jesse Ruderman 2006-02-25 04:36:47 PST
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 2006-02-25 04:38:54 PST
Btw, I tested using Mac trunk.
Comment 2 Jesse Ruderman 2006-02-25 12:27:09 PST
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 2006-02-25 13:28:45 PST
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] 2006-02-25 22:12:36 PST
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 2006-02-26 00:18:04 PST
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 [:bz] (still a bit busy) 2006-02-26 10:00:36 PST
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 [:bz] (still a bit busy) 2006-02-26 10:46:11 PST
I see this on the 1.7 branch too.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2006-02-26 10:48:55 PST
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 [:bz] (still a bit busy) 2006-02-26 11:09:11 PST
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 [:bz] (still a bit busy) 2006-02-26 11:10:10 PST
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 2006-02-26 11:49:50 PST
Morphing based on comment 6.
Comment 12 Olli Pettay [:smaug] (reviewing overload) 2006-02-26 12:04:44 PST
Created attachment 213254 [details] [diff] [review]
Prevent mutation events propagate from <input>

Would this be enough?
Comment 13 Olli Pettay [:smaug] (reviewing overload) 2006-02-26 12:12:11 PST
(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 2006-02-26 12:18:24 PST
Why only block mutation events?
Comment 15 Olli Pettay [:smaug] (reviewing overload) 2006-02-26 13:16:33 PST
(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 [:bz] (still a bit busy) 2006-02-26 15:16:17 PST
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] 2006-02-27 08:30:38 PST
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 [:bz] (still a bit busy) 2006-02-27 08:43:04 PST
See comment 7 (though that tree might not have been fully up to date on that branch).
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2006-02-27 08:48:27 PST
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] (reviewing overload) 2006-02-27 08:51:30 PST
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 [:bz] (still a bit busy) 2006-02-27 08:54:19 PST
I was testing with the "simpler" testcase, btw.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2006-02-27 09:07:21 PST
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 2006-02-27 11:17:17 PST
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 [:bz] (still a bit busy) 2006-02-27 11:34:57 PST
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 [:bz] (still a bit busy) 2006-02-27 11:35:23 PST
Created attachment 213353 [details]
Testcase that shows the bug on the 1.8 branches
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2006-02-27 11:43:48 PST
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 - use ni] 2006-02-27 12:40:37 PST
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 [:bz] (still a bit busy) 2006-02-27 12:49:19 PST
Marcia, which testcase did you use to test for the crash?
Comment 29 Marcia Knous [:marcia - use ni] 2006-02-27 13:00:51 PST
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] 2006-02-27 13:02:41 PST
Crashed in the testcase on Fx 1.0.8 build from 02/27

TB ID TB15705752H
Comment 31 Boris Zbarsky [:bz] (still a bit busy) 2006-02-27 13:12:17 PST
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 - use ni] 2006-02-27 14:31:04 PST
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] 2006-02-27 14:37:00 PST
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 [:bz] (still a bit busy) 2006-02-27 14:59:59 PST
> 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] 2006-02-27 17:20:28 PST
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] 2006-02-27 17:38:06 PST
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] 2006-02-27 17:42:51 PST
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 2006-02-27 23:12:12 PST
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 [:bz] (still a bit busy) 2006-02-27 23:23:21 PST
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] (reviewing overload) 2006-02-28 08:45:31 PST
Checked in to trunk, 1.8.0 and 1.8.
Comment 41 Daniel Veditz [:dveditz] 2006-02-28 11:24:28 PST
changing status/keywords to match comment 40
Comment 42 Dave Liebreich [:davel] 2006-03-01 14:11:27 PST
Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)
Comment 43 Daniel Veditz [:dveditz] 2006-03-01 23:10:50 PST
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] 2006-03-01 23:11:46 PST
Fix checked into aviary101 and mozilla1.7 branches
Comment 45 Jesse Ruderman 2006-03-02 00:30:00 PST
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 2006-03-02 00:30:28 PST
Created attachment 213724 [details]
with password manager - step 1
Comment 47 Jesse Ruderman 2006-03-02 00:30:53 PST
Created attachment 213725 [details]
with password manager - step 2
Comment 48 Jesse Ruderman 2006-03-02 00:31:22 PST
Created attachment 213726 [details]
simpler.html (without password manager)
Comment 49 Boris Zbarsky [:bz] (still a bit busy) 2006-03-02 01:42:37 PST
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 2006-03-02 12:18:54 PST
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 - use ni] 2006-03-02 13:54:53 PST
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 - use ni] 2006-03-02 15:11:51 PST
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 2006-03-02 22:48:10 PST
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 2006-03-03 09:42:08 PST
mouseout and mousein might be triggered on newer branches
Comment 55 Jesse Ruderman 2006-03-03 15:09:42 PST
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] 2006-03-08 17:25:44 PST
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.

Note You need to log in before you can comment on or make changes to this bug.