1717778 - Inserting the same node into the same linked list might lead to a UAF

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Kershaw Chang [:kershaw]]

Filed this as a security bug, since this is about UAF.

According to bug 1544190, we found that it's possible to insert the same node into the same linked list twice. In this case, this can lead to a UAF.
Seeing the code that inserts a node below, there is only an assertion to check if a new node is already in a list. Apparently, this is not enough. Maybe we should early return to avoid this.

  void setPreviousUnsafe(RawType aElem) {
    LinkedListElement<T>* listElem = static_cast<LinkedListElement<T>*>(aElem);
    MOZ_ASSERT(!listElem->isInList());

    listElem->mNext = this;
    listElem->mPrev = this->mPrev;
    this->mPrev->mNext = listElem;
    this->mPrev = listElem;

    Traits::enterList(aElem);
  }

Comment 1

[Daniel Veditz [:dveditz]]

Could make this RELEASE assert, or at least a DIAGNOSTIC one.

ni? to Nika because I'm not sure who else owns this. Clearly the "triage owner" here needs to be updated

Comment 2

[Andrew McCreight [:mccr8]]

I found a few fixed sec-high bugs with this assertion: bug 1557208, bug 1415770. It looks like this check is just a pointer comparison, so I don't know why we shouldn't just make it a release assert. We don't seem to have any currently open bugs with this assert, so hopefully it won't cause many common issues.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

(In reply to Daniel Veditz [:dveditz] from comment #1)

Could make this RELEASE assert, or at least a DIAGNOSTIC one.

Upgrading this to a release assert seems reasonable to me

ni? to Nika because I'm not sure who else owns this. Clearly the "triage owner" here needs to be updated

I think :glandium is the only MFBT peer employed by Mozilla right now

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1717778 - Upgrade some LinkedList asserts to RELEASE_ASSERT,

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Upgrade some LinkedList asserts to RELEASE_ASSERT, r=glandium
https://hg.mozilla.org/integration/autoland/rev/6ba608aff7b71ed48fccfc31efa3d64c497e82f3
https://hg.mozilla.org/mozilla-central/rev/6ba608aff7b7

Comment 6

[Andrew McCreight [:mccr8]]

I don't know if we want to explicitly link it here or not, but bug 1719319 looks like a place where we're hitting the assertion.

Comment 7

[Andrew McCreight [:mccr8]]

That's the only signature I can see hitting the assert, so that's nice.

Comment 8

[Andrew McCreight [:mccr8]]

In addition to the two "regressions" I marked, some, but not all, of the crashes under the signature in bug 1720692 also hit the assert. It looks like that started well after this assert landed, so I think it already detected a fresh regression, which is cool.