Closed Bug 1401066 Opened 4 years ago Closed 4 years ago

Suspicious use after free when updating SB list

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1362761
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix

People

(Reporter: tnguyen, Assigned: tnguyen)

References

Details

(Keywords: csectype-uaf, leave-open, sec-moderate)

Attachments

(1 file)

We have 2 bugs that crashed at mIndexDeltas[i].mHdr->mLength, bug 1362761 and bug 1381216, it looks like we have an uaf at nsTArray_base<nsTArrayDefaultAllocator>::Length() when updating sb list
Keywords: csectype-uaf
Look at the following lines
http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/toolkit/components/url-classifier/Classifier.cpp#745,753

It seems the 2 threads access to lookupcache and clear/write mIndexDeltas simultaneously, which may cause uaf.
Assignee: nobody → tnguyen
Summary: Suspicious user after free when updating SB list → User after free when updating SB list
Summary: User after free when updating SB list → Suspicious user after free when updating SB list
Group: toolkit-core-security
Duplicate of this bug: 1401065
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #1)
> Look at the following lines
> http://searchfox.org/mozilla-central/rev/
> 1c13d5cf85f904afb8976c02a80daa252b893fca/toolkit/components/url-classifier/
> Classifier.cpp#745,753
> 
> It seems the 2 threads access to lookupcache and clear/write mIndexDeltas
> simultaneously, which may cause uaf.

Hmm, I don't know if that it's correct. We use mIsUpdating to prevent 2 updates being in queue (expect, onStopRequest is triggered multiple times).
mHdr is never null technically 
http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/xpcom/ds/nsTArray.h#523
except the array mIndexDeltas[i] is freed somewhere
Look at the block of code
  uint32_t indexDeltaSize = mIndexDeltas.Length();
  uint32_t totalDeltas = 0;

  nsTArray<uint32_t> indexStarts;
  indexStarts.AppendElement(0);

  for (uint32_t i = 0; i < indexDeltaSize; i++) {
    uint32_t deltaLength = mIndexDeltas[i].Length();
    totalDeltas += deltaLength;
    if (!indexStarts.AppendElement(totalDeltas, fallible)) {
      return NS_ERROR_OUT_OF_MEMORY;
    }
  }

The crash point mPrefixSet->mIndexDeltas[i].mHdr->mLength
The crash is on various platforms, at my first guess mIndexDeltas[i] is cleared, but I could see we add a mutex lock every time to access to members including mIndexDeltas.
For example 
http://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp#71
How could mHdr be null, a quick look and it should be set to sEmptyHdr at array ctor and is definitely not null.
http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/xpcom/ds/nsTArray.h#523
http://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/xpcom/ds/nsTArray-inl.h#13

It's possible we have a use-after-free/corruption bug, having a null mPrefixSet would be a better bet for what's going on.

I would like to add a posion canary to confirm, just to move to crash to the canary check
MozReview-Commit-ID: GpTDMR5Ni3e
Comment on attachment 8911223 [details] [diff] [review]
Add poison canary to check for errors

This is not the fix but could help us confirm and narrow down the crash.
If we have a crash report that the canary.check() (surely, if ff crashes at canary.check(), we still have a crash without that check later), obviously, the prefix set is freed. Otherwise, that is the memory corruption in the loop.
Attachment #8911223 - Flags: review?(francois)
Comment on attachment 8911223 [details] [diff] [review]
Add poison canary to check for errors

Review of attachment 8911223 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me. We're adding a potential runtime crash, but if there's memory corruption we're likely to crash anyways.
Attachment #8911223 - Flags: review?(francois) → review+
Let's wait for a while to see if there's any crash in canary check
Summary: Suspicious user after free when updating SB list → Suspicious use after free when updating SB list
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1362761
Group: toolkit-core-security
You need to log in before you can comment on or make changes to this bug.