Closed Bug 1088617 Opened 5 years ago Closed 5 years ago

NS_SecurityCompareURIs doesn't do the right thing for Blob URIs

Categories

(Core :: Security: CAPS, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(1 file)

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?
This mimics the logic already inside nsNullPrincipal::CheckMayLoad.
Attachment #8510995 - Flags: review?(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....
Attachment #8510995 - Flags: review?(bzbarsky) → review+
(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.
(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.
Ah, yes.  So looks like revoked blob: URIs have no useful principal.
https://hg.mozilla.org/mozilla-central/rev/df5359e8c82c
https://hg.mozilla.org/mozilla-central/rev/a347286d9669
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.