Closed Bug 1739219 Opened 3 years ago Closed 3 years ago

SetLength() and GetMutableData() are dangerous APIs


(Core :: XPCOM, enhancement)




97 Branch
Tracking Status
firefox-esr91 97+ fixed
firefox95 --- wontfix
firefox96 --- wontfix
firefox97 + fixed


(Reporter: mccr8, Assigned: nika)



(Keywords: sec-want, Whiteboard: [post-critsmash-triage][adv-main97-][adv-esr91.6-])

As seen in bug 1738237, nsTSubstring<T>::SetLength() takes an nsTSubString::size_type argument. Unfortunately this means that if you pass in a 64-bit value it will be implicitly coerced into a 32-bit value, which can overflow. It would be nice if we were more resistant to this kind of issue.

One approach would be to have a (non-nsTSubString) size_t overload that does the CheckedInt conversion. Or maybe we need bounds checking on whatever does a read or write after the SetLength()?

Summary: SetLength() is dangerous API → SetLength() is a dangerous API
See Also: → CVE-2021-43537
Depends on: 1741201
Depends on: 1741210

Decoder mentioned GetMutableData() as another thing that deals with lengths.

Summary: SetLength() is a dangerous API → SetLength() and GetMutableData() are dangerous APIs

Nika is working on some kind of big fix here, so I'll assign this to her for now.

Assignee: nobody → nika
Depends on: 1741665
Closed: 3 years ago
Resolution: --- → FIXED
Group: dom-core-security → core-security-release
Target Milestone: --- → 97 Branch
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main97-]
Whiteboard: [post-critsmash-triage][adv-main97-] → [post-critsmash-triage][adv-main97-][adv-esr91.6-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.