Write beyond bounds in Key::EncodeAsString()

RESOLVED FIXED in Firefox 55

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: q1, Assigned: bevis)

Tracking

({csectype-bounds, sec-critical})

53 Branch
mozilla56
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 wontfix, firefox55+ verified, firefox56+ fixed)

Details

(Whiteboard: [adv-main55+][post-critsmash-triage])

Attachments

(4 attachments, 2 obsolete attachments)

Posted file bug_897_poc_2.htm
Key::EncodeAsString() (dom\indexedDB\key.cpp) can cause an integer overflow. When it does, it writes ~ 4GB of partially attacker-provided data beyond the end of its |mBuffer| array.

The attached POC demonstrates the overwrite.

The bug is in line 454, which does not check whether the addition overflows. There is also a similar bug on the "+=" in line 447 that might be caused to overflow with similar results,

433: template <typename T>
434: void
435: Key::EncodeAsString(const T* aStart, const T* aEnd, uint8_t aType)
436: {
437:   // First measure how long the encoded string will be.
438: 
439:   // The +2 is for initial 3 and trailing 0. We'll compensate for multi-byte
440:   // chars below.
441:   uint32_t size = (aEnd - aStart) + 2;
442:   
443:   const T* start = aStart;
444:   const T* end = aEnd;
445:   for (const T* iter = start; iter < end; ++iter) {
446:     if (*iter > ONE_BYTE_LIMIT) {
447:       size += char16_t(*iter) > TWO_BYTE_LIMIT ? 2 : 1;
448:     }
449:   }
450: 
451:   // Allocate memory for the new size
452:   uint32_t oldLen = mBuffer.Length();
453:   char* buffer;
454:   if (!mBuffer.GetMutableData(&buffer, oldLen + size)) {
455:     return;
456:   }
457:   buffer += oldLen;
458: 
459:   // Write type marker
460:   *(buffer++) = aType;
461: 
462:   // Encode string
463:   for (const T* iter = start; iter < end; ++iter) {
464:     if (*iter <= ONE_BYTE_LIMIT) {
465:       *(buffer++) = *iter + ONE_BYTE_ADJUST;
466:     }
467:     else if (char16_t(*iter) <= TWO_BYTE_LIMIT) {
468:       char16_t c = char16_t(*iter) + TWO_BYTE_ADJUST + 0x8000;
469:       *(buffer++) = (char)(c >> 8);
470:       *(buffer++) = (char)(c & 0xFF);
471:     }
472:     else {
473:       uint32_t c = (uint32_t(*iter) << THREE_BYTE_SHIFT) | 0x00C00000;
474:       *(buffer++) = (char)(c >> 16);
475:       *(buffer++) = (char)(c >> 8);
476:       *(buffer++) = (char)c;
477:     }
478:   }
479: 
480:   // Write end marker
481:   *(buffer++) = eTerminator;
482:   
483:   NS_ASSERTION(buffer == mBuffer.EndReading(), "Wrote wrong number of bytes");
484: }

Use the POC by expanding the .7z file into the corresponding ".gz" file [1], and saving it and the .htm file to a webserver. Start 64-bit FF, attach a debugger to it, and set a BP on line 454.

Load the .htm file in FF and wait for a few minutes for it to load.

The BP will trigger twice. Ignore the first hit. On the second hit, step the assembly code for line 454 and watch the quantity |oldLen + size| overflow to 4. Then step into the |for| loop beginning on line 463 and watch it write data determined by the .gz file's contents (interspersed with 0s) far beyond |mBuffer|'s end.

[1] (it is really just a file of bytes of 0x7f)
Posted file bug_897_poc_2.7z
CCing Jan, the IndexedDB module owner.
He's also active in vulnsmash EU, so I know he'll be interested to look at this.
Flags: sec-bounty?
BTW:

   441:   uint32_t size = (aEnd - aStart) + 2;

also theoretically can truncate -- with similarly bad results -- on 64-bit builds, since |aEnd| and |aStart| are 64-bit quantities.
Group: core-security → dom-core-security
This sounds bad...
Jan and Bevis can probably take a look.
Flags: needinfo?(jvarga)
Flags: needinfo?(btseng)
Priority: -- → P1
I'd like to take this bug to follow up.
Assignee: nobody → btseng
Status: NEW → ASSIGNED
Flags: needinfo?(btseng)
tracking as sec-critical.
Identify potential overflow in Key::EncodeAsString() and return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR.
Attachment #8879099 - Attachment is obsolete: true
Flags: needinfo?(jvarga)
Attachment #8879486 - Flags: review?(jvarga)
Jan, could you take a look at this patch?
Flags: needinfo?(jvarga)
I must finish something for FF 57, could you please wait a week or so or find someone else, for example :asuth or :baku
Flags: needinfo?(jvarga)
Could one of you review this patch? Thanks.
Flags: needinfo?(bugmail)
Flags: needinfo?(amarchesini)
Comment on attachment 8879486 [details] [diff] [review]
(v2) Patch: Fix Overflow in Key::EncodeAsString()

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

::: dom/indexedDB/Key.cpp
@@ +449,4 @@
>    const T* end = aEnd;
>    for (const T* iter = start; iter < end; ++iter) {
>      if (*iter > ONE_BYTE_LIMIT) {
> +      if (char16_t(*iter) > TWO_BYTE_LIMIT) {

make size a CheckedUint32 and use isValid()

@@ +467,5 @@
>  
>    // Allocate memory for the new size
>    uint32_t oldLen = mBuffer.Length();
> +
> +  if (NS_WARN_IF(UINT32_MAX - oldLen < size)) {

Can you use CheckedUint32 checkSize = size;
checkSize += oldLen;

if (!checkedSize.isValid()) {
  ...
Attachment #8879486 - Flags: review?(jvarga) → review-
Flags: needinfo?(bugmail)
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini [:baku] from comment #13)
> Comment on attachment 8879486 [details] [diff] [review]
> (v2) Patch: Fix Overflow in Key::EncodeAsString()
> 
> Review of attachment 8879486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/indexedDB/Key.cpp
> @@ +449,4 @@
> >    const T* end = aEnd;
> >    for (const T* iter = start; iter < end; ++iter) {
> >      if (*iter > ONE_BYTE_LIMIT) {
> > +      if (char16_t(*iter) > TWO_BYTE_LIMIT) {
> 
> make size a CheckedUint32 and use isValid()
> 
> @@ +467,5 @@
> >  
> >    // Allocate memory for the new size
> >    uint32_t oldLen = mBuffer.Length();
> > +
> > +  if (NS_WARN_IF(UINT32_MAX - oldLen < size)) {
> 
> Can you use CheckedUint32 checkSize = size;
> checkSize += oldLen;
> 
> if (!checkedSize.isValid()) {
>   ...

Thanks for advising better options. I'll address this in next update!
Adopt CheckedUint32 suggested in comment 13 to fix the overflow problem.
Attachment #8879486 - Attachment is obsolete: true
Attachment #8887821 - Flags: review?(amarchesini)
Comment on attachment 8887821 [details] [diff] [review]
(v3) Patch: Fix Overflow in Key::EncodeAsString()

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

::: dom/indexedDB/Key.cpp
@@ +444,4 @@
>  
>    // The +2 is for initial 3 and trailing 0. We'll compensate for multi-byte
>    // chars below.
> +  CheckedUint32 size = (aEnd - aStart) + 2;

Just to be 100% safe, do the +2 separate:

CheckedUint32 size = aEnd - aStart;
size +=2;
if (!size.isValid()) {
  ...
}
Attachment #8887821 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #16)
> Comment on attachment 8887821 [details] [diff] [review]
> (v3) Patch: Fix Overflow in Key::EncodeAsString()
> 
> Review of attachment 8887821 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/indexedDB/Key.cpp
> @@ +444,4 @@
> >  
> >    // The +2 is for initial 3 and trailing 0. We'll compensate for multi-byte
> >    // chars below.
> > +  CheckedUint32 size = (aEnd - aStart) + 2;
> 
> Just to be 100% safe, do the +2 separate:
> 
> CheckedUint32 size = aEnd - aStart;
> size +=2;
> if (!size.isValid()) {
>   ...
> }

I thought this was covered earlier at line#440:
  if (NS_WARN_IF(aStart > aEnd || UINT32_MAX - 2 < uintptr_t(aEnd - aStart))) {
    IDB_REPORT_INTERNAL_ERR();
    return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
}
Comment on attachment 8887821 [details] [diff] [review]
(v3) Patch: Fix Overflow in Key::EncodeAsString()

[Security approval request comment]
Q: How easily could an exploit be constructed based on the patch?
A: It's easy to be exploited with Blob in vary large size according to the POC attached in comment 0.
Q: Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
A: No.
Q: Which older supported branches are affected by this flaw?
A: Since FF54.
Q: If not all supported branches, which bug introduced the flaw?
A: N/A
Q: Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
A: Yes
Q: How likely is this patch to cause regressions; how much testing does it need?
A: The risk is relatively low to cause regression because what we are doing is to add more boundary check for the pointer accumulation.
Attachment #8887821 - Flags: sec-approval?
Ritu, what do you think? Can we take this on Beta as well?
Flags: needinfo?(rkothari)
It's a sec-crit so as such it meets the bar. I only the patch had fewer changes in it. We still have one beta build and an entire RC week before we go live so let's take it. Julien FYI
Flags: needinfo?(rkothari) → needinfo?(jcristau)
Comment on attachment 8887821 [details] [diff] [review]
(v3) Patch: Fix Overflow in Key::EncodeAsString()

Sec-approval+ for trunk. Please nominate a beta patch as well ASAP.
Attachment #8887821 - Flags: sec-approval? → sec-approval+
(In reply to Wes Kocher (:KWierso) from comment #23)
> Backed out for build failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=117770073&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 8adfbfd01ccd2500c243fd4e47cf08ecb9f237c2

[task 2017-07-26T01:26:04.652843Z] 01:26:04     INFO -  /home/worker/workspace/build/src/dom/indexedDB/Key.cpp:447:25: error: cannot pass an arithmetic expression of built-in types to 'CheckedInt<long>'
[task 2017-07-26T01:26:04.652934Z] 01:26:04     INFO -    CheckedUint32 size = (aEnd - aStart) + 2;

Why does |CheckedUint32| (should be unsigned) resolve to |CheckedInt<long>| (signed!) on this platform?
https://hg.mozilla.org/mozilla-central/rev/301acb1080e7

seems this landed later again
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Flags: needinfo?(jcristau)
Flags: needinfo?(btseng)
This'll need a rebased patch for Beta, FWIW.
Flags: needinfo?(btseng)
Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Potential security problem.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This patch is to enhance the boundary check when accumulating the pointer.
[String changes made/needed]: No.
Flags: needinfo?(btseng)
Attachment #8891154 - Flags: review+
Attachment #8891154 - Flags: approval-mozilla-beta?
Attachment #8891154 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [adv-main55+]
Flags: sec-bounty? → sec-bounty+
Group: dom-core-security → core-security-release
Hello, were you able to verify that this fixes the problem?
Flags: needinfo?(q1)
Whiteboard: [adv-main55+] → [adv-main55+][post-critsmash-triage]
(In reply to Matt Wobensmith [:mwobensmith][:matt:] from comment #29)
> Hello, were you able to verify that this fixes the problem?

This fix didn't make it into the source for FF55B13, which was the latest as of this morning. I'll check the next beta early next week, assuming it's out by then.
(In reply to Matt Wobensmith [:mwobensmith][:matt:] from comment #29)
> Hello, were you able to verify that this fixes the problem?

The fix detects the overflow and throws an error in FF 55.0.1.
Flags: needinfo?(q1)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.