Closed Bug 1287000 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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/
Attachment #8771165 - Flags: review?(jwwang)
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?
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?
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.
(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.
(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 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/
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 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.
Attachment #8771165 - Flags: review?(jwwang) → review+
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/
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 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/
(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 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/
Pushed by cpearce@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/bea2a824598c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.