1287000 - [EME] Clearkey can't handle PSSH boxes with 0 size field.

Status: RESOLVED FIXED

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

Description

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

Google's Web Platform EME Tests have a CENC PSSH box with a size field of 0:

https://github.com/w3c/web-platform-tests/blob/master/encrypted-media/Google/encrypted-media-utils.js#L50

Chrome handles that, even though it's invalid, so we should too.

We actually end up in an infinite loop in the GMP process if a ClearKey encounters a PSSH box with a 0 size, so we should fix it.

Comment 1

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

Attachment: Bug 1287000 - Ensure ClearKey's CENC PSSH parser can handle PSSH boxes with a 0 size field.

Google's Web Platform EME tests contain a CENC PSSH box with a 0 size field.
Our existing PSSH parser in our ClearKey plugin doesn't handle this well, it
gets stuck in an infinite loop. We should really handle everything that Chrome
handles, so we should handle this input.

We also shouldn't really be using raw pointers in the PSSH parser.

So rewrite the PSSH parser to use a ByteReader, and handle an invalid 0 sized
common SystemID box.

Also add gtests for the parser, and skip over PSSH boxes with unknown SystemIDs
(if they have valid sizes that is).

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

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

https://reviewboard.mozilla.org/r/64410/#review61466

::: media/gmp-clearkey/0.1/ClearKeyCencParser.h:23
(Diff revision 1)
> +#define __ClearKeyCencParser_h__
> +
> +#include <stdint.h>
> +#include <vector>
> +
> +#define CLEARKEY_KEY_LEN ((size_t)16)

This macro is used in the .cpp only.

::: media/gmp-clearkey/0.1/ClearKeyCencParser.cpp:36
(Diff revision 1)
> +  ByteReader(const uint8_t* aData, size_t aSize)
> +    : mPtr(aData), mRemaining(aSize), mLength(aSize)
> +  {
> +  }
> +
> +  size_t Offset()

const

::: media/gmp-clearkey/0.1/ClearKeyCencParser.cpp:55
(Diff revision 1)
> +      return 0;
> +    }
> +    return *ptr;
> +  }
> +
> +  bool CanRead32() { return mRemaining >= 4; }

const.

::: media/gmp-clearkey/0.1/ClearKeyCencParser.cpp:96
(Diff revision 1)
> +  }
> +
> +private:
> +  const uint8_t* mPtr;
> +  size_t mRemaining;
> +  size_t mLength;

const.

::: media/gmp-clearkey/0.1/ClearKeyCencParser.cpp:119
(Diff revision 1)
> +  while (reader.CanRead32()) {
> +    // Box size. For the common system Id, ignore this, as some useragents
> +    // handle invalid box sizes.
> +    const size_t start = reader.Offset();
> +    const size_t size = reader.ReadU32();
> +    if (size > 100000) {

Shouldn't it be |if (start + size > aInitDataSize)|?

::: media/gmp-clearkey/0.1/ClearKeyCencParser.cpp:123
(Diff revision 1)
> +    const size_t size = reader.ReadU32();
> +    if (size > 100000) {
> +      // Ensure 'start + size' calculation below can't overflow.
> +      return;
> +    }
> +    const size_t end = std::min<size_t>(start + size, reader.Offset() + reader.Remaining());

It would be handy if we have |.Length()| to replace |.Offset() + .Remaining()|

::: media/gmp-clearkey/0.1/ClearKeyCencParser.cpp:131
(Diff revision 1)
> +    if (!reader.CanRead32()) {
> +      return;
> +    }
> +    uint32_t box = reader.ReadU32();
> +    if (box != FOURCC('p','s','s','h')) {
> +      reader.Seek(end);

We will seek back to |start| run into a infinite loop if |size| is 0 here, right?

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Btw, how do we know the offset of next box when the previous size field is 0? We just parse the fields one by one and assume no padding bytes before the next box?

Comment 4

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

Thanks for the review.

(In reply to JW Wang [:jwwang] from comment #2)
> ::: media/gmp-clearkey/0.1/ClearKeyCencParser.cpp:119
> (Diff revision 1)
> > +  while (reader.CanRead32()) {
> > +    // Box size. For the common system Id, ignore this, as some useragents
> > +    // handle invalid box sizes.
> > +    const size_t start = reader.Offset();
> > +    const size_t size = reader.ReadU32();
> > +    if (size > 100000) {
> 
> Shouldn't it be |if (start + size > aInitDataSize)|?

If size is UINT32_MAX, then the (start + size) calculation will overflow, and that check could fail. My intention here is to make it easy to be sure that the calculation won't overflow.

> ::: media/gmp-clearkey/0.1/ClearKeyCencParser.cpp:131
> (Diff revision 1)
> > +    if (!reader.CanRead32()) {
> > +      return;
> > +    }
> > +    uint32_t box = reader.ReadU32();
> > +    if (box != FOURCC('p','s','s','h')) {
> > +      reader.Seek(end);
> 
> We will seek back to |start| run into a infinite loop if |size| is 0 here,
> right?

Yes, good catch.

Comment 5

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

(In reply to JW Wang [:jwwang] from comment #3)
> Btw, how do we know the offset of next box when the previous size field is
> 0? We just parse the fields one by one and assume no padding bytes before
> the next box?

We don't know the offset of the next box. I'm not too concerned about being overly forgiving in the case of badly formed input, as ClearKey isn't likely to see much use in the real world.

I mainly care about the 0-sized PSSH here, as it appears in Google's EME test set.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Chris Pearce (:cpearce) from comment #4)
> If size is UINT32_MAX, then the (start + size) calculation will overflow,
> and that check could fail. My intention here is to make it easy to be sure
> that the calculation won't overflow.
As you said ClearKey isn't very useful in the real world, we can just add a comment to say 100000 is a reasonable and good magic number to handle most cases.

Comment 7

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

Comment on attachment 8771165 [details]
Bug 1287000 - Ensure ClearKey's CENC PSSH parser can handle PSSH boxes with a 0 size field.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64410/diff/1-2/

Comment 8

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

https://reviewboard.mozilla.org/r/64410/#review61466

> Shouldn't it be |if (start + size > aInitDataSize)|?

I added a proper overflow check here. For future proofing...

> We will seek back to |start| run into a infinite loop if |size| is 0 here, right?

I added a test case for this; it did indeed get stuck in an infinite loop.

I changed that to seek(max(reader.Offset(), end), so that we never go backwards.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8771165 [details]
Bug 1287000 - Ensure ClearKey's CENC PSSH parser can handle PSSH boxes with a 0 size field.

https://reviewboard.mozilla.org/r/64410/#review61564

::: media/gmp-clearkey/0.1/ClearKeyCencParser.cpp:153
(Diff revision 2)
> +      continue;
> +    }
> +    reader.Read(3); // skip flags.
> +
> +    // SystemID
> +    if (reader.Remaining() < sizeof(kSystemID)) {

It is more efficient to check if sid is null below because Read() already checks |mRemaining|.

::: media/gmp-clearkey/0.1/gtest/TestClearKeyUtils.cpp:184
(Diff revision 2)
> +  EXPECT_EQ(memcmp(&keyIds[1].front(), &gW3SpecExampleCencInitData[48], 16), 0);
> +
> +  keyIds.clear();
> +  ParseCENCInitData(gOverflowBoxSize, MOZ_ARRAY_LENGTH(gOverflowBoxSize), keyIds);
> +  EXPECT_EQ(keyIds.size(), 0u);
> +  

space.

Comment 10

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

Comment on attachment 8771165 [details]
Bug 1287000 - Ensure ClearKey's CENC PSSH parser can handle PSSH boxes with a 0 size field.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64410/diff/2-3/

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

https://reviewboard.mozilla.org/r/64410/#review61580

::: media/gmp-clearkey/0.1/ClearKeyCencParser.cpp:84
(Diff revision 3)
> +    mPtr += aCount;
> +
> +    return result;
> +  }
> +
> +  const uint8_t* Seek(size_t aOffset)

It should be OK to call Seek(Length()), right? It is still a valid position with Remaining() == 0.

::: media/gmp-clearkey/0.1/ClearKeyCencParser.cpp:176
(Diff revision 3)
> +        // Not enough remaining to read key.
> +        return;
> +      }
> +      const uint8_t* kid = reader.Read(CLEARKEY_KEY_LEN);
> +      aOutKeyIds.push_back(std::vector<uint8_t>(kid, kid + CLEARKEY_KEY_LEN));
> +    }

We should call reader.Seek(end) to process the next box in the next loop provided the size field is not 0.

Comment 12

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

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

Comment 13

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

Comment on attachment 8771165 [details]
Bug 1287000 - Ensure ClearKey's CENC PSSH parser can handle PSSH boxes with a 0 size field.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64410/diff/3-4/

Comment 14

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

(In reply to JW Wang [:jwwang] from comment #2)
> https://reviewboard.mozilla.org/r/64410/#review61466
> 
> ::: media/gmp-clearkey/0.1/ClearKeyCencParser.h:23
> (Diff revision 1)
> > +#define __ClearKeyCencParser_h__
> > +
> > +#include <stdint.h>
> > +#include <vector>
> > +
> > +#define CLEARKEY_KEY_LEN ((size_t)16)
> 
> This macro is used in the .cpp only.

It's also used in ClearKeyUtils.cpp line 58.

Comment 15

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

Comment on attachment 8771165 [details]
Bug 1287000 - Ensure ClearKey's CENC PSSH parser can handle PSSH boxes with a 0 size field.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64410/diff/4-5/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bea2a824598c
Ensure ClearKey's CENC PSSH parser can handle PSSH boxes with a 0 size field. r=jwwang

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bea2a824598c