1088617 - NS_SecurityCompareURIs doesn't do the right thing for Blob URIs

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Bobby Holley (:bholley)]

The basic problem in bug 1029889 is that we have an about: URI with a codebase principal (for the about:URI), which creates an Object URI and tries to load it. When we load it, we end up invoking NS_SecurityCompareURIs, which generally doesn't know how to handle things that aren't nsStandardURL.

I've got a patch that fixes the issue, though I'm not sure if there are other callsites with similar problems. I'm also not sure how to test this reliably without (a) introducing a new about:URI or (b) relying on an about:URI from Firefox whose security characteristics may change. Boris?

Comment 1

[Bobby Holley (:bholley)]

Attachment: Handle nsIURIWithPrincipal in nsPrincipal::CheckMayLoad. v1

This mimics the logic already inside nsNullPrincipal::CheckMayLoad.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8510995 [details] [diff] [review]
Handle nsIURIWithPrincipal in nsPrincipal::CheckMayLoad. v1

>+  nsCOMPtr<nsIPrincipal> uriPrin;

Why not move this inside the "uriWithPrin" condition?  Can we actually have an nsIURIWithPrincipal with a null GetPrincipal?

>+  if (uriPrin && static_cast<nsIPrincipal*>(this)->Subsumes(uriPrin)) {

Is the static_cast needed because we're shadowing the nsIPrincipal thing in nsPrincipal?  If so, I would prefer "using nsIPrincipal::Subsumes;" in the nsPrincipal header and dropping the cast here.

r=me with that fixed.

As far as other places like this ... Looking at the callsites of SecurityCompareURIs I see:

1) nsScriptSecurityManager::CheckSameOrigin (is it still used? can we kill it?).   If this is still needed, it could use similar treatment, or just being rewritten in terms of CheckMayLoad or something perhaps.

2) nsScriptSecurityManager::CheckSameOriginURI, which is just what it is.  

3) EqualOrSubdomain which is not working with principals at all.

4) CheckLoadURIWithPrincipal, in the URI_CROSS_ORIGIN_NEEDS_WEBAPPS_PERM case.  But we won't reach that in cases when we have an nsIURIWithPrincipal, afaict, since URI_LOADABLE_BY_SUBSUMERS will be set and we'll delegate to CheckMayLoad.

I think that's it.  So maybe a followup to sort out nsScriptSecurityManager::CheckSameOrigin?

As far as testing goes, I don't have any bright ideas.  Fundamentally you need a case where a load provides a codebase principal itself for a non-hierarchical URI, since that won't normally get a codebase principal....

Comment 3

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> 1) nsScriptSecurityManager::CheckSameOrigin (is it still used? can we kill
> it?).   If this is still needed, it could use similar treatment, or just
> being rewritten in terms of CheckMayLoad or something perhaps.

Appears to be unused. I'll add a patch to kill it.

> As far as testing goes, I don't have any bright ideas.  Fundamentally you
> need a case where a load provides a codebase principal itself for a
> non-hierarchical URI, since that won't normally get a codebase principal....

Ok. I'm busy with other stuff, so I'm just going to push this and move on. Bug 1029889 will hopefully serve as test coverage here.

Comment 4

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2dbd41ba7717
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/262df50c7355

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Backed out for crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d94624ae1684

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3363132&repo=mozilla-inbound

Comment 6

[Bobby Holley (:bholley)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #5)
> Backed out for crashes.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d94624ae1684
> 
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=3363132&repo=mozilla-inbound

That answers your question in comment 2, bz.

Comment 7

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=882a9c988033

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  So looks like revoked blob: URIs have no useful principal.

Comment 9

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/df5359e8c82c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a347286d9669

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df5359e8c82c
https://hg.mozilla.org/mozilla-central/rev/a347286d9669