Closed Bug 1717778 Opened 1 year ago Closed 1 year ago

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

Categories

(Core :: MFBT, enhancement)

enhancement

Tracking

()

RESOLVED FIXED
91 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox89 --- wontfix
firefox90 --- wontfix
firefox91 --- fixed

People

(Reporter: kershaw, Assigned: nika)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Keywords: sec-audit, Whiteboard: [post-critsmash-triage][adv-main91+r])

Attachments

(1 file)

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);
  }
Group: core-security → dom-core-security
Keywords: sec-audit

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

Group: dom-core-security → core-security-release
Flags: needinfo?(nika)

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.

(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

Flags: needinfo?(nika)
Assignee: nobody → nika
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

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.

Regressions: 1719319

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

Regressions: 1721487

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.

Regressions: 1724336
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main90+r]
Whiteboard: [post-critsmash-triage][adv-main90+r] → [post-critsmash-triage][adv-main91+r]
Group: core-security-release
Blocks: 1721487
No longer regressions: 1721487
You need to log in before you can comment on or make changes to this bug.