in CheckLoadURIWithPrincipal, we have special handling for nsExpandedPrincipal, in which we iterate over the whitelist, caling CheckLoadURIWithPrincipal on each one individually. But if none of those succeed, we return NS_OK (!!!) instead of a failure code. This means that the nsExpandedPrincipal can load whatever it wants. :-( I really should have caught this in review. I'm currently investigating why this wasn't caught by our test coverage.

This is a regression from bug 734891, patch 2, which landed in Fx16.

Probably better not to mark the dependency on bug 734891, which may be findable by inspection of the patch.

(In reply to Bobby Holley (:bholley) from comment #0) > This means that > the nsExpandedPrincipal can load whatever it wants. :-( Trying to estimate the risk here... what can they load? For XHR this check is only used for the scheme check (we don't have test coverage for that :( ), but XHR to another domain that is not on the list still fails. For accessing object we don't rely on this check. Can we load anything else? Is anyone currently using nsEP based XHR?

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3) > Can we load anything else? Is anyone currently using nsEP based XHR? This check isn't just for XHR. In this particular case, it allows content to load about: (a system-principal paged) in an iframe, which should normally throw as insecure.

(In reply to Bobby Holley (:bholley) from comment #4) > (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3) > > Can we load anything else? Is anyone currently using nsEP based XHR? > > This check isn't just for XHR. In this particular case, it allows content to > load about: (a system-principal paged) in an iframe, which should normally > throw as insecure. Sure, I get that this check is used in other cases too. But I don't understand your example, how could content code ever run with nsEP to do that?

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5) > Sure, I get that this check is used in other cases too. But I don't > understand your example, how could content code ever run with nsEP to do > that? If they combine it with other vulnerabilities, like bug 865947. See bug 863933.

Thanks, I looked over that one. wow... just, wow...

Can we get a security rating here? Tracking regardless for now, but the sec rating will be important for uplift considerations.

It's hard to say. I'm going to hazard a guess at sec-moderate on its own, but this is one of the pieces that make up bug 863933, which is sec-critical.

Comment on attachment 742719 [details] [diff] [review] Tests. v1 Review of attachment 742719 [details] [diff] [review]: ----------------------------------------------------------------- replace all 777777 -> 865948

Heh, sorry about the 777777 thing. I was on a plane and couldn't file a bug. :-)

Comment on attachment 742718 [details] [diff] [review] Return the appropriate value when the whitelist checks fail. v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 734891 User impact if declined: Potential security bugs, depending on who's using nsExpandedPrincipal. They only started being used outside of jetpack in FF21. Testing completed (on m-c, etc.): Just pushed to inbound, but the patch is very safe. Risk to taking this patch (and alternatives if risky): None. Not risky. String or IDL/UUID changes made by this patch: None

See comment 11 for the security ramifications of this beyond the immediate sec-moderate rating.

Comment on attachment 742718 [details] [diff] [review] Return the appropriate value when the whitelist checks fail. v1 Given this is sec-mod,I am going to untrack it for Fx21 and let the fix land in Fx22.Also this is a regression from Fx16, hence no rush to get it landed in our final two beta's .Approving for aurora and a- for beta. please renominate if there is a disagreement here.

Comment on attachment 742718 [details] [diff] [review] Return the appropriate value when the whitelist checks fail. v1 (In reply to bhavana bajaj [:bajaj] from comment #18) > Comment on attachment 742718 [details] [diff] [review] > Return the appropriate value when the whitelist checks fail. v1 > > Given this is sec-mod See comment 16. The security rating is kind of arbitrary here. > Also this is a regression from Fx16, hence no rush to get it landed > in our final two beta's. It's a regression from Fx16 in that Fx16 introduced the feature of nsExpandedPrincipal. But we only started using it in the platform in Fx21. I think we should take the patch. It's a one-liner that very clearly only takes effect in a situation that is, otherwise, a very obvious security bug.

Comment on attachment 742718 [details] [diff] [review] Return the appropriate value when the whitelist checks fail. v1 Reconsidered based on https://bugzilla.mozilla.org/show_bug.cgi?id=865948#c19 & because the patch is very safe . Approving on beta. please land by tomorrow noon PT.

https://hg.mozilla.org/releases/mozilla-aurora/rev/32fb800ed81d https://hg.mozilla.org/releases/mozilla-beta/rev/75e01dbd4b22

Comment on attachment 742718 [details] [diff] [review] Return the appropriate value when the whitelist checks fail. v1 This is out of scope for ESR17, being sec-moderate, approving for uplift to b2g18 though.

(In reply to lsblakk@mozilla.com from comment #22) > Comment on attachment 742718 [details] [diff] [review] > Return the appropriate value when the whitelist checks fail. v1 > > This is out of scope for ESR17, being sec-moderate, approving for uplift to > b2g18 though. Ok. We should definitely advisory-embargo this until esr17-EOL then. Al, is that the default behavior when we WONTFIX a sec bug on branch? This _could_ also imply embargoing bug 863933, which gets tricky...

We only mention where we fix in advisories, not where we won't fix. This is only a sec-moderate. Normally, as an internal find, this would go into the rollup advisory under the list of "things found by Mozilla people that we fixed in 21" with a link to a list of bugs in bugzilla (and no details). This wouldn't get its own advisory.

Al, this isn't actually an internal find, despite the fact that bholley filed the bug. It is actually one part of bug 863933, which was externally reported, so it seems to me it thus qualifies as externally reported. Personally, I think this is such a low risk fix, and part of a really awful exploit, so we should really just take it on ESR17. Maybe we could even wait a few weeks, or maybe a month (when it will be in release), to get a better sense if there are any regressions.

Ah, I didn't realize that this was an external report. Let's loop in release management.

(In reply to Andrew McCreight [:mccr8] from comment #26) > Personally, I think this is such a low risk fix, and part of a really > awful exploit, so we should really just take it on ESR17. Maybe we > could even wait a few weeks, or maybe a month (when it will be in > release), to get a better sense if there are any regressions. Re-nominating for ESR-17 based on the above.

This seems safe, and given the security team's preference here, we've approved for the next ESR17 release.

Landed tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c6f050126d7