1228677 - Cloning <video crossorigin> trips assertion failure: "can not enforce CORS when calling Open2()"

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase (save, fix paths, load from file:)

Assertion failure: (loadInfo->GetSecurityMode() & nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS) == 0 (can not enforce CORS when calling Open2()), at dom/media/MediaResource.cpp:1320

This assertion was added in:

changeset:   https://hg.mozilla.org/mozilla-central/rev/f737c4a01752
user:        Christoph Kerschbaumer
date:        Sat Sep 12 12:59:08 2015 -0700
summary:     Bug 1194524 - Use channel->ascynOpen2 in dom/media/MediaResource.cpp (r=sicking)

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note that you will probably need to run the testcase from a file:// URI to get it to hit this codepath.  Or at least the video URL will need to be a file:// URI and the testcase will need to be able to link to file://.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

We just fixed a similar issue for XSLT [1].

Jonas, do we need a better fix for the problem of needing CORS when calling Open2()? Not sure if the problem described in this bug is an actual problem on a webpage and if we need to fix it at all. But if we need a fix for that problem, then potentially we should fix it in the contentSecuritymanager rather than on every callsite. What do you think?

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/f1c08ce11d24

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't have a strong opinion either way.

I don't think this needs to stay security-sensitive though. At worst we probably crash with a null-pointer dereference here.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Christoph, are you on this one?

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #5)
> Christoph, are you on this one?

Hey Jesse, sorry for the lag of response here. Anyway, I can't reproduce the assertion. I adopted the paths in your testfile (I tested that I adopted the path correctly by directly copying the location into the URL bar and it loads) but when opening the testilfe it rather uses asyncOpen2() [1] instead of open2() [2] within MediaResource.cpp. Note that the assertion only gets triggered when using the codepath of open2().

What am I doing wrong? Can you still reproduce the error?

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/f737c4a01752#l1.18
[2] https://hg.mozilla.org/integration/mozilla-inbound/rev/f737c4a01752#l1.116

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Assigning to myself to make sure this one gets fixed (if it turns out to be a real problem) ASAP.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Jesse, can you still reproduce that problem are can we close this bug as invalid?

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Tanvi, I have been trying forever to reproduce this error using mac and linux. Any chance you try and let me know if you can reproduce the problem? In my testing it's always openend using ::AsyncOpen2() and never using ::open2().

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Also opening up this bug as disccused within Comment 4.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Finally I am able to reproduce, you also need to set:
> user_pref("security.fileuri.strict_origin_policy", false);

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: MozReview Request: Bug 1228677 - Do not enforce CORS when loading file for MediaResource (r=sicking)

Review commit: https://reviewboard.mozilla.org/r/38035/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38035/

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Jonas, it seems we introduced the bug ourselves :-( If you look at [1] there was never CORS enforced for FileMediaResource, only for ChannelMediaResource. So I think we just need to revert that change.

[1] https://hg.mozilla.org/mozilla-central/rev/f737c4a01752

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Someone that knows this code should review this. But why remove the assertion before the Open2() call?

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #14)
> Someone that knows this code should review this. But why remove the
> assertion before the Open2() call?

We could keep it, but I figured we also have that assertion [1]. I am fine either way.

Chris, as the module owner, could you take a look and review those bits?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#107

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Note: We use FileMediaResource for reading blob URIs. So you don't need to be loading from a local file to hit this. I can repro this assertion failure with this testcase:

<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8">
<script>

function boom()
{
  var v = document.querySelector('video');
  v.onloadeddata = function() {
    v.cloneNode(false);
  };
  fetch('http://people.mozilla.org/~cpearce/video/h264_baseline_lvl3.mp4')
  .then(function(response) {
    return response.blob();
  })
  .then(function(myBlob) {
    v.src = URL.createObjectURL(myBlob);
  });
}

</script>
</head>

<body onload="boom();">
<video id="x" src="" crossorigin="true"></video>
</body>
</html>

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

I think this patch is safe, because we only clone a media element's underlying FileMediaResources into a new media element if the old and the new media element have equal principals and the same values of "crossorigin" attributes; see the call to HTMLMediaElement::LookupMediaElementURITable() in HTMLMediaElement::LoadResource().

Note my testcase in comment 16 causes Beta Firefox 45 to crash when the testcase was loaded from a local webserver.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Chris Pearce (:cpearce) from comment #17)
> I think this patch is safe, because we only clone a media element's
> underlying FileMediaResources into a new media element if the old and the
> new media element have equal principals and the same values of "crossorigin"
> attributes; see the call to HTMLMediaElement::LookupMediaElementURITable()
> in HTMLMediaElement::LoadResource().

That is to say, we explicitly do a CORS check before going down the clone path in question here.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1228677_fix_cors_error.patch

(In reply to Chris Pearce (:cpearce) from comment #17)
> I think this patch is safe,

Thanks for the info Chris. I think Jonas would prefer if someone signs off on the patch. Would you be willing to do that?

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8727238 [details] [diff] [review]
bug_1228677_fix_cors_error.patch

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

r=cpearce.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Chris Pearce (:cpearce) from comment #20)
> Comment on attachment 8727238 [details] [diff] [review]
> bug_1228677_fix_cors_error.patch
> 
> Review of attachment 8727238 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=cpearce.

Thanks Chris!

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30fa1cfd2ec8

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/30fa1cfd2ec8