Closed Bug 134315 Opened 23 years ago Closed 22 years ago

Inserting code into about: using __proto__ redefinition

Categories

(Core :: Security, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.1beta

People

(Reporter: security-bugs, Assigned: jst)

Details

(Whiteboard: [HAVE FIX][ADT2 RTM])

Attachments

(3 files, 1 obsolete file)

It is possible to inject JS and html in the "about:" page. For now I don't have a working exploit for execution of code because in this URL priviliges seem dropped but it is possible to open "chrome:" URLs which may be used for further attacks. An attacker with same origin exploit may exploit this. Also this technique potentially may be used on other priviliged code. Once opened in the new context, certain "chrome:" URLs may pass sensitive info via "parent","top" or probably other ways. The attached file "protabout3.html" demonstrates several tricks. 1. redefinition of navigator.userAgent (despite it does not have setter) 2. redefinition of String.match() in another window 3. redeclaration of document.createTextNode() to document.writeln() so I can inject my HTML and JS in the "about:" window. Examine the attached file for details. Georgi Guninski
Attached file Example HTML (deleted) —
Note: I intentionally dropped privileges from most about: pages some months back, so this isn't directly exploitable, but it's still a potential hole and should be fixed.
Dropping privileges is certainly a good idea. But not all priviliges are dropped - "about:" still have enough privileges to link to or open in <iframe> chrome.
Can this be used with <about:config>? That has full priviledges.
about:config cannot be opened in a normal way AFAIK. But if someone finds a way to open it bad things may happen. I notice on Bugzilla 143853 that another people are hunting for bugs in __proto__ so I once again suggest ditching __proto__ on built in objects.
Why is the opening code allowed to access a.navigator anyway? Shouldn't getting that property be foiled by cross-origin checks?
shaver, the opening code modifies a.navigator in "about:blank" page. afaik playing with "about:blank" is allowed. part of the problem is that the change is persistent. the other part of the problem according to me is that playing with __proto__ should be disallowed on built in objects.
I think the whole problem is that the change is persistent, though I'm not sure why about:blank needs to be mutable in the first place (use document.open to get a new doc if you want to touch the content no?). I've explained at length why neutering __proto__ isn't going to happen, and 4.x permitted __proto__ mutation pretty much everywhere without associated security holes -- likely because it didn't have these imperfect-scope-clear/cross-origin-access bugs -- so I don't think it's necessary for us anyway.
afaik internet exploder does not support such fun with __proto__ so keeping it for web pages is not quite portable. recently there is an increased number of security bugs in mozilla with __proto__ and prototype and i expect even more in the future. it something like redefining constants. and sometimes chrome interacts with user supplied data.
Internet Explorer doesn't support __proto__ or __parent__, but then it doesn't ship JavaScript -- the JavaScript language has supported those things for a very long time, and even when we made setting __parent__ not work in the Mozilla embedding, nobody even suggested mutilating __proto__ or the reading of __parent__. It _does_ -- and must, to be an ECMA-262 implementation -- support the .prototype property of constructors and the like. Are you proposing to do away with that as well? _Every_ one of the bugs I've seen that (misguidedly) blames __proto__ or .prototype for some security hole has really been a manifestation of a hole in our same-origin checks or a failure to properly clean up before loading new content in a given script context. (Any code that trusts content or network data needs to take great care to constrain the scope of that trust. We don't expect <BASE HREF> to affect the security origin for our pages, we don't trust redirects blindly (anymore), etc.)
Shaver, IIRC I tested some things with IE's prototype and it didn't allow me to redefine existing methods/properties. I agree that in theory you may be right, but the facts are that __proto__ and protoype manifest themselves in several exploits which I believe will not be possible without using them. So I am proposing to: Not allow redefinition of existing built-in methods via __proto__ and prototype - i.e. in String, window, document and such. Also do not allow redefinition of existing properties via getters/setters __defineGetter__ ... is there any legitimate use of redefining of location.href via getters/setters? or redefining window.alert ? If userland javascript constructs a new object, it is ok to do with it whatever it wants, but not playing with existing (global) properties and methods.
I don't know what IE you're using, but my IE6 lets me override String.prototype.indexOf, just like the language specification requires. http://off.net/~shaver/String-prototype.html Let's fix the cross-origin access and scope clearing bugs that no other shipping browser seems to have, before we start ripping nearly-decade-old features out of a language that uses them, OK? I used to redefine window.alert and document.write all the time, via a debugging library, so that I could track things that went through them. And people used to overwrite various String.prototype things to fix bugs in shipping implementations, all the time. These features are used, as I've demonstrated in other bugs on this witch hunt. (Is there any legitimate reason to ask _untrusted_content_ for location.href, trust the result it gives you, and then make important decisions based on that information? I sure can't think of any.)
Shaver, Sorry for the misinformation - IE really allows playing with prototype as you demonstrate, I am wrong. Fully agree with you that this exploit should be fixed by stopping the persistency. I understand that my proposal breaks some ECMA Script stuff, but it is worth thinking about proposing to ECMA something like Java's "final" and stopping getters/setters for built in objects. Chrome uses window._content a lot: find . -name "*.js" | xargs grep window._content | wc -l 113 btw, as the Hacker's test ask, have you managed to redefine the value of 4 from userland javascript (without using valueOf() )? :)
In Beonex Communicator, I removed all JS from <about:>, but <about:plugins> is still there. I have no clue, how dangerous that bug really is or how I could word a warning. Any suggestions? Any workarounds (for users or for me)?
->jst
Assignee: mstoltz → jst
Keywords: nsbeta1
Whiteboard: [ADT2 RTM]
Keywords: nsbeta1nsbeta1+
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [ADT2 RTM] → [HAVE FIX][ADT2 RTM]
Target Milestone: --- → mozilla1.1beta
Comment on attachment 89155 [details] [diff] [review] Don't let classes leak into documents from about:blank except when document.write()'ing into the about:blank document. Nice and simple. r=mstoltz
Attachment #89155 - Flags: review+
True, but unfortunately it doesn't quite do the right thing, I just realized that this patch makes us not clear the scope
True, but unfortunately I just realized that it doesn't quite do the right thing, this patch makes us not clear the scope when calling document.open() on a document that's not about:blank. I.e. if I load yahoo.com and call document.open(); document.write("<script>...</script>"); my script can reach methods defined by yahoo.com. And that's probably a bad thing...
Hmm, yeah, document.write() and document.open() are allAccess, so this would let people steal data from any URL... not cool. We'll need to change this, new patch coming up...
Whoops, I missed that too. We should clear scope in all cases except document.open on an about:blank page only, right?
Comment on attachment 89478 [details] [diff] [review] Only let stuff leak into the document.write()'n document when writing into about:blank Looks good. r=mstoltz.
Attachment #89478 - Flags: review+
Comment on attachment 89478 [details] [diff] [review] Only let stuff leak into the document.write()'n document when writing into about:blank sr=brendan@mozilla.org
Attachment #89478 - Flags: superreview+
Fix checked in on the trunk, nominating for the branch.
Bindu, could you verify this fix on the trunk. Thanks.
Verified on 2002-07-09-08 trunk build on Win 2K. Above test case loads the about page and the about Netscape page with correct UserAgentString.
Looks like this change caused blocker bug http://bugscape.netscape.com/show_bug.cgi?id=17487
This fix was temporarily disabled since it caused bug http://bugscape.netscape.com/show_bug.cgi?id=17487
What is that bug about?
AIM problems...
Comment on attachment 90680 [details] [diff] [review] This is needed in addition to the above diff to not break AIM rpotts says sr=rpotts
Attachment #90680 - Flags: superreview+
Comment on attachment 90680 [details] [diff] [review] This is needed in addition to the above diff to not break AIM r=bz, and checked in on the trunk...
Attachment #90680 - Flags: review+
Resolved per jst comments and trunk check in. Bindu, can you verify this fix on the trunk. Thx.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Verified on 2002-07-12-trunk build on Win 2K. Above test case works fine. Asking scalkins to verify 17487 to make sure we have no AIM regressions.
Status: RESOLVED → VERIFIED
adt1.0.1- per the ADT. let's get it in the next release. should you disagree with the minus, pls remove the minus, leaving adt1.0.1 in the keywords, then send an email to Drivers' and ADT for approval.
Keywords: adt1.0.1adt1.0.1-
Is it possible that this patch caused bug 159424 (which breaks some oracle stuff)?
Please disclose this bug, it's fixed since 2 months.
The fix for this caused a regression (bug 159424) that are not fixed yet, let's wait for that to be fixed before opening this up, just in case.
long since fixed, removing security flag
Group: security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: