1346648 - (CVE-2017-5448) ClearKeyDecryptor Integer Overflow Remote (ZDI-CAN-4535)

Status: VERIFIED FIXED

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

Description

[Daniel Veditz [:dveditz]]

Attachment: zipped POC (password is ZDI-CAN-4535)

We received the following report from Trending Micro's Zero Day
Initiative (ZDI):

ZDI-CAN-4535: Mozilla Firefox ClearKeyDecryptor Integer Overflow Remote
Code Execution Vulnerability

-- CVSS -----------------------------------------

5.1, AV:N/AC:H/Au:N/C:P/I:P/A:P

-- VULNERABILITY DETAILS ------------------------

* Version Tested: Firefox Nightly, changeset 341105:e677ba018b22, and
general release Firefox 51.0.1 (32-bit)
* Platform Tested: Windows 10 Enterprise 14393.693

This is a failed patch of the vulnerability we originally reported to
you as ZDI-CAN-3766 in May 2016.

The malicious value is found at offset +0x555 from the start of video1.m4s.

Relevant source code (from
mozilla-central\media\gmp-clearkey\0.1\clearkeydecryptionmanager.cpp):

```
Status
ClearKeyDecryptor::Decrypt(uint8_t* aBuffer, uint32_t aBufferSize,
                           const CryptoMetaData& aMetadata)
{
  CK_LOGD("ClearKeyDecryptor::Decrypt");
  // If the sample is split up into multiple encrypted subsamples, we
need to
  // stitch them into one continuous buffer for decryption.
  std::vector<uint8_t> tmp(aBufferSize);

  if (aMetadata.NumSubsamples()) {
    // Take all encrypted parts of subsamples and stitch them into one
    // continuous encrypted buffer.
    uint8_t* data = aBuffer;
    uint8_t* iter = &tmp[0];
    for (size_t i = 0; i < aMetadata.NumSubsamples(); i++) {
      data += aMetadata.mClearBytes[i];
      uint32_t cipherBytes = aMetadata.mCipherBytes[i];
      if (data + cipherBytes > aBuffer + aBufferSize) {
   <==== Integer addition data + cipherBytes may wrap around,
        // Trying to read past the end of the buffer!
                   bypassing the check
        return Status::kDecryptError;
      }

      memcpy(iter, data, cipherBytes);
          <==== BANG
```

[--- snipped debug log, moved to attachment ---]

-- CREDIT ---------------------------------------

This vulnerability was discovered by:

   Anonymous working with Trend Micro's Zero Day Initiative

Comment 1

[Daniel Veditz [:dveditz]]

Attachment: Debug output included in the ZDI notification

Comment 2

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

And the line just above, `data += aMetadata.mClearBytes[i];`, could also theoretically overflow. I'll add a check there too.

Comment 3

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

Attachment: 1346648.patch

For both `data + bytes` computations, first check that data+bytes does not overflow before checking that the result is still inside the data buffer.

Checked locally on a win32 build (PoC overflow is caught), and a mac64 build (no overflow in this case, but we still handle the OOB correctly).

Comment 4

[q1]

Thanks for CC-ing me about this interesting overflow variant. I'll add it to my bad-things-to-scan-for list.

BTW, expressions like

   (data >= reinterpret_cast<uint8_t*>(UINTPTR_MAX) - clearBytes)

are icky. Imagine what happens if someone changes

   uint32_t clearBytes

to

   uint64_t clearBytes

and builds the thing for a 32-bit platform.

Ah, along the same lines, |uint32_t clearBytes| (and cipherBytes) both can cause truncation, because |mClearBytes| and |mCipherBytes| are both std::vector<> objects, which can contain something less than |size_t| items, which is > 4G-items on x64 platforms.

Comment 5

[q1]

(In reply to q1 from comment #4)

Comment 6

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

Thank you for the feedback, q1.

mClearBytes is a vector<uint32_t> and we're accessing its elements, therefore clearBytes is really uint32_t.
(The vector's length is only used to stop the loop, and the loop counter is an appropriate size_t.)

Now, I guess it could hurt if mClearBytes was changed to vector<uint64_t> or other.
So I could make clearBytes `auto`, with a `static_assert(sizeof(clearBytes) <= sizeof(uint8_t*), "...")` to keep the subtraction safe.

Or would you suggest something else?

Comment 7

[q1]

(In reply to Gerald Squelart [:gerald] from comment #6)
> Thank you for the feedback, q1.
> 
> mClearBytes is a vector<uint32_t> and we're accessing its elements,
> therefore clearBytes is really uint32_t.
> (The vector's length is only used to stop the loop, and the loop counter is
> an appropriate size_t.)

I misread the code. Belay that comment!

> Now, I guess it could hurt if mClearBytes was changed to vector<uint64_t> or
> other.

Yes.

> So I could make clearBytes `auto`, with a `static_assert(sizeof(clearBytes)
> <= sizeof(uint8_t*), "...")` to keep the subtraction safe.
> 
> Or would you suggest something else?

That solution seems OK, though CheckedInt would be better (if it can handle pointers) since it requires less special-case code.

Comment 8

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

Attachment: 1346648.patch

Thanks to q1's suggestions, I'm now using CheckedInt to perform pointer operations and checks, and using `auto` variables where appropriate, to automatically accommodate possible future type changes.

Note: CheckedInt being made for integer numbers, a few ugly reinterpret_cast's are needed to convert from/to pointers, but I'm using uintptr_t, which should always match pointer types.
(Though theoretically, uintptr_t could be wider than a pointer, so I've added a static_assert to make sure it's not the case on the platforms we use.)

Comment 9

[Julien Cristau [:jcristau]]

Tracking for 53 and up.

Comment 10

[q1]

(In reply to Gerald Squelart [:gerald] from comment #8)
> Created attachment 8847377 [details] [diff] [review]
> 1346648.patch

It looks OK. I puzzled for a moment over whether ClearKeyUtils::DecryptAES() properly handles |aData.size() % CENC_KEY_LEN != 0|, but it seems to.

Comment 11

[q1]

Comment on attachment 8847377 [details] [diff] [review]
1346648.patch

Seems OK per previous comment.

Comment 12

[Mike Hommey [:glandium]]

So this is where bug 1347384 comes from. It seems to me this should just use something like RangePtr instead of trying to do pointer arithmetics. If RangePtr is lacking API wise to convert the code, it should arguably be augmented.

Comment 13

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

Thank you for the feedback, q1, much appreciated.


Thank you Mike, RangePtr does look interesting... But!...
AFAICT it only performs range checks in debug builds; I need checks in all channels, all the time. And I need them not to crash, but to give me a visible good/bad state, like CheckedInt.

So RangePtr is more like a developer's tool to find coding errors early through crash reports.
What I need here is a proper range checker that will never crash, but give me ways to perform pointer operations, find out if they fail, and give me access to successful results.

Tangentially, I think RangePtr currently relies on UB to check for overflows, e.g. `asUintptr() + aInc * sizeof(T) >= asUintptr()`, which an eager compiler would be allowed to remove, because "obviously" adding a positive number should always result in a greater number! :-)
(If I'm incorrect about this assumption, please tell me, I'd like to know.)


So I'm thinking of doing the following:
- Assuming I get r+ from cpearce, I will land this patch as-is, so we get a fix out ASAP.
- I will open a new bug to review the checks in RangePtr, to remove UB if present.
- I'll add a note about RangePtr in bug 1347384, in case we can re-use that code instead of CheckedInt.

Please let me know if you have any comments about these actions; or other suggestions.

Comment 14

[q1]

On more thought, I think there are some situations (e.g. segmented-pointer architectures) where the patch doesn't work. See https://bugzilla.mozilla.org/show_bug.cgi?id=1347384#c10 .

Comment 15

[q1]

(In reply to Gerald Squelart [:gerald] from comment #13)
> Thank you for the feedback, q1, much appreciated.
> ...

You're welcome.
 
> So I'm thinking of doing the following:
> - Assuming I get r+ from cpearce, I will land this patch as-is, so we get a
> fix out ASAP.

A good idea. I don't suppose FF supports any segmented-pointer architectures at present? In any case, the fix only makes things better for non-segmented-pointer architectures.

> - I will open a new bug to review the checks in RangePtr, to remove UB if
> present.

Seems good.

Comment 16

[Jeff Walden [:Waldo]]

(In reply to Gerald Squelart [:gerald] from comment #13)
> Tangentially, I think RangePtr currently relies on UB to check for
> overflows, e.g. `asUintptr() + aInc * sizeof(T) >= asUintptr()`, which an
> eager compiler would be allowed to remove, because "obviously" adding a
> positive number should always result in a greater number! :-)
> (If I'm incorrect about this assumption, please tell me, I'd like to know.)

Adding a positive number to a positive number may overflow, but C/C++ guarantees wraparound semantics.  As long as the multiplication itself can't overflow -- which it can't because of a separate multiplication-won't-overflow assertion you didn't quote -- that check will properly detect overflow.

Comment 17

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

Thanks Jeff, good to know!

I've found the relevant paragraph 3.9.1-4:
"Unsigned integers, declared unsigned, shall obey the laws of arithmetic modulo 2n where n is the number of bits in the value representation of that particular size of integer."

Comment 18

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

Comment on attachment 8847377 [details] [diff] [review]
1346648.patch

Review of attachment 8847377 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp
@@ +225,5 @@
>          // Trying to read past the end of the buffer!
> +        MOZ_ASSERT(false);
> +        return Status::kDecryptError;
> +      }
> +      auto cipherBytes = aMetadata.mCipherBytes[i];

I would prefer to have the explicit type here rather than 'auto', as that makes the code clearer and easier to reason about; it makes it obvious which values here are CheckedInt and which aren't.

Comment 19

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

Attachment: 1346648.patch

After discussing with cpearce, I'm changing `auto cipherBytes` into `const uint32_t& cipherBytes`: No auto, known type, and the reference will prevent unexpected type changes.
Removing the 2nd auto too to expose the CheckedInt type of dataAfterCipher.

Carrying r+ from comment 18.

Comment 20

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

Comment on attachment 8848356 [details] [diff] [review]
1346648.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
For someone clued enough, the patch points at the possible address overflow due to adding the number of cipher bytes, which is a 32-bit value inside an MP4, and could easily be changed; this allows writing arbitrary data anywhere before the buffer.
But after that, really exploiting it probably requires good skills, to be able to run that injected code.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The check-in comment mentions "pointer checks", but that should be obvious from the code anyway.

Which older supported branches are affected by this flaw?
If not all supported branches, which bug introduced the flaw?
All supported branches are affected: The original code landed in FF 35, bug 1044742.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should apply as-is to esr45, esr52, beta53, and aurora54; and release52 if we want it too. (I'll verify that next and request uplifts soon.)

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely, it's only adding overflow checks.
I've run media tests and the PoC locally on Mac, linux, Windows.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8848356 [details] [diff] [review]
1346648.patch

sec-approval+ for trunk.

Comment 22

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

Attachment: 1346648.patch

Previous patch still contained `MOZ_ASSERT(false);` lines I used when debugging, sorry about that.

Carrying r+ from comment 18.

Re-requesting sec approval like comment 20 that was approved in comment 21.

Comment 23

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

Comment on attachment 8848960 [details] [diff] [review]
1346648.patch

This patch applies as-is to Aurora, but will need to be rebased for beta and earlier versions.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1044742
[User impact if declined]: Potential crashes or exploits when playing clearkey-encrypted media
[Is this code covered by automated tests?]: Yes, gtests and media mochitests
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: If yes: Load PoC file from comment 0 in win32 build, without the patch the clearkey plugin should crash (a drop-down notification will indicate that), and with the patch it will not crash anymore
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: I don't believe so
[Why is the change risky/not risky?]: It only adds extra pointer-overflow checks
[String changes made/needed]: None

Comment 24

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

Attachment: 1346648-beta53.patch

Trivial rebase to beta-53.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1044742
[User impact if declined]: Potential crashes/exploits when playing clearkey-encrypted media
[Is this code covered by automated tests?]: Yes, gtests and media mochitests
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: If yes: Load PoC file from comment 0, without the patch the clearkey plugin should crash, and not crash with the patch (but just show that the file is corrupt
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: I don't believe so
[Why is the change risky/not risky?]: It only adds extra pointer-overflow checks
[String changes made/needed]: None

Comment 25

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

Attachment: 1346648-esr52.patch

Almost-trivial rebase to ESR-52 (return value has changed name.)
Could also be applied to release-52 if wanted.

[Feature/Bug causing the regression]: Bug 1044742
[User impact if declined]: Potential crashes/exploits when playing clearkey-encrypted media
[Is this code covered by automated tests?]: Yes, gtests and media mochitests
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: If yes: Load PoC file from comment 0, without the patch the clearkey plugin should crash, and not crash with the patch (but just show that the file is corrupt
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: I don't believe so
[Why is the change risky/not risky?]: It only adds extra pointer-overflow checks
[String changes made/needed]: None

Comment 26

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

Attachment: 1346648-esr45.patch

Trivial rebase of ESR-52 patch to ESR-45.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: Potential crashes/exploits when playing clearkey-encrypted media
Fix Landed on Version: Not yet landed, requested for n55, a54, b53, esr52.
Risk to taking this patch (and alternatives if risky): I don't believe there are risks, it only adds extra pointer-overflow checks
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 27

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

Comment on attachment 8848963 [details] [diff] [review]
1346648-esr52.patch

(Sorry, picked the wrong approval in comment 25)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: Potential crashes/exploits when playing clearkey-encrypted media
Fix Landed on Version: Not yet landed, requested for n55, a54, b53, esr45.
Risk to taking this patch (and alternatives if risky): I don't believe there are risks, it only adds extra pointer-overflow checks
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8848960 [details] [diff] [review]
1346648.patch

sec-approval+ and beta and aurora approval.

Comment 29

[Carsten Book [:Tomcat]]

hi gerald, could you take a look at the aurora patch, this failed for me with 
applying /sheriffs/patch/1346648.patch
abort: bad hunk #2 @@ -206,29 +208,40 @@ ClearKeyDecryptor::Decrypt(uint8_t* aBuf
 (29 27 38 38)

Comment 30

[Carsten Book [:Tomcat]]

same also with the beta patch

Comment 31

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

Attachment: 1346648-nightly55-aurora54.patch

Re-export of nightly&aurora patch.
r+ from comment 18.
sec+ in comment 21.
aurora+ in comment 28.

Comment 32

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

Attachment: 1346648-beta53.patch

Re-export of beta patch.
r+ from comment 18.
sec+ in comment 21.
beta+ after comment 28.

Comment 33

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

Attachment: 1346648-esr52.patch

Re-export of esr52 patch.
r+ from comment 18.
sec+ in comment 21.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: Potential crashes/exploits when playing clearkey-encrypted media
Fix Landed on Version: Not yet landed, requested for n55, a54, b53, esr45.
Risk to taking this patch (and alternatives if risky): I don't believe there are risks, it only adds extra pointer-overflow checks
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 34

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

Attachment: 1346648-esr45.patch

Re-export of esr45 patch.
r+ from comment 18.
sec+ in comment 21.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: Potential crashes/exploits when playing clearkey-encrypted media
Fix Landed on Version: Not yet landed, requested for n55, a54, b53, esr52.
Risk to taking this patch (and alternatives if risky): I don't believe there are risks, it only adds extra pointer-overflow checks
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 35

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

Comment on attachment 8849290 [details] [diff] [review]
1346648-nightly55-aurora54.patch

Bugzilla thinks sec-approval is needed again:
(Same as in comment 20, that was granted in comment 21. Thanks in advance.)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
For someone clued enough, the patch points at the possible address overflow due to adding the number of cipher bytes, which is a 32-bit value inside an MP4, and could easily be changed; this allows writing arbitrary data anywhere before the buffer.
But after that, really exploiting it probably requires good skills, to be able to run that injected code.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The check-in comment mentions "pointer checks", but that should be obvious from the code anyway.

Which older supported branches are affected by this flaw?
If not all supported branches, which bug introduced the flaw?
All supported branches are affected: The original code landed in FF 35, bug 1044742.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should apply as-is to esr45, esr52, beta53, and aurora54; and release52 if we want it too. (I'll verify that next and request uplifts soon.)

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely, it's only adding overflow checks.
I've run media tests and the PoC locally on Mac, linux, Windows.

Comment 36

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

(In reply to Carsten Book [:Tomcat] from comment #29)
> hi gerald, could you take a look at the aurora patch, this failed for me

Sorry about that, I shouldn't have edited the patch directly to remove my debugging lines.

I've uploaded updates for all patches, properly exported now.
Are aurora & beta requests needed again?

Comment 37

[Ryan VanderMeulen [:RyanVM]]

No, it doesn't need approvals again if it's just a rebase.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d45b0f72d98a

Comment 38

[Al Billings [:abillings - ex-MoCo]]

Yeah, I'll clear the sec-approval.

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9e43e1f59479

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d1a93263a03d

Comment 42

[Julien Cristau [:jcristau]]

Comment on attachment 8849293 [details] [diff] [review]
1346648-esr52.patch

sec-high fix in ClearKeyDecryptor, esr52+

Comment 43

[Julien Cristau [:jcristau]]

Comment on attachment 8849294 [details] [diff] [review]
1346648-esr45.patch

sec-high fix, esr45+

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/6684a3c7f834

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/9369ede30cc1

Comment 46

[Andrei Vaida [:avaida]]

Flagging this for manual testing, per Comment 25.

Comment 47

[Paul Silaghi, QA [:pauly]]

Reproduced on Fx 52, Win 10 x64.
Verified fixed Fx 53b10, 54.0a2 (2017-04-10), 55.0a1 (2017-04-10), 45.8.1 ESR tinderbox (2017-04-10), 52.0.3 ESR tinderbox (2017-04-10).