1289968 - [EME] WebM initData can have keyIds of arbitrary length

Status: RESOLVED FIXED

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

Description

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

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

Comment 1

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

InitData of type keyIds can also have keys of arbitrary length.

Comment 2

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

Attachment: Bug 1289968 - Ensure ClearKey doesn't assume keyIds can only be 16 bytes.

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/

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4361c6f3b915
Ensure ClearKey doesn't assume keyIds can only be 16 bytes. r=gerald

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 6

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

Blake: Can you find someone to get the failures here fixed and landed?

Comment 7

[Blake Wu [:bwu][:blakewu]]

(In reply to Chris Pearce (:cpearce) from comment #6)
> Blake: Can you find someone to get the failures here fixed and landed?
Kilik!

Comment 8

[Kilik Kuo [:kikuo] (inactive)]

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 ?

Comment 9

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [WIP] Check the max value for webm initdata size & keyid size.

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.

Comment 10

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [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.

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 : (

Comment 11

[Kilik Kuo [:kikuo] (inactive)]

(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 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 13

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1289968 - [Part1] Ensure ClearKey doesn't assume keyIds can only be 16 bytes.

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

Comment 14

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1289968 - [Part2] Provide max length limitation for KeyIds and Webm Initdata, then correct cooresponding web-platform-tests meta file.

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

Comment 15

[Kilik Kuo [:kikuo] (inactive)]

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 16

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8792086 [details]
Bug 1289968 - [Part1] Ensure ClearKey doesn't assume keyIds can only be 16 bytes.

Carry r+ from Comment 3.

Comment 17

[Kilik Kuo [:kikuo] (inactive)]

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.

Comment 18

[Kilik Kuo [:kikuo] (inactive)]

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).

Comment 19

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 20

[Kilik Kuo [:kikuo] (inactive)]

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 21

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

Comment 22

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

Comment 23

[Kilik Kuo [:kikuo] (inactive)]

(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 !

Comment 24

[Kilik Kuo [:kikuo] (inactive)]

A follow-up is created.  https://bugzilla.mozilla.org/show_bug.cgi?id=1303662

Comment 25

[Kilik Kuo [:kikuo] (inactive)]

Try run, test all web-platform-tests & mochitest-media.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c51f7fdfcf7b&selectedJob=27618982

Comment 26

[Kilik Kuo [:kikuo] (inactive)]

Thanks Chris and Gerald.

Comment 27

[Kilik Kuo [:kikuo] (inactive)]

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.

Comment 28

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8778740 [details]
Bug 1289968 - Ensure ClearKey doesn't assume keyIds can only be 16 bytes.

To avoid confusion.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
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

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c959ee59f94
https://hg.mozilla.org/mozilla-central/rev/3c1e96d92dcf