202994 - conversion to string after evaluation of javascript: URL is not subject to same-origin check

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

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]

Attachment: testcase: reads the location of a third-party iframe after a redirect

Comment 2

[Mitchell Stoltz (not reading bugmail)]

This is fairly serious, as it could allow tracking of browsing in some
situations. Nominating for 1.4final blocker.

Comment 3

[Brendan Eich [:brendan]]

jst's got a patch.

/be

Comment 4

[Johnny Stenback (:jst)]

Attachment: Evaluate javascript: URL's on the caller's context if evaluated from JS.

Comment 5

[Jesse Ruderman]

+    // 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)]

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)]

Attachment: 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)]

Taking.

Comment 9

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 122639 [details] [diff] [review]
This should do it.

Looks good to me. r=mstoltz.

Comment 10

[Brendan Eich [:brendan]]

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)]

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

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)]

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

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)]

FIXED.

Comment 16

[Johnny Stenback (:jst)]

So this caused regression bug 206026. The fix for that is to revert the
evaluation context change. See bug 206026 for more details.