Node cloned from external, untrusted document and appended to chrome document.

VERIFIED FIXED in mozilla1.0.1



17 years ago
12 years ago


(Reporter: jrgmorrison, Assigned: security-bugs)



Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)


(Whiteboard: ,adt1.0.1+, approval, [ADT1 RTM], URL)


(1 attachment)



17 years ago
I was looking at bug 149012, which is crash because an image with absolute 
positioning is appended as a Node to a Node in a XUL chrome document. I have
a bandaid fix for the crash, but it occurred to me that cloning a Node from 
an untrusted source and appending that into a chrome document may be an 
insecure approach. 

The code in question is in pageInfo.js, beginning at about line 791, with 
the cloneNode() at line 803 and the appendChild() at line 815. [Line numbers
in current trunk builds].

-> mstoltz for review

Including Daniel Brooks <> who I believe wrote most of pageInfo


17 years ago
Group: security?
Well, the reason I just cloned the node in the first place was to avoid having 
to special case for each type of object it displays. Unfortunatly we do that 
now anyway, so it really doesn't matter.

I suppose I haven't really given the security concerns enough thought though. I 
suppose technically if you clicked on the image itself (where it shows up in 
the preview) it would fire the image's onclick event handler, probably with all 
the permissions of chrome. That could be a bad thing, but remeber that the 
vitim has to open page info (pretty rare unless I miss my guess), preview that 
specific image (but it could be the first image on the page) and then click on 
the image itself (maybe not so rare if you're exploring for the first time?) 
Anyway, it's low priority as far as security goes.

However, I've got a number of other changes in hand, so I'll go ahead and 
rewrite it so that it doesn't clone anything. I'll probably attach all my 
changes to a single bug for my own sanity, even though it makes reviews take 
Assignee: mstoltz → db48x
Severity: normal → major
Priority: -- → P1
Target Milestone: --- → mozilla1.0.1
This bug is exploitable, as in the example below from Georgi. That means we need
a fix ASAP.

Written by <a href="">Georgi Guninski</a>
<img src="givemeanerror" id="im" width="100" onerror="alert(Components.classes)">

Select <b>"Page Info | Media"</b>
We need to make sure that independent of if we use the DOM's cloneNode() method
to create the img or whatever elements that we're about to insert into the
chrome we must make sure that we never move over event handlers. IOW we need to
either white list the attributes that are safe to copy, or black list the ones
that are not. 

Hmm, seems like this would also exploitable with <img
src="javascript:alert(Components.classes);">, but it doesn't look like that
works, probably due to the way we load images at this moment, but are we sure
that'll remain the case? IOW we should probably black list javascript: URL's (or
white list the protocols we want to support?) from images that are shown in the
page info dialog.
I think the less attributes are copied, the better.
src=javascript:alert() used to work but seems fixed now (though js console gives
an error)
Can anyone suggest a whitelist of safe attributes to copy?
Seems like the page info dialog only needs the following attributes for images:


And it seems to me that we could safely ignore showing info about images whose
src is a javascript: or data: URL's, at least for now. Or would we rather
whitelist the trusted protocols (but what would those be)?

Ok, so that's images, what about other types of elements? Does the page info
dialog show anything about plugins, frames, or iframes?
jrgm has volunteered to fix this. Thanks, John!
Assignee: db48x → jrgm
Page info shows information about anything included in the page with embed, 
object, img, link (favicons basically), image inputs, and applets. 

As far as acceptable protocalls, I would think that ftp, http, and gopher are 
the only ones that are known to be safe. data can most likely be used as well, 
if the mime type starts with image/. 
Don't forget https
mmm, yes, I'd forgotten https. I wonder if there's anything else I'm forgetting?

As for implementation, I'd started out by just not cloning the node ever, 
that's the bulk of it. Mine doesn't block by source protocol though. What I 
haven't done yet is decide exactly what to put in the preview when there isn't 
an image. I could leave it blank, but that'd look funny. I could also put an 
image in there that says "Preview not available", or perhaps that'd be more 
cleanly done with xul. Now that I think about it, using xul would be 
localizable, which is good I suppose. Just have a child of the preview window 
with a label in it, and unhide it instead of showing the image. I'd say make 
the text centered and italics or something. You could even mimic the broken 
image look pretty easily if you wanted.
Please don't get fancy at the expense of getting this fixed quickly. The broken
image icon, etc., sounds great, but our goal for 1.0.1 is to close the security
hole, and we're running out of time for that. Also, drivers are pretty
risk-averse right now on the 1.0.1 branch, so what we need is a patch for the
branch that closes the hole with minimal risk and minimal added functionality.
We can go back and do it better on the trunk.
I'd hardly call adding an extra border and an extra image node fancy, but whatever.
I didn't mean don't do it, I just meant don't let it delay the fix.


17 years ago
Keywords: mozilla1.0.1, nsbeta1

Comment 14

17 years ago
Patch for the branch. (Could also land on trunk for the security fix
but may want to remove the "javaEnabled" stuff from a trunk landing
and wait for fixes to the <applet> DOM problems).


1) elimate the use of cloneNode() entirely.
2) checks the absolute URL for a whitelist of "^(https?|ftp|file|gopher):"
3) Only for <IMG>, <INPUT TYPE=IMAGE> and <LINK REL=ICON>, where the
   URL is allowed, set the 'src' of a new image to the 'src' of the
   original image.
4) When protocol is not allowed and/or element is OBJECT or EMBED, set
   the image to 'loading-image.gif' (a blank image would be better,
5) If Java is enabled, don't try to show |document.applets| in the
   media tab, since (a) it's broken, (b) it may crash and/or have some
   problems where it shows mixed content from two sites in the media

I'll do some more testing on this, but this is ready for review.

Comment 15

17 years ago
I guess I also dropped the "weird funky hack" for 'item.href' cuz I didn't see
where it would have any affect.
Why are we dropping data: exactly?

Other than that, the patch looks good...
Because data: URL's can carry HTML data with scripts in them.

Comment 18

17 years ago
Is it bulletproof to test for /^data:image\// 

+  var isProtocolAllowed = regex.test(absoluteURL) || 
+                          /^data:image\//.test(absoluteURL);

(Although, I might be inclined to be 'better safe than sorry' on the branch).

Comment 19

17 years ago
By the way, are there any charset encoding issues that might make my regexp
matching look naive? Or will JS do the right thing.
Comment on attachment 88397 [details] [diff] [review]
patch (for branch) for review

yeah, better safe than sorry sounds good to me.  r=bzbarsky to land this on the
Attachment #88397 - Flags: review+
JS will dtrt; all the stuff is in unicode by the time JS sees it.
Comment on attachment 88397 [details] [diff] [review]
patch (for branch) for review

Attachment #88397 - Flags: superreview+

Comment 23

17 years ago
I'm assigning back to mstoltz for a final 'yo' and checkin (I don't have 
CVS access to the main tree). Also, any comments Daniel?
Assignee: jrgm → mstoltz
Looks good. I'll get it checked in.


17 years ago
Whiteboard: [ADT1 RTM]
Comment on attachment 88397 [details] [diff] [review]
patch (for branch) for review

my only comment is about the weird funky hack you took out. If I recall, I
added that to make link rel=icon work. Did you test them? If it works then
great, it's always good to remove a hack. Especially the weird funky ones that
don't make any sense.

r=db48x, fwiw
Ah, yes.  I missed the part about the weird funky hack.  That needs to stay in....

Comment 27

17 years ago
Funny. I did test for <link rel=icon> and it works. I didn't really think it
through at the time, but I see now that 'getSource()' returns the href property
if it exist, or the src property if it exists, or null.
any idea on what's broken that it needs that?
I wonder if getSource() should switch on the nodename to decide which property 
to return? maybe I should go back and look at the version before getsource was 
added. I think it's obfuscating the reason for the hack.

Comment 30

17 years ago
Actually, I thought it nicely encapsulated 'do the right thing'. Although
there are days when I wonder how much memory cost we incur from lots of 
lil' ol' JS functions.

But, as it stands, this works for icon/img/input-image's, closes the security
hole, fixes the crash from bug 149012, and works around the applet bustage.
This should land as-is on the branch.

I think it should also land as-is on the trunk, including the applet 
workaround, and we can leave various clean-up and better implementation 
issues for later work.
no, it couldn't have been for link icons (the assignment was inside an else 
block that was only hit when the nodename wasn't link or input.) I don't 
remeber what it was for then. Did you try a page with all the different things 
it should be able to display (embed, object, img, link, input) to make sure 
they all showed up?
yes, it does need to be checked in, it's a very good patch. I just can't help 
but feel that that line is still needed in one form or another. I know it 
smacks of cargo cult programming or something, but I had to have had a reason 
for putting it there in the first place, right?

Comment 33

17 years ago
W3C HTML4 says that 'SRC' is an attribute of A, AREA, LINK, and BASE.  The
legacy docs for MSIE and Nav4.x don't mention it for additional elements. 

So, I don't see where we are dropping an HREF. We use the one on the <LINK>,
and I can't find any legacy reference to some other media being imported with
HREF. (But I wouldn't be entirely surprised if some browser at some time
supported this as an undocumented feature, and some website used this feature
[cf. the Image.x and Image.y undocumented properties in Nav4.x]).

(Note: for EMBED, OBJECT or APPLET, we no longer clone, so we don't have a way
to represent these in that dialog (and they weren't really working perfectly
before). [Longer term, we could return to cloning if we worked out an
acceptable sandbox solution, and didn't append the clone to the trusted XUL

But, I think we all agree that we can land this patch and work out the other 
issues later. (And I'll add that I really appreciate the work that has gone 
into that page info dialog. Very cool.)


17 years ago
Blocks: 149012
Fixed on trunk. Needs verification and branch approval.
Keywords: approval
bsharma - can you pls verify this on the trunk? thanks!

resolving as fixed per Comment #34 From Mitchell Stoltz, and adding adt1.0.1 for
branch consideration.
Blocks: 143047
Last Resolved: 17 years ago
Keywords: nsbeta1 → adt1.0.1, nsbeta1+, topembed, verifyme
Resolution: --- → FIXED
Whiteboard: [ADT1 RTM] → [ADT1 RTM] [06/25]
*** Bug 149012 has been marked as a duplicate of this bug. ***

Comment 37

17 years ago
Verified on 2002-06-24-Trunk build on WinNT

Loaded the test case provided in comment #2. Loaded Page Info | Media, and
clicked on the Save As button. An exception is thrown in the JS console.

jrgm, please attach more test cases, if this test is not enough.

Comment 38

17 years ago
The exception "Error: uncaught exception: Permission denied to get property 
UnnamedClass.classes" is thrown when you load that testcase, right? (That
is the expected result). When you actually do the 'Save as...' in the Page Info 
dialog, it should tell you that "The link could not be saved" since it is 404
(also the expected result).

Anyways, since we never do |cloneNode()| then I think we have closed off the 
security issue. The other part of verifying this bug (which I've done, but I'd 
like someone else to confirm) is that I haven't broken anything (aside from 
the fact that (object|embed|applet) now display as a 'broken image', which I 
intended to do). So, just surfing to random pages (particularly where applets 
and/or flash, etc. are known to exist) and then doing Page Info and using the 
features of that dialog would be a reasonable test.

Comment 39

17 years ago
adding adt1.0.1+.  Please get drivers approval before checking into the branch.
Keywords: adt1.0.1 → adt1.0.1+


17 years ago
Attachment #88397 - Flags: approval+

Comment 40

17 years ago
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+
Fixed on trunk, adding fixed1.0.1.

Does embedding use the Page Info dialog? If not, then this should not be topembed.
Keywords: mozilla1.0.1+ → fixed1.0.1
You misspelled branch.

Comment 43

17 years ago
Verified on 2002-07-19-branch build on Win 2000

1. Loaded the test case provided in comment #2. Loaded Page Info | Media, and
clicked on the Save As button. An exception is thrown in the JS console.

2. When do the 'Save as...' in the Page Info 
dialog, it tells that "The link could not be saved" since it is 404
(also the expected result).
Keywords: fixed1.0.1, verifyme → verified1.0.1


17 years ago
Group: security?


17 years ago
Keywords: adt1.0.1+, approval
Whiteboard: [ADT1 RTM] [06/25]


17 years ago
Whiteboard: ,adt1.0.1+, approval, [ADT1 RTM]


13 years ago
Flags: testcase+


12 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.