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.

Btw, I tested using Mac trunk.

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

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.

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)

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.

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.

I see this on the 1.7 branch too.

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.

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.

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.

Morphing based on comment 6.

Would this be enough?

(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.

Why only block mutation events?

(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 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.

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.

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

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.

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.

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

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...

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.

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.

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.

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.

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

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.

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

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...

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.

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

> 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.

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.

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 on attachment 213254 [details] [diff] [review] Prevent mutation events propagate from <input> approved for 1.8.0 branch, a=dveditz

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.

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.

Checked in to trunk, 1.8.0 and 1.8.

changing status/keywords to match comment 40

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

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

Fix checked into aviary101 and mozilla1.7 branches

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).

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

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

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.

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.

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?

mouseout and mousein might be triggered on newer branches

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.

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.