Last Comment Bug 149777 - Node cloned from external, untrusted document and appended to chrome document.
: Node cloned from external, untrusted document and appended to chrome document.
Status: VERIFIED FIXED
,adt1.0.1+, approval, [ADT1 RTM]
: topembed
Product: Core
Classification: Components
Component: Security: CAPS (show other bugs)
: Trunk
: All All
: P1 major (vote)
: mozilla1.0.1
Assigned To: Mitchell Stoltz (not reading bugmail)
: bsharma
Mentors:
http://lxr.mozilla.org/seamonkey/sour...
Depends on:
Blocks: 143047 149012
  Show dependency treegraph
 
Reported: 2002-06-06 17:00 PDT by John Morrison
Modified: 2007-04-01 14:17 PDT (History)
11 users (show)
bob: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (for branch) for review (2.83 KB, patch)
2002-06-19 19:48 PDT, John Morrison
bzbarsky: review+
jag-mozilla: superreview+
jud: approval+
Details | Diff | Review

Description John Morrison 2002-06-06 17:00:29 PDT
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 <db48x@yahoo.com> who I believe wrote most of pageInfo
Comment 1 Daniel Brooks [:db48x] 2002-06-07 13:47:56 PDT
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 
longer.
Comment 2 Mitchell Stoltz (not reading bugmail) 2002-06-10 12:03:37 PDT
This bug is exploitable, as in the example below from Georgi. That means we need
a fix ASAP.

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

<br>
Select <b>"Page Info | Media"</b>
</html>
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2002-06-11 08:06:01 PDT
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.
Comment 4 georgi - hopefully not receiving bugspam 2002-06-11 09:26:18 PDT
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)
Comment 5 Mitchell Stoltz (not reading bugmail) 2002-06-11 11:32:26 PDT
Can anyone suggest a whitelist of safe attributes to copy?
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2002-06-11 12:12:04 PDT
Seems like the page info dialog only needs the following attributes for images:

  src
  title
  alt
  longdesc
  width
  height

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?
Comment 7 Mitchell Stoltz (not reading bugmail) 2002-06-12 16:28:15 PDT
jrgm has volunteered to fix this. Thanks, John!
Comment 8 Daniel Brooks [:db48x] 2002-06-13 10:49:25 PDT
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/. 
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2002-06-13 10:51:27 PDT
Don't forget https
Comment 10 Daniel Brooks [:db48x] 2002-06-13 12:00:05 PDT
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.
Comment 11 Mitchell Stoltz (not reading bugmail) 2002-06-13 18:52:20 PDT
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.
Comment 12 Daniel Brooks [:db48x] 2002-06-14 02:05:33 PDT
I'd hardly call adding an extra border and an extra image node fancy, but whatever.
Comment 13 Mitchell Stoltz (not reading bugmail) 2002-06-14 10:38:15 PDT
I didn't mean don't do it, I just meant don't let it delay the fix.
Comment 14 John Morrison 2002-06-19 19:48:38 PDT
Created attachment 88397 [details] [diff] [review]
patch (for branch) for review

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

Changes:

1) elimate the use of cloneNode() entirely.
2) checks the absolute URL for a whitelist of "^(https?|ftp|file|gopher):"
   URLs.
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,
   but...).
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
   panel.

I'll do some more testing on this, but this is ready for review.
Comment 15 John Morrison 2002-06-19 19:55:53 PDT
I guess I also dropped the "weird funky hack" for 'item.href' cuz I didn't see
where it would have any affect.
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2002-06-20 15:57:54 PDT
Why are we dropping data: exactly?

Other than that, the patch looks good...
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2002-06-20 16:53:06 PDT
Because data: URL's can carry HTML data with scripts in them.
Comment 18 John Morrison 2002-06-20 16:58:59 PDT
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 John Morrison 2002-06-20 17:00:36 PDT
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 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2002-06-20 17:13:27 PDT
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
branch
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2002-06-20 17:14:09 PDT
JS will dtrt; all the stuff is in unicode by the time JS sees it.
Comment 22 jag (Peter Annema) 2002-06-20 17:40:48 PDT
Comment on attachment 88397 [details] [diff] [review]
patch (for branch) for review

sr=jag
Comment 23 John Morrison 2002-06-20 17:45:40 PDT
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?
Comment 24 Mitchell Stoltz (not reading bugmail) 2002-06-20 18:55:37 PDT
Looks good. I'll get it checked in.
Comment 25 Daniel Brooks [:db48x] 2002-06-20 20:01:42 PDT
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
Comment 26 Boris Zbarsky [:bz] (Out June 25-July 6) 2002-06-20 20:08:20 PDT
Ah, yes.  I missed the part about the weird funky hack.  That needs to stay in....
Comment 27 John Morrison 2002-06-20 20:29:52 PDT
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.
Comment 28 Daniel Brooks [:db48x] 2002-06-20 20:37:49 PDT
any idea on what's broken that it needs that?
Comment 29 Daniel Brooks [:db48x] 2002-06-20 20:40:36 PDT
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 John Morrison 2002-06-20 20:45:46 PDT
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.
Comment 31 Daniel Brooks [:db48x] 2002-06-20 20:50:33 PDT
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?
Comment 32 Daniel Brooks [:db48x] 2002-06-20 20:57:42 PDT
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 John Morrison 2002-06-20 22:55:14 PDT
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. 
EMBED uses 'SRC'. OBJECT uses 'ARCHIVE', 'CLASSID', 'CODE' or 'DATA'. 
APPLET uses 'CODE' or 'ARCHIVE'.

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

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.)
Comment 34 Mitchell Stoltz (not reading bugmail) 2002-06-21 17:59:20 PDT
Fixed on trunk. Needs verification and branch approval.
Comment 35 Jaime Rodriguez, Jr. 2002-06-22 18:45:29 PDT
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.
Comment 36 jag (Peter Annema) 2002-06-24 02:59:15 PDT
*** Bug 149012 has been marked as a duplicate of this bug. ***
Comment 37 bsharma 2002-06-24 12:28:52 PDT
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 John Morrison 2002-06-24 19:47:43 PDT
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 scottputterman 2002-06-24 23:04:55 PDT
adding adt1.0.1+.  Please get drivers approval before checking into the branch.
Comment 40 Judson Valeski 2002-06-25 12:18:38 PDT
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
Comment 41 Mitchell Stoltz (not reading bugmail) 2002-06-27 18:33:20 PDT
Fixed on trunk, adding fixed1.0.1.

Does embedding use the Page Info dialog? If not, then this should not be topembed.
Comment 42 jag (Peter Annema) 2002-06-28 13:09:37 PDT
You misspelled branch.
Comment 43 bsharma 2002-07-19 16:01:41 PDT
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).

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