Bug 1346648 (CVE-2017-5448)

ClearKeyDecryptor Integer Overflow Remote (ZDI-CAN-4535)

VERIFIED FIXED in Firefox -esr45

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: dveditz, Assigned: gerald)

Tracking

({csectype-intoverflow, regression, sec-high})

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox-esr4553+ verified, firefox52 wontfix, firefox-esr5253+ verified, firefox53+ verified, firefox54+ verified, firefox55+ verified)

Details

(Whiteboard: [Disclosure deadline: end of June][adv-main53+][adv-esr45.9+][adv-esr52.1+])

Attachments

(6 attachments, 7 obsolete attachments)

771.40 KB, application/zip
Details
11.97 KB, text/plain
Details
2.71 KB, patch
gerald
: review+
Details | Diff | Splinter Review
2.63 KB, patch
gerald
: review+
Details | Diff | Splinter Review
2.63 KB, patch
gerald
: review+
Details | Diff | Splinter Review
2.62 KB, patch
gerald
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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
(Reporter)

Updated

2 years ago
Whiteboard: [Disclosure deadline: end of June]
Flags: needinfo?(gsquelart)
And the line just above, `data += aMetadata.mClearBytes[i];`, could also theoretically overflow. I'll add a check there too.
Flags: needinfo?(gsquelart)
Posted patch 1346648.patch (obsolete) — Splinter Review
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).
Attachment #8846944 - Flags: review?(cpearce)
Attachment #8846944 - Flags: review?(cpearce) → review+

Comment 4

2 years ago
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

2 years ago
(In reply to q1 from comment #4)
Flags: needinfo?(gsquelart)
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?
Flags: needinfo?(gsquelart) → needinfo?(q1)

Comment 7

2 years ago
(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.
Flags: needinfo?(q1)
Posted patch 1346648.patch (obsolete) — Splinter Review
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.)
Attachment #8846944 - Attachment is obsolete: true
Attachment #8847377 - Flags: review?(cpearce)
Attachment #8847377 - Flags: feedback?(q1)
Tracking for 53 and up.

Comment 10

2 years ago
(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

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

Seems OK per previous comment.
Attachment #8847377 - Flags: feedback?(q1) → feedback+
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.
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

2 years ago
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

2 years ago
(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.
(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.
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 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.
Attachment #8847377 - Flags: review?(cpearce) → review+
Posted patch 1346648.patch (obsolete) — Splinter Review
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.
Attachment #8847377 - Attachment is obsolete: true
Attachment #8848356 - Flags: review+
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.
Attachment #8848356 - Flags: sec-approval?
Comment on attachment 8848356 [details] [diff] [review]
1346648.patch

sec-approval+ for trunk.
Attachment #8848356 - Flags: sec-approval? → sec-approval+
Posted patch 1346648.patch (obsolete) — Splinter Review
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.
Attachment #8848356 - Attachment is obsolete: true
Attachment #8848960 - Flags: sec-approval?
Attachment #8848960 - Flags: review+
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
Attachment #8848960 - Flags: approval-mozilla-aurora?
Posted patch 1346648-beta53.patch (obsolete) — Splinter Review
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
Attachment #8848962 - Flags: review+
Attachment #8848962 - Flags: approval-mozilla-beta?
Posted patch 1346648-esr52.patch (obsolete) — Splinter Review
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
Attachment #8848963 - Flags: review+
Attachment #8848963 - Flags: approval-mozilla-beta?
Posted patch 1346648-esr45.patch (obsolete) — Splinter Review
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.
Attachment #8848965 - Flags: review+
Attachment #8848965 - Flags: approval-mozilla-esr45?
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.
Attachment #8848963 - Flags: approval-mozilla-beta? → approval-mozilla-esr52?
Comment on attachment 8848960 [details] [diff] [review]
1346648.patch

sec-approval+ and beta and aurora approval.
Attachment #8848960 - Flags: sec-approval?
Attachment #8848960 - Flags: sec-approval+
Attachment #8848960 - Flags: approval-mozilla-aurora?
Attachment #8848960 - Flags: approval-mozilla-aurora+
Attachment #8848962 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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)
Flags: needinfo?(gsquelart)
same also with the beta patch
Re-export of nightly&aurora patch.
r+ from comment 18.
sec+ in comment 21.
aurora+ in comment 28.
Attachment #8848960 - Attachment is obsolete: true
Attachment #8849290 - Flags: review+
Re-export of beta patch.
r+ from comment 18.
sec+ in comment 21.
beta+ after comment 28.
Attachment #8848962 - Attachment is obsolete: true
Flags: needinfo?(gsquelart)
Attachment #8849292 - Flags: review+
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.
Attachment #8848963 - Attachment is obsolete: true
Attachment #8849293 - Flags: review+
Attachment #8849293 - Flags: approval-mozilla-esr52?
Attachment #8848963 - Flags: approval-mozilla-esr52?
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.
Attachment #8848965 - Attachment is obsolete: true
Attachment #8848965 - Flags: approval-mozilla-esr45?
Attachment #8849294 - Flags: review+
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.
Attachment #8849290 - Flags: sec-approval?
(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?
Flags: needinfo?(cbook)
No, it doesn't need approvals again if it's just a rebase.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d45b0f72d98a
Flags: needinfo?(cbook)
Yeah, I'll clear the sec-approval.
Attachment #8849290 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/d45b0f72d98a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Attachment #8849294 - Flags: approval-mozilla-esr45?
Comment on attachment 8849293 [details] [diff] [review]
1346648-esr52.patch

sec-high fix in ClearKeyDecryptor, esr52+
Attachment #8849293 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment on attachment 8849294 [details] [diff] [review]
1346648-esr45.patch

sec-high fix, esr45+
Attachment #8849294 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
(Reporter)

Updated

2 years ago
Group: media-core-security → core-security-release
Whiteboard: [Disclosure deadline: end of June] → [Disclosure deadline: end of June][adv-main53+][adv-esr45.9+][adv-esr52.1+]
Alias: CVE-2017-5448
Flagging this for manual testing, per Comment 25.
Flags: qe-verify+
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).
(Reporter)

Updated

2 years ago
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.