Write beyond bounds in CacheFileChunkBuffer::FillInvalidRanges()

RESOLVED FIXED in Firefox 56

Status

()

Core
Networking: Cache
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: q1, Assigned: michal)

Tracking

56 Branch
mozilla56
Points:
---
Bug Flags:
sec-bounty -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 fixed)

Details

(Reporter)

Description

7 months ago
Bug https://bugzilla.mozilla.org/show_bug.cgi?id=1377369 appears to be caused by a mistaken argument in CacheFileChunkBuffer::FillInvalidRanges()'s call to EnsureBufSize() on line 52, below. It uses |aOther->mDataSize|, but the code on lines 73-76 (which includes the memcpy() that failed in https://bugzilla.mozilla.org/show_bug.cgi?id=1377369 (see https://crash-stats.mozilla.com/report/index/331dd3d0-9018-4f07-8088-bc9b80170630)) uses a length derived from |aOther->mBufSize|, which is potentially larger than |aOther->mDataSize|. This can cause a write beyond the bounds of |mBuf|:

46: nsresult
47: CacheFileChunkBuffer::FillInvalidRanges(CacheFileChunkBuffer *aOther,
48:                                         CacheFileUtils::ValidityMap *aMap)
49: {
50:   nsresult rv;
51: 
52:   rv = EnsureBufSize(aOther->mDataSize);
53:   if (NS_FAILED(rv)) {
54:     return rv;
55:   }
56: 
57:   uint32_t invalidOffset = 0;
58:   uint32_t invalidLength;
59: 
60:   for (uint32_t i = 0; i < aMap->Length(); ++i) {
61:     uint32_t validOffset = (*aMap)[i].Offset();
62:     uint32_t validLength = (*aMap)[i].Len();
63: 
64:     MOZ_RELEASE_ASSERT(invalidOffset <= validOffset);
65:     invalidLength = validOffset - invalidOffset;
66:     if (invalidLength > 0) {
67:       MOZ_RELEASE_ASSERT(invalidOffset + invalidLength <= aOther->mBufSize);
68:       memcpy(mBuf + invalidOffset, aOther->mBuf + invalidOffset, invalidLength);
69:     }
70:     invalidOffset = validOffset + validLength;
71:   }
72: 
73:   if (invalidOffset < aOther->mBufSize) {
74:     invalidLength = aOther->mBufSize - invalidOffset;
75:     memcpy(mBuf + invalidOffset, aOther->mBuf + invalidOffset, invalidLength);
76:   }
77: 
78:   return NS_OK;
79: }
Group: core-security → network-core-security
Flags: sec-bounty?
Flags: needinfo?(michal.novotny)
(Assignee)

Comment 1

7 months ago
This has been fixed in bug 1377369.
Flags: needinfo?(michal.novotny)
Assignee: nobody → michal.novotny
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox54: --- → unaffected
status-firefox55: --- → unaffected
status-firefox56: --- → fixed
status-firefox-esr52: --- → unaffected
Depends on: 1377369
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Group: network-core-security → core-security-release
This issue had already been reported and was clearly (to use) a security issue because of the crash. As such, we feel that this security bug didn't add enough to the conversation and process of fixing it to be eligible for a bounty payment.
Flags: sec-bounty? → sec-bounty-
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.