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

Chris Pearce [:cpearce (Not reading bugmail)]

Assignee

Description

•

9 years ago

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.

Chris Pearce [:cpearce (Not reading bugmail)]

Assignee

Updated

•

9 years ago

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

Chris Pearce [:cpearce (Not reading bugmail)]

Assignee

Comment 1

•

9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe68bdc5ce9a

Comment hidden (mozreview-request)

JW Wang [:jwwang] [:jw_wang]

Comment 12

•

9 years ago

mozreview-review

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+

JW Wang [:jwwang] [:jw_wang]

Comment 13

•

9 years ago

mozreview-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+

JW Wang [:jwwang] [:jw_wang]

Comment 14

•

9 years ago

mozreview-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 hidden (mozreview-request)

JW Wang [:jwwang] [:jw_wang]

Comment 25

•

9 years ago

mozreview-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+

JW Wang [:jwwang] [:jw_wang]

Comment 26

•

9 years ago

mozreview-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+

JW Wang [:jwwang] [:jw_wang]

Comment 27

•

9 years ago

mozreview-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+

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 28

•

9 years ago

mozreview-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+

Chris Pearce [:cpearce (Not reading bugmail)]

Assignee

Comment 29

•

9 years ago

mozreview-review-reply

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.

Chris Pearce [:cpearce (Not reading bugmail)]

Assignee

Comment 30

•

9 years ago

(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 hidden (mozreview-request)

JW Wang [:jwwang] [:jw_wang]

Comment 52

•

9 years ago

mozreview-review

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+

Mike Hommey [:glandium]

Comment 53

•

9 years ago

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

Mike Hommey [:glandium]

Comment 54

•

9 years ago

mozreview-review

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+

Mike Hommey [:glandium]

Comment 55

•

9 years ago

mozreview-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 hidden (mozreview-request)

Mike Hommey [:glandium]

Comment 65

•

9 years ago

mozreview-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+

Pulsebot

Comment 66

•

9 years ago

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

Comment 67

•

9 years ago

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)

Pulsebot

Comment 68

•

9 years ago

Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/00004bebfe00 Backed out changeset 690638851e62

Pulsebot

Comment 69

•

9 years ago

Carsten Book [:Tomcat]

Comment 70

•

9 years ago

bugherder

Status: NEW → RESOLVED

Closed: 9 years ago

status-firefox52: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla52

Jean-Yves Avenard [:jya]

Comment 71

•

9 years ago

mozreview-review

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

Chris Pearce [:cpearce (Not reading bugmail)]

Assignee

Updated

•

9 years ago

Flags: needinfo?(cpearce)

Bug 1308076 - Basic validation of EME initData. 9 years ago Chris Pearce [:cpearce (Not reading bugmail)] 58 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1308076 - Validate keyids json format. 9 years ago Chris Pearce [:cpearce (Not reading bugmail)] 58 bytes, text/x-review-board-request	smaug : review+	Details
Bug 1308076 - Move ClearKeyCencParser to PsshParser library. 9 years ago Chris Pearce [:cpearce (Not reading bugmail)] 58 bytes, text/x-review-board-request	glandium : review+	Details
Bug 1308076 - Don't statically link mfplat.lib into gmp-clearkey. 9 years ago Chris Pearce [:cpearce (Not reading bugmail)] 58 bytes, text/x-review-board-request	glandium : review+	Details
Bug 1308076 - Use PsshParser to validate CENC init data. 9 years ago Chris Pearce [:cpearce (Not reading bugmail)] 58 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1308076 - Mark WPT encrypted-media-generate-request-disallowed-input as expected fail. 9 years ago Chris Pearce [:cpearce (Not reading bugmail)] 58 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1308076 - Rename CLEARKEY_KEY_LEN to CENC_KEY_LEN. 9 years ago Chris Pearce [:cpearce (Not reading bugmail)] 58 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1308076 - Fixup PSSH parser gtests. 9 years ago Chris Pearce [:cpearce (Not reading bugmail)] 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. 9 years ago Chris Pearce [:cpearce (Not reading bugmail)] 58 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1308076 - Move PSSH parser gtests into media/psshparser/gtest. 9 years ago Chris Pearce [:cpearce (Not reading bugmail)] 58 bytes, text/x-review-board-request	glandium : review+	Details
Bug 1308076 - Ensure Primetime PSSH boxes pass the PSSH validator. 9 years ago Chris Pearce [:cpearce (Not reading bugmail)] 58 bytes, text/x-review-board-request	jwwang : review+	Details