Closed
Bug 1379414
Opened 7 years ago
Closed 7 years ago
Potential read beyond bounds in ReadCompressedIndexDataValuesFromBlob()
Categories
(Core :: Storage: IndexedDB, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla56
People
(Reporter: q1, Assigned: bevis)
Details
(Keywords: csectype-intoverflow, sec-moderate, Whiteboard: [adv-main56+][post-critsmash-triage])
Attachments
(2 files)
3.31 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
2.00 KB,
patch
|
bevis
:
review+
jcristau
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
If the data blobs it uses are untrusted or defective, ReadCompressedIndexDataValuesFromBlob() could read beyond bounds. The bugs are that its tests for buffer bounds can overflow, causing address-wrap and thus a false negative. This can occur on any platform that places user-mode data in the highest 0x7ffffff5 bytes [1] of the address space. (Thus, it could occur on, e.g., 32-bit Windows with the /3GB option). The bugs are on lines 878 and 896, below: 844: nsresult 845: ReadCompressedIndexDataValuesFromBlob(const uint8_t* aBlobData, 846: uint32_t aBlobDataLength, 847: nsTArray<IndexDataValue>& aIndexValues) 848: { ... 859: const uint8_t* blobDataIter = aBlobData; 860: const uint8_t* blobDataEnd = aBlobData + aBlobDataLength; 861: 862: while (blobDataIter < blobDataEnd) { 863: int64_t indexId; 864: bool unique; 865: ReadCompressedIndexId(&blobDataIter, blobDataEnd, &indexId, &unique); 866: 867: if (NS_WARN_IF(blobDataIter == blobDataEnd)) { 868: IDB_REPORT_INTERNAL_ERR(); 869: return NS_ERROR_FILE_CORRUPTED; 870: } 871: 872: // Read key buffer length. 873: const uint64_t keyBufferLength = 874: ReadCompressedNumber(&blobDataIter, blobDataEnd); 875: 876: if (NS_WARN_IF(blobDataIter == blobDataEnd) || 877: NS_WARN_IF(keyBufferLength > uint64_t(UINT32_MAX)) || 878: NS_WARN_IF(blobDataIter + keyBufferLength > blobDataEnd)) { 879: IDB_REPORT_INTERNAL_ERR(); 880: return NS_ERROR_FILE_CORRUPTED; 881: } 882: 883: nsCString keyBuffer(reinterpret_cast<const char*>(blobDataIter), 884: uint32_t(keyBufferLength)); 885: blobDataIter += keyBufferLength; 886: 887: IndexDataValue idv(indexId, unique, Key(keyBuffer)); 888: 889: // Read sort key buffer length. 890: const uint64_t sortKeyBufferLength = 891: ReadCompressedNumber(&blobDataIter, blobDataEnd); 892: 893: if (sortKeyBufferLength > 0) { 894: if (NS_WARN_IF(blobDataIter == blobDataEnd) || 895: NS_WARN_IF(sortKeyBufferLength > uint64_t(UINT32_MAX)) || 896: NS_WARN_IF(blobDataIter + sortKeyBufferLength > blobDataEnd)) { 897: IDB_REPORT_INTERNAL_ERR(); 898: return NS_ERROR_FILE_CORRUPTED; 899: } 900: 901: nsCString sortKeyBuffer(reinterpret_cast<const char*>(blobDataIter), 902: uint32_t(sortKeyBufferLength)); 903: blobDataIter += sortKeyBufferLength; 904: 905: idv.mSortKey = Key(sortKeyBuffer); 906: } If, e.g., |blobDataIter| == 0xbffff000, |aBlobDataLength| == 0x1000, and |keyBufferLength| = 0x41000000, then line 878's LHS would overflow to 0x00fff000, which would be <= |blobDataEnd| (0xc0000000), so the test incorrectly would conclude that everything was within bounds, allowing lines 883-84 to form a string from data read far beyond bounds. [1] Derived from the maximum length of an nsCString.
Updated•7 years ago
|
Group: core-security → dom-core-security
Keywords: csectype-intoverflow,
sec-moderate
Updated•7 years ago
|
Flags: sec-bounty?
Updated•7 years ago
|
status-firefox54:
--- → wontfix
status-firefox55:
--- → affected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → affected
Comment 1•7 years ago
|
||
Andrew or Bevis, could either of you take a look at this bug? Thanks!
Flags: needinfo?(bugmail)
Flags: needinfo?(btseng)
Priority: -- → P2
Assignee | ||
Comment 2•7 years ago
|
||
Use UINTPTR_MAX and uintptr_t to identify the overflow problem when accumulating pointers.
Assignee: nobody → btseng
Status: NEW → ASSIGNED
Flags: needinfo?(bugmail)
Flags: needinfo?(btseng)
Attachment #8888248 -
Flags: review?(bugmail)
(In reply to Bevis Tseng [:bevis][:btseng] from comment #2) > Created attachment 8888248 [details] [diff] [review] > (v1) Patch: Fix ReadCompressedIndexDataValuesFromBlob(). > > Use UINTPTR_MAX and uintptr_t to identify the overflow problem when > accumulating pointers. Shouldn't NS_WARN_IF(keyBufferLength > uintptr_t(blobDataEnd)) || be NS_WARN_IF(keyBufferLength > aBlobDataLength) || and similar for the other fixed clause?
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to q1 from comment #3) > Shouldn't > > NS_WARN_IF(keyBufferLength > uintptr_t(blobDataEnd)) || > > be > > NS_WARN_IF(keyBufferLength > aBlobDataLength) || > > and similar for the other fixed clause? This is to ensure that there won't be a underflow in next line when calculating blobDataEnd - keyBufferLength: NS_WARN_IF(blobDataIter > blobDataEnd - keyBufferLength)
Updated•7 years ago
|
Attachment #8888248 -
Flags: review?(bugmail) → review+
(In reply to Bevis Tseng [:bevis][:btseng] from comment #4) > (In reply to q1 from comment #3) > > Shouldn't > > > > NS_WARN_IF(keyBufferLength > uintptr_t(blobDataEnd)) || > > > > be > > > > NS_WARN_IF(keyBufferLength > aBlobDataLength) || > > > > and similar for the other fixed clause? > > This is to ensure that there won't be a underflow in next line when > calculating blobDataEnd - keyBufferLength: > NS_WARN_IF(blobDataIter > blobDataEnd - keyBufferLength) True, though the check I suggested encompasses that, since + if (uintptr_t(aBlobData) > UINTPTR_MAX - aBlobDataLength) { // fail out guarantees that |aBlobDataLength| is always < |blobDataEnd|.
Assignee | ||
Comment 6•7 years ago
|
||
Comment on attachment 8888248 [details] [diff] [review] (v1) Patch: Fix ReadCompressedIndexDataValuesFromBlob(). [Security approval request comment] Q: How easily could an exploit be constructed based on the patch? A: It's could be exploited by storing and reading untrusted/defective blobs if the app is run the memory space with high addresses especially on 32-bit machine as explained 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: 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: It's low to cause regressions because what we are doing is to add proper boundary check for the pointer accumulation and the unit tests we have currently shall be good enough to prevent the regression.
Attachment #8888248 -
Flags: sec-approval?
Comment 7•7 years ago
|
||
Comment on attachment 8888248 [details] [diff] [review] (v1) Patch: Fix ReadCompressedIndexDataValuesFromBlob(). sec-moderate doesn't require approval. Land away!
Flags: needinfo?(btseng)
Attachment #8888248 -
Flags: sec-approval?
Assignee | ||
Comment 8•7 years ago
|
||
pulsebot> Check-in: https://hg.mozilla.org/integration/mozilla-inbound/rev/853cfa601a8f - Bevis Tseng - Bug 1379414 - Fix ReadCompressedIndexDataValuesFromBlob(). r=asuth
Flags: needinfo?(btseng)
Comment 9•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/853cfa601a8f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 10•7 years ago
|
||
Please request Beta & ESR52 approval on this when you get a chance.
Flags: needinfo?(btseng)
Assignee | ||
Comment 11•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 #8891155 -
Flags: review+
Attachment #8891155 -
Flags: approval-mozilla-beta?
Comment 12•7 years ago
|
||
Comment on attachment 8891155 [details] [diff] [review] (beta) Patch: Fix ReadCompressedIndexDataValuesFromBlob(). r=asuth we're out of betas, and this is sec-moderate, beta55-
Attachment #8891155 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•7 years ago
|
Updated•7 years ago
|
Flags: sec-bounty? → sec-bounty+
Updated•7 years ago
|
Group: dom-core-security → core-security-release
Comment 13•7 years ago
|
||
Per comment 6, ESR52 is unaffected.
Updated•7 years ago
|
Whiteboard: [adv-main56+]
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main56+] → [adv-main56+][post-critsmash-triage]
Comment 14•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #13) > Per comment 6, ESR52 is unaffected. I'm afraid comment 6 is incorrect. ESR52 has the vulnerable code present (verified in the 52.4.0 source). ReadCompressedIndexDataValuesFromBlob was introduced before then. Since it's sec-moderate it'd likely not be a candidate for uplifting, but you should probably re-evaluate if this is a severe enough risk to do so. the patch is very low risk at any rate and corrects a clear code bug.
Updated•7 years ago
|
Comment 15•7 years ago
|
||
Bevis, is this something we should consider uplifting to ESR52 (typically only high/crit unless there's a really good reason for doing so)?
Flags: needinfo?(btseng)
Assignee | ||
Comment 16•7 years ago
|
||
This should only happen if |blobDataIter| points to a high address value with a "large" |keyBufferLength| but index keys in indexedDB shall always be small. It shall be fine not to uplift this ESR52.
Flags: needinfo?(btseng)
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
•