Closed
Bug 1371484
Opened 7 years ago
Closed 7 years ago
Write beyond bounds in Key::EncodeAsString()
Categories
(Core :: Storage: IndexedDB, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla56
People
(Reporter: q1, Assigned: bevis)
Details
(Keywords: csectype-bounds, sec-critical, Whiteboard: [adv-main55+][post-critsmash-triage])
Attachments
(4 files, 2 obsolete files)
1007 bytes,
text/html
|
Details | |
296.03 KB,
application/x-7z-compressed
|
Details | |
5.82 KB,
patch
|
baku
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
5.84 KB,
patch
|
bevis
:
review+
abillings
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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)
Comment 2•7 years ago
|
||
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.
Updated•7 years ago
|
Group: core-security → dom-core-security
status-firefox54:
--- → wontfix
status-firefox55:
--- → affected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox55:
--- → ?
tracking-firefox56:
--- → ?
tracking-firefox-esr52:
--- → ?
Comment 5•7 years ago
|
||
Jan and Bevis can probably take a look.
Flags: needinfo?(jvarga)
Flags: needinfo?(btseng)
Priority: -- → P1
Assignee | ||
Comment 6•7 years ago
|
||
I'd like to take this bug to follow up.
Assignee: nobody → btseng
Status: NEW → ASSIGNED
Flags: needinfo?(btseng)
Assignee | ||
Comment 8•7 years ago
|
||
Assignee | ||
Comment 9•7 years ago
|
||
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)
Comment 11•7 years ago
|
||
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)
Comment 12•7 years ago
|
||
Could one of you review this patch? Thanks.
Flags: needinfo?(bugmail)
Flags: needinfo?(amarchesini)
Comment 13•7 years ago
|
||
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-
Updated•7 years ago
|
Flags: needinfo?(bugmail)
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 14•7 years ago
|
||
(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!
Assignee | ||
Comment 15•7 years ago
|
||
Adopt CheckedUint32 suggested in comment 13 to fix the overflow problem.
Attachment #8879486 -
Attachment is obsolete: true
Attachment #8887821 -
Flags: review?(amarchesini)
Comment 16•7 years ago
|
||
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+
Assignee | ||
Comment 17•7 years ago
|
||
(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; }
Assignee | ||
Comment 18•7 years ago
|
||
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?
Comment 19•7 years ago
|
||
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 21•7 years ago
|
||
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+
Updated•7 years ago
|
Keywords: checkin-needed
Updated•7 years ago
|
Comment 22•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/810a0598980114e79c968546b24d75337f83c79b
Keywords: checkin-needed
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
Flags: needinfo?(btseng)
Reporter | ||
Comment 24•7 years ago
|
||
(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?
Comment 25•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/301acb1080e7 seems this landed later again
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Flags: needinfo?(jcristau)
Flags: needinfo?(btseng)
Assignee | ||
Comment 27•7 years ago
|
||
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?
Updated•7 years ago
|
Attachment #8891154 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Keywords: checkin-needed
Comment 28•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/52b2aac1366175503ed2316be358687decd4445b
Keywords: checkin-needed
Updated•7 years ago
|
Whiteboard: [adv-main55+]
Updated•7 years ago
|
Flags: sec-bounty? → sec-bounty+
Updated•7 years ago
|
Group: dom-core-security → core-security-release
Comment 29•7 years ago
|
||
Hello, were you able to verify that this fixes the problem?
Flags: needinfo?(q1)
Whiteboard: [adv-main55+] → [adv-main55+][post-critsmash-triage]
Reporter | ||
Comment 30•7 years ago
|
||
(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.
Reporter | ||
Comment 31•7 years ago
|
||
(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.
Updated•7 years ago
|
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•