Closed Bug 1379414 Opened 7 years ago Closed 7 years ago

Potential read beyond bounds in ReadCompressedIndexDataValuesFromBlob()

Categories

(Core :: Storage: IndexedDB, defect, P2)

54 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: q1, Assigned: bevis)

Details

(Keywords: csectype-intoverflow, sec-moderate, Whiteboard: [adv-main56+][post-critsmash-triage])

Attachments

(2 files)

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.
Group: core-security → dom-core-security
Flags: sec-bounty?
Andrew or Bevis, could either of you take a look at this bug? Thanks!
Flags: needinfo?(bugmail)
Flags: needinfo?(btseng)
Priority: -- → P2
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?
(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)
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|.
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 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?
pulsebot> Check-in: https://hg.mozilla.org/integration/mozilla-inbound/rev/853cfa601a8f - Bevis Tseng - Bug 1379414 - Fix ReadCompressedIndexDataValuesFromBlob(). r=asuth
Flags: needinfo?(btseng)
https://hg.mozilla.org/mozilla-central/rev/853cfa601a8f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Please request Beta & ESR52 approval on this when you get a chance.
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 #8891155 - Flags: review+
Attachment #8891155 - Flags: approval-mozilla-beta?
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-
Flags: sec-bounty? → sec-bounty+
Group: dom-core-security → core-security-release
Whiteboard: [adv-main56+]
Flags: qe-verify-
Whiteboard: [adv-main56+] → [adv-main56+][post-critsmash-triage]
(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.
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)
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)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.