163598 - File upload vulnerability using event.originalTarget

Status: VERIFIED FIXED

(Core :: Security, defect, P1)

Description

[Jesse Ruderman]

The originalTarget of a mouseover event over the text field inside a file 
upload control is an anonymous div inside the text field.  The div's parentNode 
is the text field.  This allows a script to get a reference to the text field 
inside the file upload control, which in turn allows the script to upload a 
file if it knows the file's name.

This is a regression of bug 29517, "File upload vulnerability using 
event.target", which Joki fixed in nsHTMLInputElement.cpp (1.115).  Hyatt then 
changed the code to move event.target to event.originalTarget instead of 
discarding event.target (1.138).  He added event.originalTarget to allow 
listeners to see anonymous content behind an event (required because of 
regression fallout from bug 46505) 
<http://www.mozilla.org/status/2000-09-13.html>.

Comment 1

[Jesse Ruderman]

Attachment: demo

Comment 2

[David Hyatt]

The file upload control should patch its HandleDOMEvent method to retarget 
originalTarget to become the control itself.  THat way you don't break input 
fields, and you protect the contents of the file upload control from scrutiny.

Comment 3

[John Keiser (jkeiser)]

Setting originalTarget to null or this, whether you do it for just input
type=file or both, has the problem of breaking Home and End and likely a
crapload of other things.

CC'ing editor and selection dude, who are the ones that seem to be using
originalTarget (specifically selection).

A few possibilities:
- Make it impossible for web content to create a JS wrapper around anonymous
content (hmm.)
- Disallow web content from getting originalTarget (fixes this, but just a stopgap?)
- Make whoever needs originalTarget not need it anymore (sounds hard or
impossible, but may work)
- Have originalTargetNoReally in a separate, non-web-content-accessible
interface for people that need originalTarget without the security hassle
- Stop relying on web content being unable to access anonymous content of an
input type=file.  We could protect .value and fix this exploit, but then we'd
have to guard against keypress events and all other potential types of attack as
well.  This seems more security-leak-prone.

I will pick this up when I get in tomorrow.  Feel free to attack this, anyone,
or at least give suggestions.

Comment 4

[georgi - hopefully not receiving bugspam]

A little simpler testcase is:
------------------------------------------------------------
<form action="http://localhost/cgi-bin/x.pl" enctype="multipart/form-data"
method="POST">
<input type=text name=a><br>
<input type=file name=c
onmouseover='event.originalTarget.value="/var/www/html/index.html"'><br>
<input type=submit>
</form>
------------------------------------------------------------

Since there is a growing number of attacks on file upload, I suggest adding a
warning whenever the user uploads a file. 
Probably a dialog box showing the files which shall be uploaded with OK/Cancel
should do.
Uploading a file is relatively rare for most users I think.

Comment 5

[Jaime Rodriguez, Jr.]

Removing adt1.0.1, as there is no patch to eveluate for checkin to the 1.0 branch.

Comment 6

[joki (gone)]

Hmm.  My impression of this had been that originalTarget was a protected
property.  Clearly its not and should be.

>- Make it impossible for web content to create a JS wrapper around anonymous
>content (hmm.)
If you mean web content, sure.  If you mean script content that that breaks us.
>- Disallow web content from getting originalTarget (fixes this, but just a
>stopgap?)
My suggestion.
>- Make whoever needs originalTarget not need it anymore (sounds hard or
>impossible, but may work)
Very hard.
>- Have originalTargetNoReally in a separate, non-web-content-accessible
>interface for people that need originalTarget without the security hassle
That's more or less the first one again since it still has to be accessible from
trusted script.
>- Stop relying on web content being unable to access anonymous content of an
>input type=file.  We could protect .value and fix this exploit, but then we'd
>have to guard against keypress events and all other potential types of attack as
>well.  This seems more security-leak-prone.
I agree but this might be worthwhile in addition to protecting the
'orignalTarget' prop.

Comment 7

[kinmoz]

>- Disallow web content from getting originalTarget

I vote for this one too since it's not part of any standard.

Have any of the other nsIDOMNS* interfaces been audited yet?

Comment 8

[John Keiser (jkeiser)]

Hmm, do you think we can remove originalTarget altogether without breaking the
world?  If we could it seems like a good thing anyway.  Retargeting is for
internal stuff, not external.

I like the idea of disallowing creation of a wrapper around anon content for web
content since that makes the wall to anonymous content a lot more impenetrable.
 A chrome JS would have to create the wrapper and give it to web content JS,
which is a far less likely situation.  This would disable the whole range of
exploits related to getting the anonymous content of the file.  I'll try to
figure out how to do this.  Classinfo seems the logical place to start.

Comment 9

[joki (gone)]

No, I don't think we can remove it.

If you think we can feasibly disable wrapper creation in web content JS only I'm
all for it.  I agree that's a better long term fix.  On the other hand I don't
view the fix of disabling access of web content to originalTarget as a
shoestring fix, I think its a reasonable and effective fix.  So if a wrapper
creation fix looks isn't a fairly quick fix then we should just put the security
check in.

Comment 10

[David Hyatt]

John, I meant change originalTarget for the file upload control, not for the text field inside 
the file upload control.  I don't think that would break any existing code like Home and 
End.  In other words retarget originalTarget in the file upload control's HandleDOMEvent.

If you do restrict originalTarget access, you should do it based off the generator of the 
anonymous content.  In other words, don't let authors get to user agent anonymous 
content, but do allow authors to get to their own anonymous content (and let user agents 
get to user agent anonymous content).

Don't just wall off originalTarget arbitrarily for all HTML, however, as that would be a 
cheap overkill solution that would then limit XBL authors' ability to determine the original 
target of events in bindings.

Comment 11

[David Hyatt]

originalTarget is not an implementation detail or an "internal" field only.  It is supposed to 
be scriptable and available to Web page authors.  That was the intent.  The security flaw 
here is the mismatch in author vs. user agent, not the exposure of originalTarget itself, 
i.e., the ability of a Web page to get to user agent anonymous content is the real flaw 
here.

Comment 12

[John Keiser (jkeiser)]

Hyatt, as I said, if you retarget originalTarget *just for file inputs* to point
to the file input itself or to null, Home and End don't work.  I put if (type ==
NS_FORM_INPUT_FILE) { ...->SetOriginalTarget(nsnull) and (*this) } and both had
the same effect.  input type=text code remained the same.

What is originalTarget used for?  Denying access to it for web content is the
only short-term solution we have been able to think up.  The long-term one is
for web content never to be able to access anonymous content (unless it is XBL
anonymous content).

Comment 13

[David Hyatt]

If you allow access to originalTarget to get to XBL content and only cut off C++-created 
anonymous content, I'm ok with that solution.  So something like this?

(1) If originalTarget == target, then allow access, since it's ok.
(2) Otherwise call getBindingParent.  If the binding parent is null, then this is C++-created 
content, and access should be denied.
(3) Otherwise it's XBL content, so let it work.

Comment 14

[David Hyatt]

jkeiser, did you restore originalTarget when handling the bubbling phase?  You have to 
retarget it when going out to the parent, but then you return it to its value when handling 
bubbling (changing it again when you cross the parent boundary a second time).  I'm just 
curious if you only retargeted once in capturing and then left it mutated when bubbling 
happened.

Comment 15

[David Hyatt]

Attachment: You're right. This does break Home/End/arrow keys, but why?

Comment 16

[David Hyatt]

What DOM element are the editor listeners attached to?  I had assumed they 
were attached to the input field itself (the one inside the file upload 
control), but if that were the case, this patch should have worked.

Comment 17

[David Hyatt]

Ah, I see.  The problem is that the focus controller (over in dom/src/base) is 
using originalTarget in order to accurately know what is focused.  It has to 
know this information.  So if we start lying in the originalTarget field, it 
gets confused about where the focus is.

So we have a couple of options:
(1) Wall off originalTarget from all content (and let chrome keep using it 
obviously).  (Drastic overkill, but probably the easiest patch to implement.)
(2) Add another field, realTarget, or some such that could always hand back 
the true original target and never lie.  The focus controller could be patched 
to use that, and then file upload controls could lie about originalTarget.
(3) Are we allowed to hack the glue code? i.e., can we leave the C++ code 
alone and make sure JS hands back target when you ask for originalTarget but 
only for the file upload control?

Comment 18

[David Hyatt]

Unfortunately doing (1) would cripple XBL in HTML.

Comment 19

[David Hyatt]

Attachment: This patch fixes the problem.

Comment 20

[David Hyatt]

If we want to be really complete, we can patch all C++ code to use the trusted 
method, but I think these will be the only cases where it matters.

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 96094 [details] [diff] [review]
This patch fixes the problem.

Drive-by nit picking:

Remove the nsevent local declared on line 1360 in nsHTMLInputElement.cpp, it's
now useless.

How about changing those later variables' names from nsevent to privateEvent,
to match the earlier files in the patch, and to avoid reusing the bogo-name
"nsevent"?

/be

Comment 22

[Brian Ryner (not reading)]

Comment on attachment 96094 [details] [diff] [review]
This patch fixes the problem.

I'd suggest that if the caller is not allowed access to the real
originalTarget, we should return event.target, not null.  This would be more
consistent with the behavior if the event happened on non-anonymous content.

Comment 23

[John Keiser (jkeiser)]

Comment on attachment 96094 [details] [diff] [review]
This patch fixes the problem.

hyatt, that's a good fix, pretty low-risk too (for testing we need some
pounding on sites that do input type=file now--Hotmail attachments, Bugzilla
attachments).

r=jkeiser with two things:
(1) check IsContentOfType (nsIContent::eHTML_FORM_CONTROL) instead of eHTML to
narrow it down even further and do fewer QI misses
(2) QI to nsIFormControl instead of nsIDOMHTMLInputElement and do PRInt32 type;
GetType(&type); if (type == NS_FORM_INPUT_FILE).  Again performance.

I think we should remove this stuff once we make it impossible for to wrap
non-XBL anonymous content (and I still think we should do that).  I will file a
bug on that and reference this patch.

Comment 24

[David Hyatt]

Attachment: Address brendan, bryner, jkeiser comments.

Comment 25

[Asa Dotzler [:asa]]

this is wanted for the 1.1 branch for sure. please find a driver to review and
hopefully approve as soon as a patch makes it through r and sr. thanks.

Comment 26

[John Keiser (jkeiser)]

Sicking came up with a potential problem with this patch.  You can still upload
a file by creating XBL with an anonymous input type=file under it, and then
repeating the exploit.  target will be the XBL, originalTarget will be the text,
and BAM.

Sicking suggested having Yet Another Member for the realOriginalTarget to fix this.

I am gone to the airport, I cannot test or review patches for the next few hours.

Comment 27

[John Keiser (jkeiser)]

Attachment: Disable originalTarget for web content (Does Bad Things)

This patch disables originalTarget for web content.  It works, but probably
kills all sorts of XBL (assuming XBL relies on originalTarget a lot) and makes
the browser crash on shutdown (I haven't had time to look into this and hyatt's
patch is more surgical anyway).

Posting for posterity or Just In Case.

Comment 28

[Brian Ryner (not reading)]

Comment on attachment 96101 [details] [diff] [review]
Address brendan, bryner, jkeiser comments.

>+
>+  *aOriginalTarget = mOriginalTarget;
>+  NS_IF_ADDREF(*aOriginalTarget);
>+  return NS_OK;
>+}

You've already checked that mOriginalTarget is non-null, so change this to
NS_ADDREF instead of NS_IF_ADDREF.  r/sr=bryner with that change.

Comment 29

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

I'm not quite sure that the <input type=file> will actually submit when it's an
anonymous child of the bound element. However what I described would make it
possible to set the value and you can then move the node into the real document
and have it submitted

Comment 30

[John Keiser (jkeiser)]

Another potential fix for the Sicking Conundrum is to see if the first anonymous
parent of the originalTarget is input type=file.

I'm out, yo.  Good luck, see you all in a few hours.

Comment 31

[Johnny Stenback (:jst)]

Comment on attachment 96101 [details] [diff] [review]
Address brendan, bryner, jkeiser comments.

This won't plug the hole, an event originally dispatched inside a file control
must get the same protection, independent of the current event target. We
either have to walk the parent chain on every get of the original target on
every event to find out if we can expose the original target or not, or we do
this check (and set a bit or some such) when setting the original target.

Comment 32

[Johnny Stenback (:jst)]

Comment on attachment 96105 [details] [diff] [review]
Disable originalTarget for web content (Does Bad Things)

sr=jst, but this can potentially break pages if they rely on this mozilla only
property. I don't know what the risk involved in taking this fix would be. Our
evangelists might have a better idea on the risk involved here.

Comment 33

[Jaime Rodriguez, Jr.]

cc'ing evangelist to check and make we do not cause any site compatibility
regressions

Comment 34

[David Hyatt]

The walk of the parent chain is the best fix.  That will plug sicking's hole.  All you have to 
do is go up two levels (once to the text field, check that, then up to the file upload control).

Comment 35

[David Hyatt]

I'll whip that up here.

Comment 36

[Doron Rosenberg (IBM)]

cc: bclary, who has more US-based major site experience.  Personally, I haven't
seen originalTarget used on the web. I just searched all my site fixes (~190
European topsite fixes) and didn't find any use of it.

Comment 37

[Bob Clary [:bc] (inactive)]

I don't know of any sites which use this property. Considering it is non
standard it will be easy to evangelize sites to use target instead. This
property is also *not* included in the mozilla.org DOM Ref for Event.

Comment 38

[Christopher Blizzard (:blizzard)]

What's the status here?  I haven't seen an update in a while.

Comment 39

[John Keiser (jkeiser)]

hyatt: are you on this?  Should I whip up a patch?

I noticed the patch in here needs to fix nsGenericHTMLElement as
well--HandleDOMEvent there does this same originalTarget munging (I am not sure
why we do it both places).

Comment 40

[John Keiser (jkeiser)]

I am working on the patch.  Should have it up shortly.

Comment 41

[John Keiser (jkeiser)]

Attachment: Patch

This patch:
(1) moves the check to SetOriginalTarget() to increase speed in
GetOriginalTarget() (this necessitated adding mRealOriginalTarget to the event,
which should be OK since most of our DOM events are on the stack and are
temporary)
(2) checks the three most recent parents of originalTarget to see if any of
them are input type=file

Comment 42

[joki (gone)]

Comment on attachment 96242 [details] [diff] [review]
Patch

We'll need to test well to make sure that focus controller really is the only
internal customer that needs to use the new GetOriginalTargetTrusted method but
other than that it looks ok.

r=joki

Comment 43

[David Hyatt]

Yeah, that looks good to me.

Comment 44

[John Keiser (jkeiser)]

CC'ing Terri, who will be helping us test tonight

Comment 45

[Johnny Stenback (:jst)]

Comment on attachment 96242 [details] [diff] [review]
Patch

sr=jst

Comment 46

[Brendan Eich [:brendan]]

Comment on attachment 96242 [details] [diff] [review]
Patch

>+      while (parent) {
>+        numParentsChecked++;
>+        // Only matters if we're an HTML form control.  |originalTarget| is
>+        // defined all over the place for XUL, so this saves XUL (and non-form
>+        // controls) from paying the cost of a failed QI.
>+        if (parent->IsContentOfType(nsIContent::eHTML_FORM_CONTROL)) {
>+          nsCOMPtr<nsIFormControl> inputControl(do_QueryInterface(parent));
>+          if (inputControl) {
>+            PRInt32 type;
>+            inputControl->GetType(&type); 
>+            if (type == NS_FORM_INPUT_FILE) {
>+              NS_IF_RELEASE(mOriginalTarget);
>+              mOriginalTarget = nsnull;
>+              mRealOriginalTarget = aOriginalTarget;
>+              NS_IF_ADDREF(mRealOriginalTarget);
>+              return NS_OK;
>+            }
>+          }
>+        }
>+
>+        // We're looking for input type=file, and anonymous content doesn't go any
>+        // deeper than 3 deep under input type=file (input type=text, div,
>+        // textnode)
>+        if (numParentsChecked >= 3) {
>+          break;
>+        }
>+

Huge-ass silly nit: why not move the ++ down to the if that tests
numParentsChecked, prefixing it?

Also, why >= instead of == unless you're paranoid about someone not capable of
hacking on this code hacking on it?  I don't mind that paranoia, I'm not
picking a nit here -- just curious.

/be

Comment 47

[John Keiser (jkeiser)]

Sure, I'll do #1

#2: I just have a habit of doing checks like that as safely as possible, for the
reasons you cited as well as when checking arguments to functions.  In this case
it's not necessary, but unless there's a reason to change it I think safety first :)

Comment 48

[Jaime Rodriguez, Jr.]

Updating to ADT1 RTM.

tpreston/jkeiser: Any word on the test build results?

Comment 49

[Terri Preston]

I just got the build from John, will report back shortly

Comment 50

[Terri Preston]

Looks good to me, no new form control regressions found

Comment 51

[Terri Preston]

forgot to add, form testing was done on Win XP

Comment 52

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending
Drivers' approval. pls check this in asap, then replace "Mozilla1.0.1+" with
"fixed1.0.1". thanks! SHIP IT!!!

Comment 53

[Jaime Rodriguez, Jr.]

a=valeski

Received a verbal approval via mobile phone from Jud (on Drivers' behalf) for
checking this in to the 1.0 and every else it is needed (trunk and 1.1 branch). 

pls check this in asap, then replace "Mozilla1.0.1+" with "fixed1.0.1". thanks
again!

Comment 54

[John Keiser (jkeiser)]

Checked into 1.0 and trunk, will check in to 1.1 shortly

Comment 55

[John Keiser (jkeiser)]

Checked in to 1.1.  Fixed everywhere needed.

Comment 56

[David Hyatt]

Sorry about not super-reviewing this.  My "Looks good to me" comment was 
supposed to have an sr with it. :)

Comment 57

[georgi - hopefully not receiving bugspam]

A modification of this still causes the same troubles.
Check bug 164023

Comment 58

[chris hofmann]

Comment on attachment 96242 [details] [diff] [review]
Patch

a=chofmann,jud

Comment 59

[Jaime Rodriguez, Jr.]

jst and jkeiser believe it would be best to back this patch out in favor of the
new patch that will be used to block all ot these types of exploits. 

adt1.0.1+ (on ADT's behalf) approval to do so, pending drivers' approval.

Comment 60

[chris hofmann]

a=chofmann on the back out.

Comment 61

[Mitchell Stoltz (not reading bugmail)]

I filed bug 164687 based on Georgi's report of a similar bug using
event.relatedTarget.

Comment 62

[John Keiser (jkeiser)]

Please note that this patch was backed out and re-fixed in bug 164086.

Comment 63

[bsharma]

Verified on 7.0RTM on Win2000

The demo works as expected.