Closed Bug 149777 Opened 23 years ago Closed 23 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.
Attached patch patch (for branch) for review — — Splinter 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.
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: 23 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: