Closed Bug 149777 Opened 22 years ago Closed 22 years ago

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

Categories

(Core :: Security: CAPS, defect, P1)

Product:
Core
Component:
Security: CAPS
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0.1

People

(Reporter: jrgmorrison, Assigned: security-bugs)

References

(
URL
)

Details

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

Attachments

(1 file)

patch (for branch) for review
2.83 KB, patch
bzbarsky
: review+
jag+mozilla
: superreview+
jud
: approval+
Details | Diff | Splinter Review 
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
Group: security?
Status: NEW → ASSIGNED
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.
Assignee: mstoltz → db48x
Severity: normal → major
Status: ASSIGNED → NEW
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.

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

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

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

sr=jag
Attachment #88397 - Flags: superreview+
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.
Status: NEW → ASSIGNED
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....
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.
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?
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.)
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
Status: ASSIGNED → RESOLVED
Closed: 22 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. ***
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.
Status: RESOLVED → VERIFIED
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.
adding adt1.0.1+.  Please get drivers approval before checking into the branch.
Keywords: adt1.0.1 → adt1.0.1+
Attachment #88397 - Flags: approval+
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.
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
Group: security?
Keywords: adt1.0.1+, approval
Whiteboard: [ADT1 RTM] [06/25]
Whiteboard: ,adt1.0.1+, approval, [ADT1 RTM]
Flags: testcase+
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: