Closed Bug 1313711 (CVE-2017-5408) Opened 3 years ago Closed 3 years ago

Same-Origin-Policy violation via Text Track

Categories

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

52 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox-esr45 52+ fixed
firefox51 --- wontfix
firefox52 + verified
firefox-esr52 --- fixed
firefox53 + verified
firefox54 + verified

People

(Reporter: ericlaw1979, Assigned: hchang)

References

Details

(Keywords: csectype-sop, sec-moderate, Whiteboard: [adv-main52+][adv-esr45.8+])

Attachments

(2 files, 9 obsolete files)

274.76 KB, patch
abillings
: sec-approval+
Details | Diff | Splinter Review
2.82 KB, patch
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20161024030205

Steps to reproduce:

Repro at https://whytls.com/test/crossorigin/index.html


Actual results:

The video refers to a cross-origin text track file; the text tracks are used in the video despite no CORS header permitting cross-origin use. JavaScript can steal the content of the VTT text using <trackId>.track.cues[i].getCueAsHTML().


Expected results:

Captions should not be loaded unless a CORS directive on the VTT response permits cross-origin read access.
Component: Untriaged → Security
Radar'ing this to some folks who work on audio/video/vtt code.

Christoph, I would have expected triggering/loadingPrincipal stuff to prevent this - do we need another load type, are we using the wrong one, or... worse?
Group: firefox-core-security → core-security
Component: Security → Audio/Video: Playback
Flags: needinfo?(ckerschb)
Flags: needinfo?(alwu)
Flags: needinfo?(ajones)
Product: Firefox → Core
Relevant spec section seems to be
https://www.w3.org/TR/html5/embedded-content-0.html#sourcing-out-of-band-text-tracks

Can <track> be used (read) on its own? the element itself (§4.7.9) doesn't have a crossorigin attribute and the section above only gets CORS based on the setting of it's media parent.

We seem to block VTT when there's a redirect: https://whytls.com/test/crossorigin/index-302.html
Group: core-security → media-core-security
Passing this onto Blake to follow up on.
Flags: needinfo?(ajones) → needinfo?(bwu)
Assigned to Benjamin since he is the expert of WebVTT in my team!
Assignee: nobody → bechen
Flags: needinfo?(bwu)
Flags: needinfo?(alwu)
I would prefer when playback, we still load the vtt crossorigin. Then not allow crossorigin at getCueAsHTML().
https://www.w3.org/TR/html5/embedded-content-0.html#sourcing-out-of-band-text-tracks
Spec says: 
"If the track element's parent is a media element then let CORS mode be the state of the parent media element's crossorigin content attribute. Otherwise, let CORS mode be No CORS."
===
It is easy to get the MediaElement's CORSmode value from TrackElement.
===

"If URL is not the empty string, perform a potentially CORS-enabled fetch of URL, with the mode being CORS mode, the origin being the origin of the track element's Document, and the default origin behaviour set to fail."
==
I guess we need to consider the CORSmode value and the "default origin behaviour", then find a proper 
security flag in nsILoadInfo.idl for NS_NewChannel() in HTMLTrackElement::LoadResource().

https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLTrackElement.cpp#298
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsILoadInfo.idl#53
==

"The resource obtained in this fashion, if any, contains the text track data. If any data is obtained, it is by definition CORS-same-origin (cross-origin resources that are not suitably CORS-enabled do not get this far)."
==
hmm, I don't understand this paragraph...
==
(In reply to :Gijs Kruitbosch from comment #1)
> Radar'ing this to some folks who work on audio/video/vtt code.
> 
> Christoph, I would have expected triggering/loadingPrincipal stuff to
> prevent this - do we need another load type, are we using the wrong one,
> or... worse?

Sorry for the lag of response on this bug and unfortunately I will not be able to look at this bug for quite some time. To avoid any further delays I am hoping that Tanvi could take a look and provide some pointers. Sorry for loading this off onto you Tanvi :-(
Flags: needinfo?(ckerschb) → needinfo?(tanvi)
Adding dveditz and we will look at this tomorrow.
Flags: needinfo?(tanvi) → needinfo?(dveditz)
Flags: needinfo?(tanvi)
Flags: needinfo?(tanvi)
Flags: needinfo?(tanvi)
Dan and I discussed during triage and he said he will take a closer look, so clearing my needinfo.
Flags: needinfo?(tanvi)
Un-assign Benjamin since he cannot help on this bug much. 
CC Ethan as well to see if we could work together on this bug in Taipei.
Assignee: bechen → nobody
Blake - can you find another owner for this?
Dan - have you looked at this per comment 9?

Thanks!
Flags: needinfo?(bwu)
Per previous discussion with Benjamin, this is more related to security. 
Ni Ethan, TPE security manager, to see if any of his team member can work with Benjamin to move this bug forward.
Flags: needinfo?(bwu) → needinfo?(ettseng)
Flags: needinfo?(ettseng)
Henry, please look into this bug to see if we can do anything on it, thanks.
Assignee: nobody → hchang
Attached patch Bug1313711.patch (obsolete) — Splinter Review
:bechen has explained very well in comment 6, the solution is so straightforward: 

- if the track has no parent element, use "No CORS"
- if the parent element is a media element, use its CORS mode

Unlike <img>, the default cross origin behavior of <track> is "fail".

The patch is still required a test case but at least the attached link has been
verified to behaves as expected.
Comment on attachment 8829356 [details] [diff] [review]
Bug1313711.patch

Hi :dveditz,

Could you please review my solution to this issue? 

I still have no idea how to write a test case from scratch 
but I believe there have been bunch of CORS test case
templates I can learn from so it shouldn't be a big deal.

Thanks!
Flags: needinfo?(dveditz)
Attachment #8829356 - Flags: review?(dveditz)
https://dxr.mozilla.org/mozilla-central/source/dom/canvas/test/crossorigin/test_canvas2d_crossorigin.html

may be a perfect template which I can use to test all CORS mode behaviors.
Attached patch Bug1313711_v2.patch (obsolete) — Splinter Review
Updated the patch:

1) Test case added.
2) If AnsyncOpen2 failed, call "SetReadyState" to obtain an onerror event.
3) Fixed security flag determination issue in the previous patch.
Attachment #8829356 - Attachment is obsolete: true
Attachment #8829356 - Flags: review?(dveditz)
Comment on attachment 8829430 [details] [diff] [review]
Bug1313711_v2.patch

Hi :dveditz,

Could you please review the entire patch including the test case?
The main purpose of the test case is not to re-test our CORS implementation
but to verify if the security flag is properly set in different CORS mode.

Hi :bechen,

Could you review or feedback the part where we call SetReadyState
after AsyncOpen2 failed? I don't know what's the possible downside 
if not putting the element into a ready state but I or the web developer
should need "onerror" event (triggered in SetReadyState) for
error handling.

Thanks you two :)
Attachment #8829430 - Flags: review?(dveditz)
Attachment #8829430 - Flags: review?(bechen)
Comment on attachment 8829430 [details] [diff] [review]
Bug1313711_v2.patch

r+, please fix/figure out the mochitest fail and run the web-platform-tests at tryserver.
Attachment #8829430 - Flags: review?(bechen) → review+
Attached patch Bug1313711_Part1.patch (obsolete) — Splinter Review
Attachment #8829430 - Attachment is obsolete: true
Attachment #8829430 - Flags: review?(dveditz)
Attached patch Bug1313711_Part2.patch (obsolete) — Splinter Review
Attached patch Bug1313711_Part3.patch (obsolete) — Splinter Review
Attachment #8829804 - Flags: review?(dveditz)
Attachment #8829806 - Flags: review?(francois)
Attachment #8829805 - Flags: review?(dveditz)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f767f69fd7792127a677adb1b8341c7088eb2d98

All the failed test cases in the previous push should have been fixed. Let's wait and see.
Henry, his is rated sec-high and you pushed to try (which is public) with bug number, commit messages, and additional tests, all describing exactly what the problem is. Please don't do that. :-(

Do we need to hold 51 over this given that this has happened? (I don't know how strict our VTT parsing is, if we are strict about the mimetype, and if you could abuse this to steal arbitrary data, or not.)
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Attachment #8829806 - Flags: review?(francois) → review+
We can't hold 51. It is already built and out the door. It shipped this morning and was in process yesterday. I'll defer to Dan if we need to do anything here but I suspect that if we do a point release, we should try to get this in.
Flags: needinfo?(abillings)
(In reply to Al Billings [:abillings] from comment #27)
> We can't hold 51. It is already built and out the door. It shipped this
> morning and was in process yesterday. I'll defer to Dan if we need to do
> anything here but I suspect that if we do a point release, we should try to
> get this in.

We're currently doing a point release for the geolocation/safebrowsing bustage in bug 1333516, but I don't think this is ready to make release. :-\
(In reply to :Gijs from comment #26)
> Henry, his is rated sec-high and you pushed to try (which is public) with
> bug number, commit messages, and additional tests, all describing exactly
> what the problem is. Please don't do that. :-(
> 

Sorry about that :( Is there any other guideline that I can find while fixing
sec-high bugs?
(In reply to Henry Chang [:henry][:hchang] from comment #29)
> (In reply to :Gijs from comment #26)
> > Henry, his is rated sec-high and you pushed to try (which is public) with
> > bug number, commit messages, and additional tests, all describing exactly
> > what the problem is. Please don't do that. :-(
> > 
> 
> Sorry about that :( Is there any other guideline that I can find while fixing
> sec-high bugs?

For example, do I also have to leave no bug number and any message when I 
actually land the patch? People can learn something by reading the 
commit message in mozilla-central and get to know the vulnerability
of the current release.
Hi Jon,

Since you are the one who implemented HTMLMediaElement's CORS mode, 
do you know if there is any reason that we initialize mCORSMode 
only when loading a resource? When trying to re-enable track element's
CORS web-platform test cases, I found we have to either 

1) Initialize |mCORSMode| without having to load a resource or
2) Modify the test case [2] to set a dummy "video.src" to
   force HTMLMediaElement::LoadResource() to be called

to get a correct track element's CORS behavior.

For your information, track element's CORS mode is decided by
its parent's CORS mode (what specification says). Given that 
loading a track resource is independent of its parent media element [3], 
(i.e. tracks can be loaded even if their parent media element 
has nothing to load.), we have to know a track element's
parent element's CORS mode no matter if its parent has
loaded any resource.


[1] https://hg.mozilla.org/mozilla-central/diff/05316b7ecf15/content/html/content/src/nsHTMLMediaElement.cpp#l1.12
[2] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/html/semantics/embedded-content/media-elements/track/track-element/cors/support/common.js#44
[3] It's how we implemented it. Not sure if it's in the spec.
Flags: needinfo?(jon)
(In reply to Henry Chang [:henry][:hchang] from comment #29)
> (In reply to :Gijs from comment #26)
> > Henry, his is rated sec-high and you pushed to try (which is public) with
> > bug number, commit messages, and additional tests, all describing exactly
> > what the problem is. Please don't do that. :-(
> > 
> 
> Sorry about that :( Is there any other guideline that I can find while fixing
> sec-high bugs?

https://wiki.mozilla.org/Security/Bug_Approval_Process

(In reply to Henry Chang [:henry][:hchang] from comment #30)
> (In reply to Henry Chang [:henry][:hchang] from comment #29)
> > (In reply to :Gijs from comment #26)
> > > Henry, his is rated sec-high and you pushed to try (which is public) with
> > > bug number, commit messages, and additional tests, all describing exactly
> > > what the problem is. Please don't do that. :-(
> > > 
> > 
> > Sorry about that :( Is there any other guideline that I can find while fixing
> > sec-high bugs?
> 
> For example, do I also have to leave no bug number and any message when I 
> actually land the patch? People can learn something by reading the 
> commit message in mozilla-central and get to know the vulnerability
> of the current release.

As noted in the wiki (and in a note underneath the attachments on bugzilla!), you would need to ask for sec-approval before landing on mozilla-central, and then the security team will coordinate with you when patches land where (e.g. they might suggest landing only 2 weeks before release, to reduce the time period between when we 'reveal' the bug via checkins and when we ship a fix on all affected branches). Security patches often land with only a bug number and reviewers, but no description in the commit message, and we sometimes delay landing relevant automated tests (until after the fix has shipped everywhere) if they give a clear indication of what the problem is.
Going forward, please use the WHATWG HTML Standard as reference: https://html.spec.whatwg.org/multipage/embedded-content.html#start-the-track-processing-model. That's what we're attempting to follow and implement.
Comment on attachment 8829804 [details] [diff] [review]
Bug1313711_Part1.patch

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

r=dveditz
Attachment #8829804 - Flags: review?(dveditz) → review+
Comment on attachment 8829805 [details] [diff] [review]
Bug1313711_Part2.patch

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

r=dveditz

::: dom/media/test/test_trackelement_cors.html
@@ +13,5 @@
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +SimpleTest.waitForExplicitFinish();
> +
> +const TRACK_CORSS_ORIGIN_PATH_PREFIX = "http://example.org/tests/dom/media/test/";

CROSS_ORIGIN typo? or a pun? Same here and elsewhere so it'll work; if it's intentional I'm fine with it.
Attachment #8829805 - Flags: review?(dveditz) → review+
(In reply to :Gijs from comment #26)
> this is rated sec-high

I didn't find enforcement that the Content-Type must be text/vtt (required by the version of the spec I found), but the parser does enforce that the file must start with the WEBVTT line. That limits this SOP violation to things intended to be text tracks, and while those might be private I don't think there's enough of it deployed for this to be respin-worthy. We can catch it in 52. Please request sec-approval before landing, and we're going to want to uplift to aurora and beta.

When did we start supporting WebVTT? do we need to patch ESR-45 as well?
Flags: needinfo?(dveditz)
cc Ralph who worked on WebVTT; he can probably answer comment 36's question
Flags: needinfo?(giles)
Basic WebVTT/HTMLTrackElement support landed in Firefox 24, according to mdn and bug 833382. It looks like the last major change to the policy (possibly a no-op, I'm unclear) was bug 1182540 landing in Firefox 42 so I expect esr releases to behave the same. May need rebasing past bug 1262406 which added the url classifier.

IIRC text tracks were one of the last things which slipped in before we started enforcing CORS on new features.
Flags: needinfo?(giles)
(In reply to Daniel Veditz [:dveditz] from comment #35)
> Comment on attachment 8829805 [details] [diff] [review]
> Bug1313711_Part2.patch
> 
> Review of attachment 8829805 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=dveditz
> 
> ::: dom/media/test/test_trackelement_cors.html
> @@ +13,5 @@
> > +<pre id="test">
> > +<script class="testbody" type="text/javascript">
> > +SimpleTest.waitForExplicitFinish();
> > +
> > +const TRACK_CORSS_ORIGIN_PATH_PREFIX = "http://example.org/tests/dom/media/test/";
> 
> CROSS_ORIGIN typo? or a pun? Same here and elsewhere so it'll work; if it's
> intentional I'm fine with it.

It's typo. Sorry for that :(

BTW, I have fixed all the web-platform-test, which requires 3 more patches

1) is a fix for WebVTT redirection [1]. It has nothing to do with this bug but
   leads some web-platform tests to fail. |cb| has to be called before return
   according to [2].
2) is to initialize HTMLMediaElement::mCORSMode in a different way 
   as I mentioned in comment 31.
3) is to fix all "expected PASS" for most of the web-platform-tests. Some tests
   still got FAIL because deciding whether "Same origin" or not on redirection
   seems to behave different from what web-platform-test expects [3].

However, given that we might need to uplift the patches all the way to 51 (as well as esr 45),
I wonder what's the best way to deal with this bug:

1) Fix everything including the web-platform-test
2) Only fix the change where we use proper security flag (attachment 8829804 [details] [diff] [review]) 
   plus the test case I added.
3) Only fix the change where we use proper security flag (attachment 8829804 [details] [diff] [review]) w/o the test case.

If we take (2) and (3), I'll create separate bugs for what's unresolved in this bug.

p.s. I'd like to have a try push with all web-platform-test fixes. Is it okay or
     I'd better do that later? Thanks!

[1] https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/dom/media/WebVTTListener.cpp#72 
[2] https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/netwerk/base/nsIChannelEventSink.idl#73
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=814141
Flags: needinfo?(dveditz)
Attached patch WebVTT_redirection_fix.patch (obsolete) — Splinter Review
Attached patch All_web-platform_tests_fix.patch (obsolete) — Splinter Review
Comment on attachment 8832728 [details] [diff] [review]
WebVTT_redirection_fix.patch

Hi Ralph,

Could you please review this fix for WebVTT redirection issue? 

Without this patch, the vtt redirection will fail silently in release
build and crash (assertion) in debug build. I found this issue when
I was trying to fix web-platform tests. 

I haven't had a idea whether I can land this patch for this bug yet
but I wonder if you can have it a review first. Thanks :)
Attachment #8832728 - Flags: review?(giles)
Comment on attachment 8832729 [details] [diff] [review]
Init_HTMLMediaElement_mCORSMode_early.patch

Hi JW,

Could you review this patch for initializing HTMLMediaElement::mCORSMode
before loading resource? It makes sense to initialize mCORSMode right before
loading resource without embedded track. However, since the embedded track
will use its parent media element's CORS mode, we should keep 
HTMLMediaElement::mCORSMode update-to-date at all time. As other patch,
I don't know I am going to land this patch for this bug but do
you mind having it a review first for web-platform tests? Thanks!
Attachment #8832729 - Flags: review?(jwwang)
Comment on attachment 8832728 [details] [diff] [review]
WebVTT_redirection_fix.patch

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

Thanks. If this fixes bug 973384, you could land this change there without needing to wait for security review.
Attachment #8832728 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) | needinfo me from comment #45)
> Comment on attachment 8832728 [details] [diff] [review]
> WebVTT_redirection_fix.patch
> 
> Review of attachment 8832728 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks. If this fixes bug 973384, you could land this change there without
> needing to wait for security review.

Awesome! I believe it will fix Bug 973384 since that assertion is
exactly what I hit, too. I'll directly work on that bug.

Thanks!
Comment on attachment 8832729 [details] [diff] [review]
Init_HTMLMediaElement_mCORSMode_early.patch

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

::: dom/html/HTMLMediaElement.cpp
@@ +4217,5 @@
>      }
>    }
>  
> +  if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::crossorigin) {
> +      mCORSMode = AttrValueToCORSMode(aValue);

2-space indent.
Attachment #8832729 - Flags: review?(jwwang) → review+
Attachment #8832730 - Flags: review?(dveditz)
Flags: needinfo?(jon)
Attachment #8832730 - Flags: review?(dveditz) → review+
(In reply to Henry Chang [:henry][:hchang] from comment #39)
> 1) Fix everything including the web-platform-test
> 2) Only fix the change where we use proper security flag (attachment 8829804 [details] [diff] [review]
> [details] [diff] [review]) 
>    plus the test case I added.
> 3) Only fix the change where we use proper security flag (attachment 8829804 [details] [diff] [review]
> [details] [diff] [review]) w/o the test case.

When we land this fix on older branches we can't turn the trees red. We don't need to add the new testcase but we need to fix up everything that's going to be a new failure, and in that case we might as well add the new test.

> p.s. I'd like to have a try push with all web-platform-test fixes. Is it
> okay or
>      I'd better do that later? Thanks!

That will be fine for this bug.
Flags: needinfo?(dveditz)
Depends on: 973384
Squash all patches to one. Some of the r+'ed patches are not picked since
we disable all track element CORS related wpt.
Attachment #8829804 - Attachment is obsolete: true
Attachment #8829805 - Attachment is obsolete: true
Attachment #8829806 - Attachment is obsolete: true
Attachment #8832728 - Attachment is obsolete: true
Attachment #8832729 - Attachment is obsolete: true
Attachment #8832730 - Attachment is obsolete: true
Comment on attachment 8836719 [details] [diff] [review]
Bug1313711_for_sec-approval.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? 
A: The patch reveals we didn't implement track element's CORS behavior correctly
   so it's easy to construct an exploit to load cross origin track.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
A: Yes.


Which older supported branches are affected by this flaw?
A: All.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
A: Yes. Just same as this patch.

How likely is this patch to cause regressions; how much testing does it need?
A: Very less likely since this is a simple fix. Try loading a track in a parent media element.
   The track should follow the its parent media element's CORS behavior.
Attachment #8836719 - Flags: sec-approval?
(In reply to Henry Chang [:henry][:hchang] from comment #50)
> Try result for Bug1313711_for_sec-approval.patch
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4afbbcd190744dd2da7c754656f622c4d183c2bd&selectedJob=7
> 6757833

and even worse:

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
> A: Yes.


Sigh.  Please be very careful about pushing sec-bug patches, especially with new tests, to Try.  In general, don't push patches for sec-high or sec-critical to Try.  On occasions where it must be done, and local tests don't suffice, obscuring the purpose of the Try and tests may be warranted, and checking with someone on sec-team before pushing is smart (or ask for approval first, and ask about running a Try).

NI to abillings/deveditz to deal with the fallout

We're still waiting to have a sec-bug Try/treeherder instance.... :-(
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Also note comment 26, which pointed out the same mistake earlier
We shouldn't even be pushing tests anywhere (or checking them in) before a bug is fixed and that fix is released to the public unless we have no choice.

I'll give sec-approval+ for the patch WITHOUT any tests. Do not check in these tests, Henry.

We're going to need Aurora, Beta, and ESR45 patches made and nominated as well.
Attachment #8836719 - Flags: sec-approval? → sec-approval+
(In reply to Randell Jesup [:jesup] from comment #52)
> (In reply to Henry Chang [:henry][:hchang] from comment #50)
> > Try result for Bug1313711_for_sec-approval.patch
> > 
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=4afbbcd190744dd2da7c754656f622c4d183c2bd&selectedJob=7
> > 6757833
> 
> and even worse:
> 
> > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
> > A: Yes.
> 
> 
> Sigh.  Please be very careful about pushing sec-bug patches, especially with
> new tests, to Try.  In general, don't push patches for sec-high or
> sec-critical to Try.  On occasions where it must be done, and local tests
> don't suffice, obscuring the purpose of the Try and tests may be warranted,
> and checking with someone on sec-team before pushing is smart (or ask for
> approval first, and ask about running a Try).
> 

I thought I was granted to push try with test cases in comment 48.
Maybe the grant was for "web-platform-test" but not all tests.

(At that time we already have a bunch of web-platform test cases specifically
for this bug and this bug will change some expected value of those tests
if Bug 973384 didn't disable all those tests first)


Anyways, sorry about that :(


> NI to abillings/deveditz to deal with the fallout
> 
> We're still waiting to have a sec-bug Try/treeherder instance.... :-(
Asking for sec-approval again since I have no idea how to deal with 
a required change in the existed test case. (test_classify_track.html)
Attachment #8836719 - Attachment is obsolete: true
Attachment #8836981 - Flags: sec-approval?
(In reply to Henry Chang [:henry][:hchang] from comment #56)
> Created attachment 8836981 [details] [diff] [review]
> Bug1313711_sec-approval.patch
> 
> Asking for sec-approval again since I have no idea how to deal with 
> a required change in the existed test case. (test_classify_track.html)

Also, not sure if the check-in comment is appropriate.
> I thought I was granted to push try with test cases in comment 48.
> Maybe the grant was for "web-platform-test" but not all tests.

It appears you were given the go-ahead to push tests to Try - I didn't go back and read the entire history of the bug -- it might not hurt when pushing a try to note in the bug that it was approved to do so.  And I'm not clear if dveditz was approving just pushing some test fixes to Try, separately from the patch here, or both - you indicate in comment 39 that some of the tests are wrong regardless of this patch.  He's right that if we're uplifting, and the uplift will break tests, we have to at least fix those, and in that case we may simply add tests if they don't expose anything beyond what may be obvious already, but this is a case-by-case thing.

We do want to avoid painting bullseyes on sec bugs in Try pushes (or inbound/etc pushes), which is why we often hold new tests from landing, and may avoid immediately adding comments pointing out the flaw.  (We then would schedule landing tests and comments after the fix is in release for a reasonable period.)
(In reply to Henry Chang [:henry][:hchang] from comment #55)
> I thought I was granted to push try with test cases in comment 48.
> Maybe the grant was for "web-platform-test" but not all tests.

No, it was "for this bug" because we already blew it earlier (comment 26).

In general we want to have a separate testcase patch for sec-high/sec-critical security bugs, and don't land that patch--or push to try!--until after the security bug is fixed. People have different strategies for remembering. I strongly urge filing a "Land testcase for bug XXXX after release YY" hidden bug assigned to yourself. Some people set the in-testsuite flag to "?" and then later set it to "in-testsuite+" when the tests are landed, but that's usually harder to remember unless you do this regularly and check often.
Flags: needinfo?(dveditz)
Comment on attachment 8836981 [details] [diff] [review]
Bug1313711_sec-approval.patch

sec-approval+ for the patch but *not* for tests. Do not check in tests.
Attachment #8836981 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #60)
> Comment on attachment 8836981 [details] [diff] [review]
> Bug1313711_sec-approval.patch
> 
> sec-approval+ for the patch but *not* for tests. Do not check in tests.

Thanks for the approval but I have to either modify the "existed" tests or
remove it. The reason is the existed test has to load a track from a 
foreign origin to do safebrowsing test and we disallow loading track from
foreign site with no CORS mode inherited from its parent media element.
Flags: needinfo?(abillings)
Ok. Just modify the tests then.
Flags: needinfo?(abillings)
Thanks Al! I am flagging "checkin-needed" but not sure if the patch comment is proper.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0b21c30d0edc
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
We really need uplift requests on this bug ASAP (and variant patches if needed), since it's already landed on inbound/central.
Flags: needinfo?(hchang)
Comment on attachment 8836981 [details] [diff] [review]
Bug1313711_sec-approval.patch

Approval Request Comment
[Feature/Bug causing the regression]: WebVTT
[User impact if declined]: Cross origin track can be loaded even if its parent doesn't allow.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Yes. STR is in comment 0.
[List of other uplifts needed for the feature/fix]: no.
[Is the change risky?]: no.
[Why is the change risky/not risky?]: It's a simple fix.
[String changes made/needed]: No.

Should I request beta/release/esrs individually?

I am sure the patch cannot directly apply on esr45 so I have to make
a specific patch esr 45 at least.
Flags: needinfo?(hchang)
Attachment #8836981 - Flags: approval-mozilla-aurora?
(In reply to Henry Chang [:henry][:hchang] from comment #67)

> Should I request beta/release/esrs individually?

You should request every branch the patch applies to, and can do so in a single step. Approvals will be granted (or not) individually based on the review schedule for each branch.

Please check if it applies to beta and esr45 and if not, prepare backports asap.
Comment on attachment 8836981 [details] [diff] [review]
Bug1313711_sec-approval.patch

Other than a trivial mochitest manifest conflict, this grafts cleanly to Beta too.
Attachment #8836981 - Flags: approval-mozilla-beta?
Comment on attachment 8836981 [details] [diff] [review]
Bug1313711_sec-approval.patch

Fix a sec-high. Aurora53+.
Attachment #8836981 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8836981 [details] [diff] [review]
Bug1313711_sec-approval.patch

fix cross-origin bug with subtitles in beta52
Attachment #8836981 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: media-core-security → core-security-release
Comment on attachment 8840285 [details] [diff] [review]
Bug1313711_backport_for_esr45.patch

Looks like the same basic patch as what landed on the other branches minus the pile of test-only changes. See comment 67 for approval request comments.
Attachment #8840285 - Flags: approval-mozilla-esr45?
Comment on attachment 8840285 [details] [diff] [review]
Bug1313711_backport_for_esr45.patch

Fix a sec-high. This patch matches the ESR criteria. ESR45+.
Attachment #8840285 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Reproduced the issue with an affected build (50.1.0, 20161208153507) using the instructions from Comment 0 on Ubuntu 16.04 x64. WebVTT captions were being displayed.

This is verified fixed on Ubuntu 16.04 x64, Windows 10 x64 and macOS 10.12.3 using:

    - 52.0b9-build2 (20170223185858)
    - 53.0a2 (2017-02-24)
    - 54.0a1 (2017-02-24)

The test page now no longer shows WebVTT captions.
Whiteboard: [adv-main52+][adv-esr45.8+]
Alias: CVE-2017-5408
Although we reflexively rate SOP violations "sec-high", in this particular case that seems to overstate the risk (e.g. given the limitations mentioned in comment 36).
Keywords: sec-highsec-moderate
See Also: → 1337242
See Also: → 1396916
Group: core-security-release
Depends on: 1464012
You need to log in before you can comment on or make changes to this bug.