Closed Bug 1289968 Opened 8 years ago Closed 8 years ago

[EME] WebM initData can have keyIds of arbitrary length

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: cpearce, Assigned: kikuo)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files, 3 obsolete files)

The EME format registry for WebM says that EME initData in the WebM format may contain keyIds of one or more bytes:

https://w3c.github.io/encrypted-media/format-registry/initdata/webm.html#format

Our ClearKey implementation is assuming that WebM keyIds are always 16 bytes long.

This is causing this EME WPT of Google's to fail:
https://w3c-test.org/encrypted-media/Google/encrypted-media-keystatuses-multiple-sessions.html
InitData of type keyIds can also have keys of arbitrary length.
I also added more testing around ClearKey's base64 decoding, since that affected
how keyIds were handled.

Review commit: https://reviewboard.mozilla.org/r/69900/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69900/
Attachment #8778740 - Flags: review?(gsquelart)
Comment on attachment 8778740 [details]
Bug 1289968 - Ensure ClearKey doesn't assume keyIds can only be 16 bytes.

https://reviewboard.mozilla.org/r/69900/#review67038

r+ with nit:

::: media/gmp-clearkey/0.1/gtest/TestClearKeyUtils.cpp:43
(Diff revision 1)
>      true
>    },
>    {
>      "flczM35XMzN-VzMzflczMw",
>      { 0x7e, 0x57, 0x33, 0x33, 0x7e, 0x57, 0x33, 0x33, 0x7e, 0x57, 0x33, 0x33, 0x7e, 0x57, 0x33, 0x33 },
> -    true
> +    true,

Unnecessary comma, inconsistent with other groups.
Attachment #8778740 - Flags: review?(gsquelart) → review+
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4361c6f3b915
Ensure ClearKey doesn't assume keyIds can only be 16 bytes. r=gerald
Backed out for failing encrypted-media-generate-request-disallowed-input.html:

https://hg.mozilla.org/integration/autoland/rev/40c1b2d67a06bb684e353607556262e2ccc7fa5c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4361c6f3b91504ad106f0f9125987891c748ddee
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=1567276&repo=autoland

05:24:43     INFO - TEST-START | /encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html
05:24:44     INFO - 
05:24:44     INFO - TEST-UNEXPECTED-FAIL | /encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html | generateRequest() with webm initData longer than 64Kb characters. - assert_unreached: generateRequest() succeeded Reached unreachable code
05:24:44     INFO - test_session/</<@http://web-platform.test:8000/encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html:30:25
05:24:44     INFO - Async*test_session/<@http://web-platform.test:8000/encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html:24:28
05:24:44     INFO - Async*test_session@http://web-platform.test:8000/encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html:19:24
05:24:44     INFO - @http://web-platform.test:8000/encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html:41:24
05:24:44     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1403:20
05:24:44     INFO - promise_test/tests.promise_tests<@http://web-platform.test:8000/resources/testharness.js:532:36
05:24:44     INFO - Async*promise_test@http://web-platform.test:8000/resources/testharness.js:531:31
05:24:44     INFO - @http://web-platform.test:8000/encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html:38:13
05:24:44     INFO - 
05:24:44     INFO - TEST-PASS | /encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html | generateRequest() with cenc initData longer than 64Kb characters. 
05:24:44     INFO - TEST-PASS | /encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html | generateRequest() with keyids initData longer than 64Kb characters. 
05:24:44     INFO - TEST-PASS | /encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html | generateRequest() with invalid pssh data. 
05:24:44     INFO - TEST-PASS | /encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html | generateRequest() with non pssh data. 
05:24:44     INFO - TEST-PASS | /encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html | generateRequest() with too short key ID. 
05:24:44     INFO - TEST-UNEXPECTED-FAIL | /encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html | generateRequest() with too long key ID. - assert_unreached: generateRequest() succeeded Reached unreachable code
05:24:44     INFO - test_session/</<@http://web-platform.test:8000/encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html:30:25
05:24:44     INFO - Async*test_session/<@http://web-platform.test:8000/encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html:24:28
05:24:44     INFO - Async*test_session@http://web-platform.test:8000/encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html:19:24
05:24:44     INFO - @http://web-platform.test:8000/encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html:100:24
05:24:44     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1403:20
05:24:44     INFO - promise_test/tests.promise_tests<@http://web-platform.test:8000/resources/testharness.js:532:36
05:24:44     INFO - Async*promise_test@http://web-platform.test:8000/resources/testharness.js:531:31
05:24:44     INFO - @http://web-platform.test:8000/encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html:95:13
05:24:44     INFO - TEST-OK | /encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html | took 801ms
Flags: needinfo?(cpearce)
Blake: Can you find someone to get the failures here fixed and landed?
Flags: needinfo?(cpearce) → needinfo?(bwu)
(In reply to Chris Pearce (:cpearce) from comment #6)
> Blake: Can you find someone to get the failures here fixed and landed?
Kilik!
Flags: needinfo?(bwu) → needinfo?(kikuo)
Sure, I'd like to help on this. I found that this patch seems to cause other unexpected results when I ran web-platform-tests under /encrypted-media/Google/, the result was shown below,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
Ran 161 tests (38 parents, 123 subtests)
Expected results: 154
Unexpected results: 7 (FAIL: 3, PASS: 3, TIMEOUT: 1)

Unexpected Results
==================

/encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html
------------------------------------------------------------------------------
FAIL generateRequest() with webm initData longer than 64Kb characters.
FAIL generateRequest() with too long key ID.
/encrypted-media/Google/encrypted-media-keystatuses-multiple-sessions.html
--------------------------------------------------------------------------
PASS expected TIMEOUT Verify MediaKeySession.keyStatuses with multiple sessions.
/encrypted-media/Google/encrypted-media-keystatuses-multiple-updates.html
-------------------------------------------------------------------------
PASS expected TIMEOUT Verify MediaKeySession.keyStatuses with multiple updates.
/encrypted-media/Google/encrypted-media-keystatuses.html
--------------------------------------------------------
FAIL expected TIMEOUT Verify MediaKeySession.keyStatuses.
/encrypted-media/Google/encrypted-media-syntax.html
---------------------------------------------------
TIMEOUT Test MediaKeys generateRequest().
PASS expected TIMEOUT Test MediaKeySession update().
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<


There's no explicit size limitation for webm InitData (ContentEncKeyID), but considering the data size specified by EBML principle [1], it may exceed the 64KB boundary in the test.

[1] https://matroska.org/technical/specs/index.html
[2] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/encrypted-media/Google/encrypted-media-generate-request-disallowed-input.html#42

I've done size-check for webm initdata & the size of keyids, then I got the following test results,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
Ran 161 tests (38 parents, 123 subtests)
Expected results: 155
Unexpected results: 6 (FAIL: 1, PASS: 3, TIMEOUT: 2)

Unexpected Results
==================

/encrypted-media/Google/encrypted-media-async-creation-with-gc.html
-------------------------------------------------------------------
TIMEOUT Test asynchronous creation of MediaKeys and MediaKeySession while running garbage collection.
/encrypted-media/Google/encrypted-media-keystatuses-multiple-sessions.html
--------------------------------------------------------------------------
PASS expected TIMEOUT Verify MediaKeySession.keyStatuses with multiple sessions.
/encrypted-media/Google/encrypted-media-keystatuses-multiple-updates.html
-------------------------------------------------------------------------
PASS expected TIMEOUT Verify MediaKeySession.keyStatuses with multiple updates.
/encrypted-media/Google/encrypted-media-keystatuses.html
--------------------------------------------------------
FAIL expected TIMEOUT Verify MediaKeySession.keyStatuses.
/encrypted-media/Google/encrypted-media-session-closed-event.html
-----------------------------------------------------------------
TIMEOUT Test MediaKeySession closed event.
/encrypted-media/Google/encrypted-media-syntax.html
---------------------------------------------------
PASS expected TIMEOUT Test MediaKeySession update().
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

I need more time to figure out how to fix those unexpected results.
And I'm wondering that should "PASS expected TIMEOUT" be fixed too ?
Flags: needinfo?(kikuo)
Provide a max size check for webm initdata & keyid.
That solved the unexpected failures in Comment 5 and results in all PASS for encrypted-media-generate-request-disallowed-input.html.

I'm checking those unexpected results in Comment 8.
Blocks: 1294021
Hi Gerald,
I've fixed the unexpected results in web-platform-tests.

But pushing this patch by mozreview will generate another duplicated review request against Chris's patch. (Because I needed that Chris's patch imported to my local ...) and I don't know how to avoid that.
So I'm asking for review in this old-school way : (
Attachment #8790602 - Attachment is obsolete: true
Attachment #8791109 - Flags: review?(gsquelart)
(In reply to Kilik Kuo [:kikuo] from comment #10)
> Created attachment 8791109 [details] [diff] [review]
> [Part2] Provide max length limitation for KeyIds and Webm Initdata, then
> correct cooresponding web-platform-tests meta file.
> 
> Hi Gerald,
> I've fixed the unexpected results in web-platform-tests.

Should add, 

I've changed the expected result from "TIMEOUT" to "FAIL" in "testing/web-platform/meta/encrypted-media/Google/encrypted-media-keystatuses.html.ini".
Originally, our ClearKey implementation cannot parse these keyids correctly, so the promise was rejected and resulted to a timeout.  With Chris's patch, keyids were parsed correctly, so we'll hit the other assertion in that test.  The reason is that GMP child crashed while releasing keyId without corresponding decryptor [1].

[1] https://dxr.mozilla.org/mozilla-central/rev/82d0a583a9a39bf0b0000bccbf6d5c9ec2596bcc/media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp#127-129

I'll create a follow-up to fix that
Comment on attachment 8791109 [details] [diff] [review]
[Part2] Provide max length limitation for KeyIds and Webm Initdata, then correct cooresponding web-platform-tests meta file.

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

r+ with nit.

(Also, you could have pushed both patches to mozreview; just put 'r=cpearce' in the commit description of the first patch, and that should leave it r+'d)

::: media/gmp-clearkey/0.1/ClearKeyUtils.h
@@ +41,5 @@
>  typedef std::vector<uint8_t> Key;
>  
> +// Provide limitation for KeyIds length and webm initData size.
> +const uint32_t kMaxWebmInitDataSize = 65536;
> +const uint32_t kMaxKeyIdsLength = 512;

I think you should make these static.
Attachment #8791109 - Flags: review?(gsquelart) → review+
Comment on attachment 8792086 [details]
Bug 1289968 - [Part1] Ensure ClearKey doesn't assume keyIds can only be 16 bytes.

https://reviewboard.mozilla.org/r/79306/#review77846

Oops, it doesn't seem to work to make the 1st patch r+'d after I've modified the commit descriptions with "r=cpearce" (tried "r=gerald" too, didn't work either.)

So I pushed the review request again with the nit addressed. Sorry for bothering you again and thanks for the review : )
Comment on attachment 8792086 [details]
Bug 1289968 - [Part1] Ensure ClearKey doesn't assume keyIds can only be 16 bytes.

Carry r+ from Comment 3.
Attachment #8792086 - Flags: review?(cpearce) → review+
Comment on attachment 8792087 [details]
Bug 1289968 - [Part2] Provide max length limitation for KeyIds and Webm Initdata, then correct cooresponding web-platform-tests meta file.

Nit was addressed and carry r+ from comment 12.
Attachment #8792087 - Flags: review?(gsquelart) → review+
Comment on attachment 8792086 [details]
Bug 1289968 - [Part1] Ensure ClearKey doesn't assume keyIds can only be 16 bytes.

https://reviewboard.mozilla.org/r/79308/#review77852

Only modified the commmit message according Comment 12 (https://bugzilla.mozilla.org/show_bug.cgi?id=1289968#c12)
But it did't work to stop generating another new review request for the 1st patch which was already r+'d by gerald.
Then I found that I can change the state from review borad.
So carry r+ from Comment 3 (https://bugzilla.mozilla.org/show_bug.cgi?id=1289968#c3).
Attachment #8792087 - Flags: review+ → review?(gsquelart)
Comment on attachment 8792087 [details]
Bug 1289968 - [Part2] Provide max length limitation for KeyIds and Webm Initdata, then correct cooresponding web-platform-tests meta file.

https://reviewboard.mozilla.org/r/79310/#review77854

Nit was addressed and carry r+ from Comment 12, https://bugzilla.mozilla.org/show_bug.cgi?id=1289968#c12
Attachment #8792087 - Flags: review+
Attachment #8792086 - Flags: review?(gsquelart)
Comment on attachment 8792086 [details]
Bug 1289968 - [Part1] Ensure ClearKey doesn't assume keyIds can only be 16 bytes.

https://reviewboard.mozilla.org/r/79308/#review77852

I thought changing review state to r+ and setting r=me would lead to a "carry r+"-like result on review borad, but it turned out I still got a "r?" in the field <Landable?>.
So I think I need to get another r+ for these 2 patches again, Sorry to bother you, Gerald, hope you don't mind :(
Comment on attachment 8792086 [details]
Bug 1289968 - [Part1] Ensure ClearKey doesn't assume keyIds can only be 16 bytes.

https://reviewboard.mozilla.org/r/79308/#review77898
Attachment #8792086 - Flags: review?(gsquelart) → review+
Comment on attachment 8792087 [details]
Bug 1289968 - [Part2] Provide max length limitation for KeyIds and Webm Initdata, then correct cooresponding web-platform-tests meta file.

https://reviewboard.mozilla.org/r/79310/#review77900
Attachment #8792087 - Flags: review?(gsquelart) → review+
(In reply to Gerald Squelart [:gerald] from comment #22)
> Comment on attachment 8792087 [details]
> Bug 1289968 - [Part2] Provide max length limitation for KeyIds and Webm
> Initdata, then correct cooresponding web-platform-tests meta file.
> 
> https://reviewboard.mozilla.org/r/79310/#review77900

Thanks again !
Assignee: cpearce → kikuo
Thanks Chris and Gerald.
Keywords: checkin-needed
Comment on attachment 8791109 [details] [diff] [review]
[Part2] Provide max length limitation for KeyIds and Webm Initdata, then correct cooresponding web-platform-tests meta file.

To avoid confusion.
Attachment #8791109 - Attachment is obsolete: true
Comment on attachment 8778740 [details]
Bug 1289968 - Ensure ClearKey doesn't assume keyIds can only be 16 bytes.

To avoid confusion.
Attachment #8778740 - Attachment is obsolete: true
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c959ee59f94
[Part1] Ensure ClearKey doesn't assume keyIds can only be 16 bytes. r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c1e96d92dcf
[Part2] Provide max length limitation for KeyIds and Webm Initdata, then correct cooresponding web-platform-tests meta file. r=gerald
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3c959ee59f94
https://hg.mozilla.org/mozilla-central/rev/3c1e96d92dcf
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: