Closed Bug 1308076 Opened 8 years ago Closed 8 years ago

[EME] Web Platform Test failure in clearkey-generate-request-disallowed-input.html

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(11 files)

Bug 1308076 - Basic validation of EME initData.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1308076 - Validate keyids json format.
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1308076 - Move ClearKeyCencParser to PsshParser library.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1308076 - Don't statically link mfplat.lib into gmp-clearkey.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1308076 - Use PsshParser to validate CENC init data.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1308076 - Mark WPT encrypted-media-generate-request-disallowed-input as expected fail.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1308076 - Rename CLEARKEY_KEY_LEN to CENC_KEY_LEN.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1308076 - Fixup PSSH parser gtests.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1308076 - Consistently put expected value where it's expected in EXPECT_EQ() in Pssh Parser gtest.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1308076 - Move PSSH parser gtests into media/psshparser/gtest.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1308076 - Ensure Primetime PSSH boxes pass the PSSH validator.
58 bytes, text/x-review-board-request
jwwang
: review+
Details 
We're currently failing in the Web Platform Tests:
http://www.w3c-test.org/encrypted-media/clearkey-generate-request-disallowed-input.html

This is due to us not sanitizing and validating the init data being passed to the CDM, and us not throwing the expected exception types.
Summary: clearkey-generate-request-disallowed-input. → [EME] Web Platform Test failure in clearkey-generate-request-disallowed-input.html
Comment on attachment 8798653 [details]
Bug 1308076 - Use PsshParser to validate CENC init data.

https://reviewboard.mozilla.org/r/84036/#review82684
Attachment #8798653 - Flags: review?(jwwang) → review+
Comment on attachment 8798654 [details]
Bug 1308076 - Mark WPT encrypted-media-generate-request-disallowed-input as expected fail.

https://reviewboard.mozilla.org/r/84038/#review82686
Attachment #8798654 - Flags: review?(jwwang) → review+
Comment on attachment 8798655 [details]
Bug 1308076 - Rename CLEARKEY_KEY_LEN to CENC_KEY_LEN.

https://reviewboard.mozilla.org/r/84096/#review82688
Attachment #8798655 - Flags: review?(jwwang) → review+
Comment on attachment 8798649 [details]
Bug 1308076 - Basic validation of EME initData.

https://reviewboard.mozilla.org/r/84030/#review82760

::: dom/media/eme/MediaKeySession.cpp:179
(Diff revision 2)
> +static bool
> +ValidateInitData(const nsTArray<uint8_t>& aInitData, const nsAString& aInitDataType)
> +{
> +  if (aInitDataType.LowerCaseEqualsLiteral("webm")) {
> +    // WebM initData consists of a single keyId. Ensure it's of reasonable length.
> +    return aInitData.Length() <= 512;

Define the magic number in a static const and add a comment to show its reference in the spec.

::: dom/media/eme/MediaKeySession.cpp:231
(Diff revision 2)
>    }
>  
> +  // Let this object's uninitialized value be false.
>    mUninitialized = false;
>  
> +  // If initDataType is the empty string, return a promise rejected

is 'an' empty string.
Attachment #8798649 - Flags: review?(jwwang) → review+
Comment on attachment 8798772 [details]
Bug 1308076 - Fixup PSSH parser gtests.

https://reviewboard.mozilla.org/r/84190/#review82768
Attachment #8798772 - Flags: review?(jwwang) → review+
Comment on attachment 8798773 [details]
Bug 1308076 - Consistently put expected value where it's expected in EXPECT_EQ() in Pssh Parser gtest.

https://reviewboard.mozilla.org/r/84192/#review82770
Attachment #8798773 - Flags: review?(jwwang) → review+
Comment on attachment 8798650 [details]
Bug 1308076 - Validate keyids json format.

https://reviewboard.mozilla.org/r/84032/#review82844

Please file a spec bug to clarify the expected format. And perhaps test, if possible, what other browsers do there.

::: dom/media/eme/MediaKeySession.cpp:201
(Diff revision 2)
> +      return false;
> +    }
> +    if (!keyIds.Init(json)) {
> +      return false;
> +    }
> +    if (keyIds.mKids.Length() == 0) {

of course the spec draft is very vague, so this array length check is a guess only, I think.

::: dom/media/eme/MediaKeySession.cpp:205
(Diff revision 2)
> +    }
> +    if (keyIds.mKids.Length() == 0) {
> +      return false;
> +    }
> +    for (const auto& kid : keyIds.mKids) {
> +      if (kid.IsEmpty()) {

and this too. Nothing in the spec talks about empty string check.
Attachment #8798650 - Flags: review?(bugs) → review+
Comment on attachment 8798649 [details]
Bug 1308076 - Basic validation of EME initData.

https://reviewboard.mozilla.org/r/84030/#review82760

> is 'an' empty string.

That's text copied from the spec, so I'd rather it matched the spec so it can be searched for if needed.
(In reply to Olli Pettay [:smaug] from comment #28)
> Comment on attachment 8798650 [details]
> Bug 1308076 - Validate keyids json format.
> 
> https://reviewboard.mozilla.org/r/84032/#review82844
> 
> Please file a spec bug to clarify the expected format. And perhaps test, if
> possible, what other browsers do there.
> 
> ::: dom/media/eme/MediaKeySession.cpp:201
> (Diff revision 2)
> > +      return false;
> > +    }
> > +    if (!keyIds.Init(json)) {
> > +      return false;
> > +    }
> > +    if (keyIds.mKids.Length() == 0) {
> 
> of course the spec draft is very vague, so this array length check is a
> guess only, I think.
> 
> ::: dom/media/eme/MediaKeySession.cpp:205
> (Diff revision 2)
> > +    }
> > +    if (keyIds.mKids.Length() == 0) {
> > +      return false;
> > +    }
> > +    for (const auto& kid : keyIds.mKids) {
> > +      if (kid.IsEmpty()) {
> 
> and this too. Nothing in the spec talks about empty string check.

This behaviour is enforced by a web platform test. But yes, I'll follow up to get the spec clarified.
Comment on attachment 8799626 [details]
Bug 1308076 - Ensure Primetime PSSH boxes pass the PSSH validator.

https://reviewboard.mozilla.org/r/84768/#review83348
Attachment #8799626 - Flags: review?(jwwang) → review+
Comment on attachment 8798651 [details]
Bug 1308076 - Move ClearKeyCencParser to PsshParser library.

https://reviewboard.mozilla.org/r/84034/#review83354

::: media/psshparser/PsshParser.cpp
(Diff revision 4)
>  
> -#include "ClearKeyCencParser.h"
> +#include "PsshParser.h"
>  
>  #include "mozilla/Assertions.h"
> -#include "ArrayUtils.h"
> -#include "BigEndian.h"

Back in bug 1150437, Endian.h (the former name of BigEndian.h) was added, as it seems, so that gmp-clearkey would be somewhat self-contained. The same changeset also removed uses of other mozilla/ headers, such as mozilla/Assertions.h, which made a reentry later on, and appears in this file. If MFBT can now be used here, then I don't see why you're adding readUint32 instead of including mozilla/EndianUtils.h.
Attachment #8798651 - Flags: review?(mh+mozilla)
Comment on attachment 8798652 [details]
Bug 1308076 - Don't statically link mfplat.lib into gmp-clearkey.

https://reviewboard.mozilla.org/r/84094/#review83356
Attachment #8798652 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8798774 [details]
Bug 1308076 - Move PSSH parser gtests into media/psshparser/gtest.

https://reviewboard.mozilla.org/r/84194/#review83358
Attachment #8798774 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8798651 [details]
Bug 1308076 - Move ClearKeyCencParser to PsshParser library.

https://reviewboard.mozilla.org/r/84034/#review83362
Attachment #8798651 - Flags: review?(mh+mozilla) → review+
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9d2383c4a89a
Basic validation of EME initData. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/690638851e62
Validate keyids json format. r=smaug
https://hg.mozilla.org/integration/autoland/rev/68f1d429368c
Move ClearKeyCencParser to PsshParser library. r=glandium
https://hg.mozilla.org/integration/autoland/rev/b2ef285116b3
Don't statically link mfplat.lib into gmp-clearkey. r=glandium
https://hg.mozilla.org/integration/autoland/rev/40ce3fa30dfa
Use PsshParser to validate CENC init data. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/6281e90a180f
Mark WPT encrypted-media-generate-request-disallowed-input as expected fail. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/4ce11c2f2e69
Rename CLEARKEY_KEY_LEN to CENC_KEY_LEN. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/11fc2627544c
Fixup PSSH parser gtests. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/2ce4a30bdf86
Consistently put expected value where it's expected in EXPECT_EQ() in Pssh Parser gtest. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/5862e3ccc989
Move PSSH parser gtests into media/psshparser/gtest. r=glandium
https://hg.mozilla.org/integration/autoland/rev/6d16f00b9858
Ensure Primetime PSSH boxes pass the PSSH validator. r=jwwang
Backed out for bustage: 'mozilla::BigEndian' has not been declared:

https://hg.mozilla.org/integration/autoland/rev/5edc55eb4c9c54b45e8d43dacfd45ed302d64b00
https://hg.mozilla.org/integration/autoland/rev/3a50b5d1be7499da54f96ef73325c08f85a8f479
https://hg.mozilla.org/integration/autoland/rev/0913c44e357e8ebf08401eb05448479bc6fe5529
https://hg.mozilla.org/integration/autoland/rev/0a86b763825593f9a31bd57b7237d7b637267561
https://hg.mozilla.org/integration/autoland/rev/2b7dd0a985e58fda6cb8edc526bde7d64e315c2e
https://hg.mozilla.org/integration/autoland/rev/66dc1c89f8971a30383e9b95ccbbffcda37c78d9
https://hg.mozilla.org/integration/autoland/rev/48df10a10b9f522ce54536a56ba96b364d5c4310
https://hg.mozilla.org/integration/autoland/rev/68e333d34c7046738be602a2cbe580f787c3beab
https://hg.mozilla.org/integration/autoland/rev/c2a888e7ade0494f0e3b913677224361bc61fe9b
https://hg.mozilla.org/integration/autoland/rev/d55993c25ecea4da67b9026301817332cd2c24c5

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6d16f00b98584146ed998dd089476f44d79b77f9
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=4840410&repo=autoland

01:28:00     INFO -  In file included from /builds/slave/autoland-l64-00000000000000000/build/src/obj-firefox/media/psshparser/Unified_cpp_media_psshparser0.cpp:2:0:
01:28:00     INFO -  /builds/slave/autoland-l64-00000000000000000/build/src/media/psshparser/PsshParser.cpp: In member function 'uint32_t ByteReader::ReadU32()':
01:28:00     INFO -  /builds/slave/autoland-l64-00000000000000000/build/src/media/psshparser/PsshParser.cpp:66:21: error: 'mozilla::BigEndian' has not been declared
01:28:00     INFO -       return mozilla::BigEndian::readUint32(ptr);
01:28:00     INFO -                       ^
01:28:00     INFO -  /builds/slave/autoland-l64-00000000000000000/build/src/media/psshparser/PsshParser.cpp:67:3: error: control reaches end of non-void function [-Werror=return-type]
01:28:00     INFO -     }
01:28:00     INFO -     ^
01:28:00     INFO -  cc1plus: all warnings being treated as errors
01:28:00     INFO -  gmake[5]: *** [Unified_cpp_media_psshparser0.o] Error 1
01:28:00     INFO -  gmake[5]: Leaving directory `/builds/slave/autoland-l64-00000000000000000/build/src/obj-firefox/media/psshparser'
01:28:00     INFO -  gmake[4]: *** [media/psshparser/target] Error 2
Flags: needinfo?(cpearce)
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03d54a71b829
Basic validation of EME initData. r=jwwang
https://hg.mozilla.org/integration/mozilla-inbound/rev/873a2bd81df7
Validate keyids json format. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/01722597f70b
Move ClearKeyCencParser to PsshParser library. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/6994a09c4551
Don't statically link mfplat.lib into gmp-clearkey. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/8311203aabe2
Use PsshParser to validate CENC init data. r=jwwang
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfae967e994b
Mark WPT encrypted-media-generate-request-disallowed-input as expected fail. r=jwwang
https://hg.mozilla.org/integration/mozilla-inbound/rev/884cccc8f139
Rename CLEARKEY_KEY_LEN to CENC_KEY_LEN. r=jwwang
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cda1fca7cdc
Fixup PSSH parser gtests. r=jwwang
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd00715b29e3
Consistently put expected value where it's expected in EXPECT_EQ() in Pssh Parser gtest. r=jwwang
https://hg.mozilla.org/integration/mozilla-inbound/rev/72dd4e4ca10b
Move PSSH parser gtests into media/psshparser/gtest. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/134e6af548f5
Ensure Primetime PSSH boxes pass the PSSH validator. r=jwwang
Comment on attachment 8798653 [details]
Bug 1308076 - Use PsshParser to validate CENC init data.

https://reviewboard.mozilla.org/r/84036/#review83778

::: media/psshparser/PsshParser.cpp:137
(Diff revision 6)
>      if (!reader.CanRead32()) {
> -      return;
> +      return false;
>      }
>      uint32_t box = reader.ReadU32();
>      if (box != FOURCC('p','s','s','h')) {
> -      reader.Seek(std::max<size_t>(reader.Offset(), end));
> +      return false;

Wouldnt this cause us to fail every init data not starting with pssh box?

This used to be accepted and the parser would search for a pssh box within the init data.

only if not found or invalid would it abort...

hmm.. actually from spec:
Initialization Data is always one or more concatenated 'pssh' boxes. The CDM must be able to examine multiple 'pssh' boxes in the Initialization Data to find a 'pssh' box that it supports
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: