Closed
Bug 134315
Opened 23 years ago
Closed 22 years ago
Inserting code into about: using __proto__ redefinition
Categories
(Core :: Security, defect, P1)
Core
Security
Tracking
()
VERIFIED
FIXED
mozilla1.1beta
People
(Reporter: security-bugs, Assigned: jst)
Details
(Whiteboard: [HAVE FIX][ADT2 RTM])
Attachments
(3 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
security-bugs
:
review+
brendan
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•23 years ago
|
||
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.
Comment 2•23 years ago
|
||
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.
Comment 3•23 years ago
|
||
Can this be used with <about:config>? That has full priviledges.
Comment 4•23 years ago
|
||
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?
Comment 6•23 years ago
|
||
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.
Comment 8•23 years ago
|
||
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.)
Comment 10•23 years ago
|
||
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.)
Comment 12•23 years ago
|
||
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() )? :)
Comment 13•23 years ago
|
||
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)?
Assignee | ||
Comment 15•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [ADT2 RTM] → [HAVE FIX][ADT2 RTM]
Target Milestone: --- → mozilla1.1beta
Reporter | ||
Comment 16•22 years ago
|
||
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+
Assignee | ||
Comment 17•22 years ago
|
||
True, but unfortunately it doesn't quite do the right thing, I just realized
that this patch makes us not clear the scope
Assignee | ||
Comment 18•22 years ago
|
||
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...
Assignee | ||
Comment 19•22 years ago
|
||
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...
Reporter | ||
Comment 20•22 years ago
|
||
Whoops, I missed that too. We should clear scope in all cases except
document.open on an about:blank page only, right?
Assignee | ||
Comment 21•22 years ago
|
||
Attachment #89155 -
Attachment is obsolete: true
Reporter | ||
Comment 22•22 years ago
|
||
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 23•22 years ago
|
||
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+
Assignee | ||
Comment 24•22 years ago
|
||
Fix checked in on the trunk, nominating for the branch.
Keywords: adt1.0.1,
mozilla1.0.1
Comment 25•22 years ago
|
||
Bindu, could you verify this fix on the trunk. Thanks.
Comment 26•22 years ago
|
||
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.
Assignee | ||
Comment 27•22 years ago
|
||
Looks like this change caused blocker bug
http://bugscape.netscape.com/show_bug.cgi?id=17487
Assignee | ||
Comment 28•22 years ago
|
||
This fix was temporarily disabled since it caused bug
http://bugscape.netscape.com/show_bug.cgi?id=17487
Comment 29•22 years ago
|
||
What is that bug about?
Assignee | ||
Comment 30•22 years ago
|
||
AIM problems...
Assignee | ||
Comment 31•22 years ago
|
||
Assignee | ||
Comment 32•22 years ago
|
||
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+
Assignee | ||
Comment 33•22 years ago
|
||
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+
Comment 34•22 years ago
|
||
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
Comment 35•22 years ago
|
||
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
Comment 36•22 years ago
|
||
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.
Comment 37•22 years ago
|
||
Is it possible that this patch caused bug 159424 (which breaks some oracle stuff)?
Comment 38•22 years ago
|
||
Please disclose this bug, it's fixed since 2 months.
Assignee | ||
Comment 39•22 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•