Closed Bug 202994 Opened 17 years ago Closed 17 years ago

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


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






(Reporter: jruderman, Assigned: jst)


(Whiteboard: [HAVE FIX])


(2 files, 2 obsolete files)

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).
This is fairly serious, as it could allow tracking of browsing in some
situations. Nominating for 1.4final blocker.
Severity: minor → major
Flags: blocking1.4?
Keywords: nsbeta1
Flags: blocking1.4? → blocking1.4+
jst's got a patch.

+    // 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 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.
Attachment #122635 - Attachment is obsolete: true
Attached patch This should do it. (obsolete) — Splinter Review
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

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

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?
Assignee: mstoltz → jst
Attachment #122639 - Flags: superreview?(brendan)
Attachment #122639 - Flags: review?(mstoltz)
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Whiteboard: [HAVE FIX]
Target Milestone: --- → mozilla1.4beta
Comment on attachment 122639 [details] [diff] [review]
This should do it.

Looks good to me. r=mstoltz.
Attachment #122639 - Flags: review?(mstoltz) → review+
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?

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.
Attachment #122639 - Attachment is obsolete: true
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!

Attachment #122797 - Flags: superreview+
Attachment #122797 - Flags: review+
Comment on attachment 122797 [details] [diff] [review]
Same as above, with OOM error propagation

Requesting approval to check in for 1.4final.
Attachment #122797 - Flags: approval1.4?
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
Attachment #122797 - Flags: approval1.4? → approval1.4+
Closed: 17 years ago
Resolution: --- → FIXED
So this caused regression bug 206026. The fix for that is to revert the
evaluation context change. See bug 206026 for more details.
Attachment #122639 - Flags: superreview?(brendan)
Group: security
You need to log in before you can comment on or make changes to this bug.