The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.4beta

Status

()

Core
Security: CAPS
P1
major
RESOLVED FIXED
14 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: jst)

Tracking

Trunk
mozilla1.4beta
Points:
---
Bug Flags:
blocking1.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [HAVE FIX])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

14 years ago
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).
(Reporter)

Comment 1

14 years ago
Created attachment 121378 [details]
testcase: reads the location of a third-party iframe after a redirect
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

Updated

14 years ago
Flags: blocking1.4? → blocking1.4+
jst's got a patch.

/be
(Assignee)

Comment 4

14 years ago
Created attachment 122635 [details] [diff] [review]
Evaluate javascript: URL's on the caller's context if evaluated from JS.
(Reporter)

Comment 5

14 years ago
+    // 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?
(Assignee)

Comment 6

14 years ago
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
(Assignee)

Comment 7

14 years ago
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?
(Assignee)

Comment 8

14 years ago
Taking.
Assignee: mstoltz → jst
(Assignee)

Updated

14 years ago
Attachment #122639 - Flags: superreview?(brendan)
Attachment #122639 - Flags: review?(mstoltz)
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
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?

/be
(Assignee)

Comment 11

14 years ago
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.
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!

/be
Attachment #122797 - Flags: superreview+
Attachment #122797 - Flags: review+
(Assignee)

Comment 13

14 years ago
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 14

14 years ago
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+
(Assignee)

Comment 15

14 years ago
FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

14 years ago
So this caused regression bug 206026. The fix for that is to revert the
evaluation context change. See bug 206026 for more details.
(Assignee)

Updated

14 years ago
Attachment #122639 - Flags: superreview?(brendan)
(Reporter)

Updated

14 years ago
Group: security
You need to log in before you can comment on or make changes to this bug.