1401066 - Suspicious use after free when updating SB list

Status: RESOLVED DUPLICATE of bug 1362761

(Toolkit :: Safe Browsing, defect)

Description

[Thomas Nguyen (:tnguyen)]

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

Comment 1

[Thomas Nguyen (:tnguyen)]

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.

Comment 3

[Thomas Nguyen (:tnguyen)]

(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

Comment 4

[Thomas Nguyen (:tnguyen)]

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

Comment 5

[Thomas Nguyen (:tnguyen)]

Attachment: Add poison canary to check for errors

MozReview-Commit-ID: GpTDMR5Ni3e

Comment 6

[Thomas Nguyen (:tnguyen)]

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.

Comment 7

[François Marier [: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.

Comment 8

[Thomas Nguyen (:tnguyen)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f08155dff5e1a04371b83b27226ee344d613d973
Land the check, leave the bug open

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f08155dff5e1

Comment 10

[Thomas Nguyen (:tnguyen)]

Let's wait for a while to see if there's any crash in canary check