The default bug view has changed. See this FAQ.

File upload vulnerability using event.originalTarget

VERIFIED FIXED in mozilla1.0.1

Status

()

Core
Security
P1
major
VERIFIED FIXED
15 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: John Keiser (jkeiser))

Tracking

Trunk
mozilla1.0.1
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt1 RTM] [ETA 08/22] [FIX])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

15 years ago
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>.
(Reporter)

Comment 1

15 years ago
Created attachment 95984 [details]
demo
(Reporter)

Updated

15 years ago
Keywords: adt1.0.1
Keywords: nsbeta1
OS: Windows XP → All

Comment 2

15 years ago
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.
Keywords: nsbeta1
OS: All → Windows XP

Updated

15 years ago
Keywords: mozilla1.0.1, mozilla1.1
Keywords: nsbeta1+
(Assignee)

Comment 3

15 years ago
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.
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

15 years ago
Removing adt1.0.1, as there is no patch to eveluate for checkin to the 1.0 branch.
Blocks: 143047
Severity: normal → major
Keywords: adt1.0.1
Priority: -- → P1
Whiteboard: [adt2 RTM] [ETA Needed]
Target Milestone: --- → mozilla1.0.1

Comment 6

15 years ago
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

15 years ago
>- 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?
(Assignee)

Comment 8

15 years ago
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.

Updated

15 years ago
Blocks: 157592

Comment 9

15 years ago
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

15 years ago
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

15 years ago
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.
(Assignee)

Comment 12

15 years ago
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

15 years ago
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

15 years ago
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

15 years ago
Created attachment 96091 [details] [diff] [review]
You're right.  This does break Home/End/arrow keys, but why?

Comment 16

15 years ago
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

15 years ago
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

15 years ago
Unfortunately doing (1) would cripple XBL in HTML.

Comment 19

15 years ago
Created attachment 96094 [details] [diff] [review]
This patch fixes the problem.

Comment 20

15 years ago
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.

Updated

15 years ago
Attachment #96091 - Attachment is obsolete: true
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 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.
(Assignee)

Comment 23

15 years ago
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.
Attachment #96094 - Flags: review+

Comment 24

15 years ago
Created attachment 96101 [details] [diff] [review]
Address brendan, bryner, jkeiser comments.
Attachment #96094 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Blocks: 163768

Comment 25

15 years ago
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.
(Assignee)

Comment 26

15 years ago
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.
(Assignee)

Comment 27

15 years ago
Created attachment 96105 [details] [diff] [review]
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 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.
Attachment #96101 - Flags: review+
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
(Assignee)

Comment 30

15 years ago
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 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.
Attachment #96101 - Flags: needs-work+
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.
Attachment #96105 - Flags: superreview+

Updated

15 years ago
Whiteboard: [adt2 RTM] [ETA Needed] → [adt2 RTM] [ETA 08/20] [needs r=]

Comment 33

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

Comment 34

15 years ago
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

15 years ago
I'll whip that up here.
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

15 years ago
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.
What's the status here?  I haven't seen an update in a while.
(Assignee)

Comment 39

15 years ago
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).
Status: NEW → ASSIGNED
(Assignee)

Comment 40

15 years ago
I am working on the patch.  Should have it up shortly.
(Assignee)

Comment 41

15 years ago
Created attachment 96242 [details] [diff] [review]
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
Attachment #96101 - Attachment is obsolete: true

Comment 42

15 years ago
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
Attachment #96242 - Flags: review+

Comment 43

15 years ago
Yeah, that looks good to me.
(Assignee)

Comment 44

15 years ago
CC'ing Terri, who will be helping us test tonight
Comment on attachment 96242 [details] [diff] [review]
Patch

sr=jst
Attachment #96242 - Flags: superreview+
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
(Assignee)

Comment 47

15 years ago
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 :)

Updated

15 years ago
Keywords: adt1.0.1
Whiteboard: [adt2 RTM] [ETA 08/20] [needs r=] → [adt2 RTM] [ETA 08/20] [Needs ADT & Drivers Approval]

Comment 48

15 years ago
Updating to ADT1 RTM.

tpreston/jkeiser: Any word on the test build results?
Whiteboard: [adt2 RTM] [ETA 08/20] [Needs ADT & Drivers Approval] → [adt1 RTM] [ETA 08/22] [Needs ADT & Drivers Approval]

Comment 49

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

Comment 50

15 years ago
Looks good to me, no new form control regressions found

Comment 51

15 years ago
forgot to add, form testing was done on Win XP
(Assignee)

Updated

15 years ago
Whiteboard: [adt1 RTM] [ETA 08/22] [Needs ADT & Drivers Approval] → [adt1 RTM] [ETA 08/22] [Needs ADT & Drivers Approval][FIX]

Comment 52

15 years ago
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!!!
Keywords: adt1.0.1 → adt1.0.1+

Comment 53

15 years ago
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!

Keywords: mozilla1.0.1 → mozilla1.0.1+

Updated

15 years ago
Whiteboard: [adt1 RTM] [ETA 08/22] [Needs ADT & Drivers Approval][FIX] → [adt1 RTM] [ETA 08/22] [FIX]
(Assignee)

Comment 54

15 years ago
Checked into 1.0 and trunk, will check in to 1.1 shortly
(Assignee)

Comment 55

15 years ago
Checked in to 1.1.  Fixed everywhere needed.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 56

15 years ago
Sorry about not super-reviewing this.  My "Looks good to me" comment was 
supposed to have an sr with it. :)
A modification of this still causes the same troubles.
Check bug 164023

Comment 58

15 years ago
Comment on attachment 96242 [details] [diff] [review]
Patch

a=chofmann,jud
Attachment #96242 - Flags: approval+

Comment 59

15 years ago
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

15 years ago
a=chofmann on the back out.
(Assignee)

Updated

15 years ago
Depends on: 164086
I filed bug 164687 based on Georgi's report of a similar bug using
event.relatedTarget.
(Assignee)

Comment 62

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

Comment 63

15 years ago
Verified on 7.0RTM on Win2000

The demo works as expected.
Status: RESOLVED → VERIFIED
Group: security
You need to log in before you can comment on or make changes to this bug.