Last Comment Bug 202994 - conversion to string after evaluation of javascript: URL is not subject to same-origin check
: conversion to string after evaluation of javascript: URL is not subject to sa...
Status: RESOLVED FIXED
[HAVE FIX]
:
Product: Core
Classification: Components
Component: Security: CAPS (show other bugs)
: Trunk
: All All
: P1 major (vote)
: mozilla1.4beta
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
: Charles Rosendahl
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-04-22 20:24 PDT by Jesse Ruderman
Modified: 2011-08-05 21:26 PDT (History)
5 users (show)
asa: blocking1.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase: reads the location of a third-party iframe after a redirect (473 bytes, text/html)
2003-04-22 20:25 PDT, Jesse Ruderman
no flags Details
Evaluate javascript: URL's on the caller's context if evaluated from JS. (7.33 KB, patch)
2003-05-06 17:29 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Review
This should do it. (13.85 KB, patch)
2003-05-06 19:36 PDT, Johnny Stenback (:jst, jst@mozilla.com)
security-bugs: review+
Details | Diff | Review
Same as above, with OOM error propagation (14.09 KB, patch)
2003-05-08 15:51 PDT, Johnny Stenback (:jst, jst@mozilla.com)
brendan: review+
brendan: superreview+
asa: approval1.4+
Details | Diff | Review

Description Jesse Ruderman 2003-04-22 20:24:27 PDT
After a javascript URL executes, if the value of the last statement executed is
not the value |undefined|, that value is converted to a string and treated as
the HTML for a new page.  The conversion to string is not subject to the
same-origin check, allowing a frame to see the URL of another frame (but nothing
else about the other frame).  Other implicit calls to toString, such as
String(foo) or ""+foo, are subject to this check.

This bug might also affect noAccess set for severalObjects.toString in mail
messages.  If it does (I haven't tested), this bug would allow a mail
message to determine its URL (I don't know if this would be a problem) and the
URLs of links in the message ("wiretap", bug 84545 and bug 66938).
Comment 1 Jesse Ruderman 2003-04-22 20:25:27 PDT
Created attachment 121378 [details]
testcase: reads the location of a third-party iframe after a redirect
Comment 2 Mitchell Stoltz (not reading bugmail) 2003-05-05 14:30:13 PDT
This is fairly serious, as it could allow tracking of browsing in some
situations. Nominating for 1.4final blocker.
Comment 3 Brendan Eich [:brendan] 2003-05-06 16:44:48 PDT
jst's got a patch.

/be
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2003-05-06 17:29:35 PDT
Created attachment 122635 [details] [diff] [review]
Evaluate javascript: URL's on the caller's context if evaluated from JS.
Comment 5 Jesse Ruderman 2003-05-06 17:47:21 PDT
+    // Grab a context to evaluate the javascript: URL on. If the
+    // evaluation of a javascript: URL is caused by some running
+    // script, use the context of the running script. If no JS is
+    // running, use the context of the window where the javascript:
+    // URL is being evaluated.

I don't understand this comment.  When would there be no JS running during the
evaluation of a javascript: URL?  What if the evaluation of the javascript: URL
was caused by a targeted link click rather than a script?
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2003-05-06 18:13:33 PDT
Comment on attachment 122635 [details] [diff] [review]
Evaluate javascript: URL's on the caller's context if evaluated from JS.

Hmm, yeah, this would only fix this hole when the src of an [i]frame is set
from JS, if it was done using a link with a target, we would see the same
problem, even with this fix.
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2003-05-06 19:36:04 PDT
Created attachment 122639 [details] [diff] [review]
This should do it.

In addition to the earlier patch there was a bug in
nsScriptSecurityManager::IsCapabilityEnabled() that assumed that if no
principals were found on the JS stack, all principals were enabled (i.e. we
were called from native code). That's not a safe assumption, that's only a safe
assumption if the subject principal is the system principal (or there is no
system principal).

So the problem in this case, be this a javascript: URL that is executed from JS
or not, is that the call to JS_ValueToString() in
nsJSEnvironment::EvaluateString() succeeds since caps thinks that the
capability "UniversalBrowserRead" is enabled (at the end of
nsScriptSecurityManager::CheckSameOriginDOMProp()).

This patch plugs that hole, as well as makes javascript: URL's execute on the
calling context (or the target's context if no caller), yet in the scope of the
target.

This patch, as well as the previous one, checks if there's a pending exception
after calling JS_ValueToString() in JSValueToAString() (a helper now in
nsJSEnvironment.cpp), I'm not sure if I need to check that before telling
XPConnect that an exception should be thrown. Is the fact that
JS_ValueToString() returned null enough?
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2003-05-06 19:37:10 PDT
Taking.
Comment 9 Mitchell Stoltz (not reading bugmail) 2003-05-07 17:25:36 PDT
Comment on attachment 122639 [details] [diff] [review]
This should do it.

Looks good to me. r=mstoltz.
Comment 10 Brendan Eich [:brendan] 2003-05-08 13:39:42 PDT
Comment on attachment 122639 [details] [diff] [review]
This should do it.

Null return from JS_ValueToString indeed means an error, and errors are
exceptions (except for "out of memory" -- that gets reported via the context's
error reporter immediately, no exception is created for it).

Do you need the nccx->SetExceptionWasThrown call and surrounding code from
JS_GetPendingException down?  Shouldn't the new helper JSValueToString return
false or failure, and that r.v. propagate?

/be
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2003-05-08 15:51:27 PDT
Created attachment 122797 [details] [diff] [review]
Same as above, with OOM error propagation

This is baiscally the same as the above, with the addition of distinguishing
between OOM and an exception being thrown in JS_ValueToString().

Brendan, yes, I do need the call to SetPendingException() on the current call
context, w/o that, XPConnect will go on as if no error occured. But I do not
want to throw an error to the XPCOM caller in this case, doing that makes a lot
of code freak out. Ideally, we could clean up this mess and do that, but I'm
not up for doing that at this point.
Comment 12 Brendan Eich [:brendan] 2003-05-09 15:44:56 PDT
Comment on attachment 122797 [details] [diff] [review]
Same as above, with OOM error propagation

carrying forward r=mstoltz, marking sr=me.

Nit: "as if the result *were* undefined" -- uphold the dying subjunctive mood!

/be
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2003-05-09 17:22:24 PDT
Comment on attachment 122797 [details] [diff] [review]
Same as above, with OOM error propagation

Requesting approval to check in for 1.4final.
Comment 14 Asa Dotzler [:asa] 2003-05-10 19:25:58 PDT
Comment on attachment 122797 [details] [diff] [review]
Same as above, with OOM error propagation

a=as (on behalf of drivers) for checkin to 1.4
Comment 15 Johnny Stenback (:jst, jst@mozilla.com) 2003-05-12 15:24:45 PDT
FIXED.
Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2003-05-20 18:35:23 PDT
So this caused regression bug 206026. The fix for that is to revert the
evaluation context change. See bug 206026 for more details.

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