Closed Bug 1270679 Opened 4 years ago Closed 4 years ago

Ensure blob URLs are only accessible within the same usercontextId

Categories

(Core :: DOM: Security, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: tanvi, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Whiteboard: [OA][domsecurity-backlog][userContextId])

Attachments

(1 file, 2 obsolete files)

Check if blob urls can be accessed across contexts.

+++ This bug was initially created as a clone of Bug #1264556 +++
Baku, can you take a look at the code for this one and provide a link to it?
Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attached patch blob.patch (obsolete) — Splinter Review
Attachment #8749617 - Flags: review?(bzbarsky)
Comment on attachment 8749617 [details] [diff] [review]
blob.patch

> NS_IMETHODIMP
>-nsHostObjectProtocolHandler::NewChannel(nsIURI* uri, nsIChannel* *result)
>+nsHostObjectProtocolHandler::NewChannel(nsIURI* uri, nsIChannel** result)
> {
>-  return NewChannel2(uri, nullptr, result);
>+  // We don't implement this method because we want to force the use of
>+  // NewChannel2 in order to execute security checks based on OriginAttributes.
>+  return NS_ERROR_NOT_IMPLEMENTED;
> }
> 

Also r? Christoph for this change.  This may break addons that call NewChannel().

baku, can you elaborate on the behavior of blob urls without this patch.  Thanks!
Attachment #8749617 - Flags: review?(ckerschb)
Comment on attachment 8749617 [details] [diff] [review]
blob.patch

This patch needs a commit message describing exactly what the goal is.  r- just based on that, because I can't tell whether the patch is correct (e.g., why is it checking loadingPrincipal and not triggeringPrincipal).

>+// Here we want to test that a new opened window shows the same UI of the
>+// parent one if this has been loaded from a particular container.

This comment is not making sense to me.  That's not what this test is testing!

>+    return new Promise(resolve => {
>+      resolve(content.window.URL.createObjectURL(new content.window.Blob([123])));
>+    });

Is this different from:

  return Promise.resolve(content.window.URL.createObjectURL(new content.window.Blob([123])));

in some substantive way that requires the extra complexity?  If so, document.  If not, do the simpler thing.

>+  }).then(blobUrl => { blobURL = blobUrl });

How about:

  }).then(newURL => { blobURL = newURL });

to be a bit less confusing?

>+    is(status, "OK", "It works");

How about we make the statuses consistent in the two tests.  resolve("OpenSucceeded") if open() does not throw, resolve("OpenThrew") if it did.  Then your is() calls will make it clear what you were expecting to happen, exactly.  And instead of "It works", say "Using a blob URI from one user context id in another should not work" and "Using a blob URI within a single user context id should work" respectively or something.

The actual code changes look fine, except for me not being able to tell whether they do what we want.
Attachment #8749617 - Flags: review?(bzbarsky) → review-
> baku, can you elaborate on the behavior of blob urls without this patch. 
> Thanks!

With the current setup, when a blob URL is created, it is available for any other page who knows that unique blob URL.
This is a security issue because it can be used to track pages across containers (and we also have a similar issue for private browsing).

With this patch, I want to check that the originAttributes of the loading principal matches with the originAttributes of the nsIPrincipal that has created the Blob URL. In order to do that, I disable the "old" NewChannel() method for Blob URLs.
Attached patch blob.patch (obsolete) — Splinter Review
Attachment #8749617 - Attachment is obsolete: true
Attachment #8749617 - Flags: review?(ckerschb)
Attachment #8750225 - Flags: review?(ckerschb)
Bz, can you take a look? Thanks
Flags: needinfo?(bzbarsky)
> With the current setup, when a blob URL is created, it is available for any other page who knows that unique blob URL.

That is... just not true.  CheckLoadURIWithPrincipal, if it sees a blob: URI target, will check that the given principal subsumes the URI's principal.  If it doesn't, the load is denied.  You can check this trivially:

1)  Load http://mozilla.org
2)  var url = URL.createObjectURL(new Blob(["a"], { type: "text/html" })) in the console.
3)  Type "url" in the console to see the URL.
4)  Open a new tab, load http://web.mit.edu
5)  In the console, type: var ifr = document.createElement("iframe"); ifr.src = your-url-string-here; document.body.before(ifr)

Observe the lack of loading in the iframe and the nice error along these lines in the browser console:

  Security Error: Content at http://web.mit.edu/ may not load data from blob:https://www.mozilla.org/9ea28411-11f0-cb49-8436-cfaaaba7d2ba.

At first glance, the Subsumes() check should be checking the origin attributes.  So again, why are the changes in this bug needed and what are they actually trying to accomplish?
Flags: needinfo?(bzbarsky) → needinfo?(amarchesini)
Or put another way, does the test pass _without_ the blob protocol handler changes?
(In reply to Boris Zbarsky [:bz] from comment #9)
> Or put another way, does the test pass _without_ the blob protocol handler
> changes?

My fault, the comment is misleading. What this bug is about is sharing blob URL from pages with the same origin but running in different containers. If you see what my test does, it creates 2 tabs, loading the same page but using 2 different userContextIds.

What I want to have is that the blob URLs are not shared.
Flags: needinfo?(amarchesini) → needinfo?(bzbarsky)
> from pages with the same origin but running in different containers.

OK, but why does that work right now?  Again, CheckLoadURI will call CheckMayLoad which will call Subsumes, which will return false if "OriginAttributesRef() != Cast(aOther)->OriginAttributesRef()".

I looked at the test and patch carefully, since you never did answer my question about what part of the test actually fails without the patch.  I believe the only difference between the behavior we have right now and the behavior with the patch is that with the patch if chrome code tries to directly load a blob URI from one user context in another user context the load will fail.  Is that specific circumstance what this bug is really about?  Because for loads not triggered by chrome code (not tested by the test, naturally), we already shouldn't share blob URIs across user contexts.
Flags: needinfo?(bzbarsky) → needinfo?(amarchesini)
OK, I stepped through this test (the part I was asking you to do), and the issue is that nsPrincipal::MayLoadInternal has logic like so:

  if (uriPrin && nsIPrincipal::Subsumes(uriPrin)) {
    return true;
  }

and otherwise it falls back on SecurityCompareURIs(mCodebase, aURI).  In this case the Subsumes() call returns false (because the load is not in fact triggered from chrome, turns out), but then the SecurityCompareURIs(mCodebase, aURI) bit just compares URIs and not context attributes.

I _think_ the right thing is to change the code to do this:

  if (uriPrin0 {
    return nsIPrincipal::Subsumes(uriPrin);
  }

and I could have sworn this came up before...
Oh, and note that that will block send() but not open().
Priority: -- → P1
Comment on attachment 8750225 [details] [diff] [review]
blob.patch

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

It seems bz is already on this bug and he is definitely the better reviewer for this patch.

::: dom/base/nsHostObjectProtocolHandler.cpp
@@ +611,4 @@
>  {
> +  // We don't implement this method because we want to force the use of
> +  // NewChannel2 in order to execute security checks based on OriginAttributes.
> +  return NS_ERROR_NOT_IMPLEMENTED;

I don't know how common addons create a channel through protocolHandler.newChannel(...) directly instead of going through the ioservice. This change will definitely affect and break some addons.
Attachment #8750225 - Flags: review?(ckerschb) → feedback-
Attached patch blob2.patchSplinter Review
bz, can you take a look, please?
Attachment #8750225 - Attachment is obsolete: true
Flags: needinfo?(amarchesini) → needinfo?(bzbarsky)
Comment on attachment 8751300 [details] [diff] [review]
blob2.patch

Yeah, this makes a lot more sense to me.  It also has the benefit of not breaking any addons that don't try to do the exact thing we're trying to prevent, right?

You should probably s/"Open/"Send/ in various parts of the test.
Flags: needinfo?(bzbarsky)
The patch is green on try. Can I take your comment as a r+ ?
Flags: needinfo?(bzbarsky)
Comment on attachment 8751300 [details] [diff] [review]
blob2.patch

Yes, r=me with the s/"Open/"Send/ bit.
Flags: needinfo?(bzbarsky)
Attachment #8751300 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/fe2ea22e1ede
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.