865948 - (CVE-2013-1703) nsScriptSecurityManager::CheckLoadURIWithPrincipal is broken for nsExpandedPrincipal

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Andrew McCreight [:mccr8]]

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

Comment 2

[Andrew McCreight [:mccr8]]

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

Comment 3

[Gabor Krizsanits (INACTIVE)]

(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?

Comment 4

[Bobby Holley (:bholley)]

(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.

Comment 5

[Gabor Krizsanits (INACTIVE)]

(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?

Comment 6

[Bobby Holley (:bholley)]

(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.

Comment 7

[Gabor Krizsanits (INACTIVE)]

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

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 9

[Bobby Holley (:bholley)]

Attachment: Return the appropriate value when the whitelist checks fail. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Comment 11

[Bobby Holley (:bholley)]

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 12

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 742719 [details] [diff] [review]
Tests. v1

Review of attachment 742719 [details] [diff] [review]:
-----------------------------------------------------------------

replace all 777777 -> 865948

Comment 13

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf9e342c1e7c

Comment 14

[Bobby Holley (:bholley)]

Attachment: Tests. v2 r=gabor

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

Comment 15

[Bobby Holley (:bholley)]

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

Comment 16

[Andrew McCreight [:mccr8]]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cf9e342c1e7c

Comment 18

[bhavana bajaj [:bajaj]]

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 19

[Bobby Holley (:bholley)]

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 20

[bhavana bajaj [:bajaj]]

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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e74a88ada013

Comment 24

[Bobby Holley (:bholley)]

(In reply to [e-mail] 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...

Comment 25

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 26

[Andrew McCreight [:mccr8]]

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.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

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

Comment 30

[Daniel Veditz [:dveditz]]

(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.

Comment 31

[Alex Keybl [:akeybl]]

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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/a857a443defb

Comment 34

[Bobby Holley (:bholley)]

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

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1c6f050126d7