User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13 Build Identifier: <script> window.SyntaxError.prototype.__defineGetter__('name',function(){var e=this;for(var j in e){if(j!='name')console.log(j,e[j])}}); </script> <script src="http://google.com/"></script> Reproducible: Always Steps to Reproduce: 1. execute the afore mentioned code 2. see the console 3. see that fileName includes the final URL of the redirection Actual Results: fileName leaks Expected Results: fileName shouldn't leak, or should say the URL in the <script src="..." attribute

<script>ReferenceError.prototype.__defineGetter__('name', function(){ e=this;for(var j in e){if(j!='name')alert(j+':'+e[j])} });</script> <script src="http://jsbin.com/ujoho6/2"></script> that code will also leak the contents of the remote site.

Acknowledgement should go to Daniel Divricean.

Opera, Safari, Chrome are vulnerable. IE is vulnerable as well via window.onerror.

Opera is tracking this as DSK-325495

Johnny, you might know our error handling code well enough to know what's going on here?

What's going on is that nsScriptLoader::EvaluateScript passes aRequest->mFinalURI as the script filename when evaluating the script. And JS exceptions include the filename of the script in the data they expose. So that means that as long as you can get hold of exceptions from the script you know what URI the script was loaded from. We've closed this hole for window.onerror by sanitizing the information the onerror handler gets when the exception is thrown from an off-site script. But the code in comment 0 or comment 1 gets hold of exceptions by overriding the "name" getter on the exception's prototype object. Note that we're using mFinalURI here due to the fix for bug 461735. The fundamental issue in that bug is that we need to know that the script was cross-origin to avoid giving too much information to the onerror handler. Fundamentally, as long as we expose the script filename to the web site at all, and only have the single field to work with for both the "filename" and the concept of "where did this script come from", this is a no-win situation. If I had my way, then in the redirect case, we should be using the pre-redirect URI as the filename for reporting purposes, and the post-redirect URI for any security checks... but JSScript just doesn't let us store both pieces of information. As far as why the code in comment 0 and comment 1 works at all, the reason the 'name' getter is called is that js_ReportUncaughtException calls js_ValueToString on the exception, which lands us in exn_toString, which gets the "name" and "message" properties off the object in a way that lands us in the getter defined by the page JS above. We could try to fix this, which would at least help pages that don't examine the exceptions themselves... but I think that would still be a band-aid; the real fix is to store both "filenames" in the script and use the right one for the right purpose.

mrbkap, this is exactly what I was worried about when we allowed the error message of exceptions to cross different origin compartments. I still think we should just clamp those to a generic error.

There is only one compartment here, no?

Ah right. I didn't read the testcase right. We throw in our own compartment.

Being able to extract arbitrary data across domains seems like sg:high not moderate per https://wiki.mozilla.org/Security_Severity_Ratings?

This is not extracting arbitrary data. It's extracting the URI the script was redirected to. Still a problem, but somewhat limited.

It is extracting arbitrary data across domains. Comment 1: <script>ReferenceError.prototype.__defineGetter__('name', function(){ e=this;for(var j in e){if(j!='name')alert(j+':'+e[j])} });</script> <script src="http://jsbin.com/ujoho6/2"></script> Extracts: "asdf" from "name" in ReferenceError.

That can extract the first bareword in the linked-to file, right?

Again, I'm not saying this is not a serious problem. I'm saying that this lets you extract _some_ information from other sites, but not arbitrary information...

yes that's accurate, and not only the first bareword, since in some cases not only that (eg. if it starts with a 0, or contains a - etc..) will fails with a syntax error. This, however was enough to create a security vulnerability in Google.. (that here, Daniel reported to us). So yes, sorry, it's not arbitrary data, it's.. some what limited data.

s/fails/fail/ spelling.fail()

Marking non-blocking because it's sg:crit and the leakage seems fairly limited. Correct me if I'm wrong.

Hi all! Sorry if this is the wrong place to have this discussion. After reading the comments of this bug, I was hoping I could get the security team's point of view regarding if redirects shouldn't be used to pass security sensitive tokens anymore? Because plugins and browsers have vulnerabilities that leak redirects more frequent that other vulnerabilities, such as uxss, or at least that's my impression, also it's worth noting that in Mozilla it seems like reading http redirects is considered medium risk, vs. uxss that is considered high risk, while in some web apps, reading redirects almost represents the same risk. I can't give a lot of details, but several popular and sensitive websites (as well as some open authentication and identification protocols) are affected by this type of bugs in a way that it creates very high risk vulnerabilities, and we feel bad for not being able to fix them ourselves (though, I've been studying Spidermonkey/Gecko2 code, so hopefully one of this days I'll be able to contribute with something more than just bugs). Anyways, just some thoughts :) since I am considering to start filing bugs internally in Google for features that set sensitive tokens in URL redirects because I just feel like they are vulnerable.

Luke offered to help drive this and discuss with Boris what we want to do here. Over to him for now.

I talked to Boris and his non-bandaid solution in comment 6 makes sense to me. From the JS side, we'd just add an extra "origin principals" field to scripts that DOM sets to mFinalURI. This would let the DOM set script->filename to the pre-redirect URI (fixing the hole) and use the "origin principals" for making onerror security judgments. We also discussed how this might work with zero JS engine additions (keeping an external hash table) but this is gross in two ways: dealing with script lifetimes and transitively setting the origin principals for nested scripts.

This adds an API to attach originPrincipals to a script. See comment in jsapi.h. Note: scripts created via Function/eval will get the calling script's originPrincipals (for both direct and indirect eval). Does that sound right?

Comment on attachment 542517 [details] [diff] [review] add JS engine API for originPrincipals It's not clear to me what enforces the equality js_XDRScript asserts. jsscript.h should document the "principals implies originprincipals" invariant. Other than that, this looks good, I think. I didn't review the nitty-gritty details, obviously.

(In reply to comment #22) > It's not clear to me what enforces the equality js_XDRScript asserts. The informal reasoning I passed by mrbkap was: we don't XDR content which should be the only place we have differing originPrincipals. My worry was 2x calls to the principal transcoder would be bad for startup XDR time.

> we don't XDR content which should be the only place we have differing > originPrincipals. There's nothing preventing extensions from loading scripts over https://, in which case the origin principal will be https:// while the principal is system. Can we instead make XDR smart about the (common) case where the two principals are pointer-equal and only serialize once in that case? That seems like a better approach.

(In reply to comment #24) > Can we instead make XDR smart about the (common) case where the two > principals are pointer-equal and only serialize once in that case? Yeah, that's what I was doing until I thought I had found a simplification...

Luke, ping?

Oops, this dropped off my radar. Yes, I'll put up a new patch. This just adds an API so that you can write a patch that actually fixes the bug so, in the meantime, perhaps you could write said patch and see if this actually works?

This patch supports script->principals != script->originPrincipals.

err, I meant to say "XDR'ing scripts where script->principals != script->originPrincipals".

(In reply to Luke Wagner [:luke] from comment #28) > Created attachment 562555 [details] [diff] [review] [diff] [details] [review] > add JS engine API for originPrincipals (v2) Hm, why not just add a hook that allows to sanitize the information that goes into error instances that the compiler creates? Also why cannot we make JSScript::filename different from the URL stored in JSScript::principals so JSScript::filename would point to the name as used in the script tag?

> Hm, why not just add a hook that allows to sanitize the information that goes into error > instances that the compiler creates? Sanitize based on what? > Also why cannot we make JSScript::filename different from the URL stored in > JSScript::principals so JSScript::filename would point to the name as used in the script > tag? See comment 6, which discusses the reason at length, with links to the relevant bugs.

Rebased. bz: IIRC, I try server'd this with a big other stack of patches and there was a hang on one mochitest, so there is apparently a bug I need to flush out.

Hmm. So there's still a jsapi bit missing here from my point of view. What I have to work with is a JSErrorReport (well, and JSContext and a const char* message), not a JSScript. Can we propagate the origin principal to the JSErrorReport?

Yes, this can be done; PopulateReportBlame seems like the only place that initializes JSErrorReport::filename, so it can just fill in the origin principals as well. The scary part, of course is ref-counting the principals correctly. JSErrorReport has no destructor so I'll need to track down all the places we free it and make them call a destructor that can drop the ref. Blech.

JSErrorReport: now with origin principals.

The last patch had a nice crash (arguments shadow members with the same dontcha know). This patch is looking green on try.

I suck. I finally got to looking at this, and of course it doesn't apply on tip anymore... Luke, do you recall what changeset you based this on or do you have a rebased version to hand?

>+#if JS_BITS_PER_WORD == 32 >+ private: >+ void *padding_; >+#endif This makes everything after that point private in 32-bit builds, which doesn't so much compile. Why is that even needed?

Comment on attachment 579908 [details] [diff] [review] add JS engine API for originPrincipals (v4, rebased on 2087537890f0) Looks good to me. With this, I can totally fix the bug.

(In reply to Boris Zbarsky (:bz) from comment #39) > This makes everything after that point private in 32-bit builds, which > doesn't so much compile. Why is that even needed? Oops, I accidentally added the missing 'public:' in a down patch. The padding is needed b/c JSScript is a GC-thing and its size must be a multiple of sizeof(Value).

Comment on attachment 579908 [details] [diff] [review] add JS engine API for originPrincipals (v4, rebased on 2087537890f0) Review of attachment 579908 [details] [diff] [review]: ----------------------------------------------------------------- It is hard to imagine the amount of extra code we carry thanks to redirects. ::: js/src/frontend/TokenStream.h @@ +842,5 @@ > JSPackedBool maybeStrSpecial[256];/* speeds up string scanning */ > JSVersion version; /* (i.e. to identify keywords) */ > bool xml; /* see JSOPTION_XML */ > + JSContext * const cx; > + JSPrincipals * const originPrincipals; What's with the crazy spaces after the *s? ::: js/src/jscntxt.cpp @@ +761,5 @@ > + /* > + * Depend on the caller to HOLD originPrincipals if the report > + * outlives the stack frame. > + */ > + report->originPrincipals = iter.fp()->script()->originPrincipals; When would a report outlive the stack frame? As far as I can tell, all callers of this function pass a stack-local address here. ::: js/src/jsscript.h @@ +560,5 @@ > js::types::TypeScript *types; > > +#if JS_BITS_PER_WORD == 32 > + private: > + void *padding_; Still need a public: here, right?

(In reply to Blake Kaplan (:mrbkap) from comment #44) > > + * Depend on the caller to HOLD originPrincipals if the report > > + * outlives the stack frame. > > + */ > > + report->originPrincipals = iter.fp()->script()->originPrincipals; > > When would a report outlive the stack frame? As far as I can tell, all > callers of this function pass a stack-local address here. True, but *report may be copied into a new malloc()'d JSErrorReport which is stored in an exception object. But you're right that the comment is overly general; perhaps it should just say "Callers only pass stack-local JSErrorReport objects.".

Any reason I should wait before landing the JS engine patch?

Imo, no. Just give it a better summary and all? ;) Blake, do you want to just review the content patches too?

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab1bb6660d5c Note to sheriff: there are more patches to land.

This was merged into m-c today: https://hg.mozilla.org/mozilla-central/rev/ab1bb6660d5c

I believe only part 1 was checked in (see comment 48), the other two are still waiting on reviews.

Pushed the DOM changes: https://hg.mozilla.org/integration/mozilla-inbound/rev/18332142caa5 https://hg.mozilla.org/integration/mozilla-inbound/rev/c68ddc33f098

Backed out: http://hg.mozilla.org/integration/mozilla-inbound/rev/19062b47ae9f Back out c68ddc33f098 and 18332142caa5 (bug 624621) for jsreftest failures

There was, in particular, one jsreftest failure: REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_5/Regress/regress-328897.js | JS_ReportPendingException should Expected match to '/(Script error.|uncaught exception: Permission denied to get property UnnamedClass.classes)/', Actual value 'Permission denied for to get property XPCComponents.classes' item 1 Looks to me like the test assertion is just assuming this bug and needs fixing. I'll do that and push this again.

https://hg.mozilla.org/integration/mozilla-inbound/rev/98617f7b667b https://hg.mozilla.org/integration/mozilla-inbound/rev/316d6a49a603

Oh, bah. That fails too, because there's a missing space....

Oh, its not a missing space. It's actually the tbpl log parser thing just lying straight out, looks like.

In particular, the actual value in the log is: Permission denied for <file://> to get property XPCComponents.classes (compare to the output from the log parser in comment 53)

Ah, in the popup summary *after* a failure has been starred? Yeah, I'll take care of that, I remember the bug where the *before* it was starred case was fixed.

> Ah, in the popup summary *after* a failure has been starred? Yeah. Didn't realize that's different from the before case!

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c7b395bce29 https://hg.mozilla.org/integration/mozilla-inbound/rev/3a24ad267c77 now after tryservering it to make sure J is green.

Did you check Android? :) Actual value 'Permission denied for <http://10.250.48.218:30168> to get property XPCComponents.classes'

Marked as fails-if(browserIsRemote) in https://hg.mozilla.org/integration/mozilla-inbound/rev/6cff9824c2c1

> Did you check Android? :) Does try run that test on Android? Apparently not! See https://tbpl.mozilla.org/?tree=Try&rev=469f98207b4c while it lasts!

bc, what's a reasonable test assertion here that would actually work on the remote browser setup?

Bug 664857 invites you to actually enjoy and be entirely satisfied with the fact that "-u jsreftest" is broken, and that you must always say "-u jsreftest,jsreftest-1,jsreftest-2,jsreftest-3" until you know which hunk of Android you need, and the same with crashtests and reftests. https://hg.mozilla.org/mozilla-central/rev/2c7b395bce29 https://hg.mozilla.org/mozilla-central/rev/3a24ad267c77 https://hg.mozilla.org/mozilla-central/rev/6cff9824c2c1

The current version is: expect = /(Script error.|Permission denied for <file:\/\/> to get property XPCComponents.classes)/; but bug 570807 comment 2 failed trying to match: Permission denied for <http://10.250.48.217:30139> to get property XPCComponents.classes I think this would work in both the file based reftests and the web based android tests: expect = /(Script error.|Permission denied for <[^>]+> to get property XPCComponents.classes)/; jmaher: you agree?

changing this to be a more general regex sounds like a good idea. For all android automation we do http instead of file.

> and that you must always say "-u jsreftest,jsreftest-1,jsreftest-2,jsreftest-3" Ah, I didn't check enough on the try chooser. OK, then.

FIXED by the landings in comment 65 sirdarckcat: what is the state of other browsers? should this issue remain embargoed? On the current trajectory the fix won't be available until Firefox 12 (late April) though the fix would be safe enough to land in Aurora (Firefox 11 in mid-March).

(In reply to Daniel Veditz from comment #69) > FIXED by the landings in comment 65 > > sirdarckcat: what is the state of other browsers? should this issue remain > embargoed? On the current trajectory the fix won't be available until > Firefox 12 (late April) though the fix would be safe enough to land in > Aurora (Firefox 11 in mid-March). Does this still need to be considered for landing on Beta 11?

[Triage Comment] Please nominate for approval-mozilla-esr10 if this is ready to land on that branch. See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for details.

Just got an auto-bug email: have we decided to land this on ESR? The patches aren't exactly small and I thought this bug goes back a long time, so I wasn't sure that the risk/reward ratio was in favor, but I could be wrong.

(In reply to Luke Wagner [:luke] from comment #73) > Just got an auto-bug email: have we decided to land this on ESR? The > patches aren't exactly small and I thought this bug goes back a long time, > so I wasn't sure that the risk/reward ratio was in favor, but I could be > wrong. In bug 714614 comment 24, akeybl said "We're committed to taking all sg:crit bugs that affect FF10 on the ESR. We should move forward with a new fix." So if you don't think this bug should be fixed on ESR, I think you need to make the case for an exception.

(In reply to David Mandelin from comment #74) > So if you don't think this bug should be fixed on ESR, I think you need to make > the case for an exception. I did: poor risk/reward ratio.

lowering severity to moderate for parity with chrome's SecSeverity-Medium http://code.google.com/p/chromium/issues/detail?id=69187#c63

Is this still embargoed for public disclosure? In other words, has everyone else fixed this yet? I see that Chromium fixed this last year. Any word on Opera or IE?

With the low ratio of reward to risk here, we'll wontfix and see this on esr17

When I evaluate the following in the error console: document.location="data:text/html,<script>window.SyntaxError.prototype.__defineGetter__('name',function(){var e=this;for(var j in e){if(j!='name')console.log(j,e[j])}});</script><script src='http://google.com/'></script>" I get "Source File: http://www.google.com" in the error for both 11 and nightly. The error is "Error: SyntaxError: syntax error" in nightly and just "Error: Syntax Error" in 11. Did we really fix this? If so, how do I verify the fix correctly? I spoke to Dan Veditz about this and he didn't get any further than me.

> When I evaluate the following in the error console: The error console runs with chrome privileges. The right way to test this bug is to put the relevant code into an untrusted webpage and see what gets logged.

After much discussion with Boris and Dan Veditz, this was verified fixed by me on nightly trunk builds.