1377532 - Write beyond bounds in CacheFileChunkBuffer::FillInvalidRanges()

Status: RESOLVED FIXED

(Core :: Networking: Cache, enhancement)

Description

[q1]

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: }

Comment 1

[Michal Novotny [:michal]]

This has been fixed in bug 1377369.

Comment 2

[Al Billings [:abillings - ex-MoCo]]

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.