1670358 - (CVE-2020-26960) Calling Compact on a nsTArray will lead to use-after-free error in JS_GC because of the reallocation

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P1)

Description

[Zijie Zhao]

Attachment: test_and_output.zip

Version

commit	62c443a7c801ba9672de34c2867ec1665a4bbe67
parent	b2dee52fd392f7130bc3858a70c683faa773aa8f
hg	48f10c4139655c2c4d52fa773f1fd760ea681d42
git	62c443a7c801ba9672de34c2867ec1665a4bbe67
author	Gijs Kruitbosch <[e-mail]>
committer	Gijs Kruitbosch <[e-mail]>
commit time	Fri, 11 Sep 2020 15:05:50 +0000

Steps to reproduce

• Enable address sanitizer in mozconfig.
• In TestGCPostBarriers.cpp,
  add array->Compact() after line 58 and before line 64
• run the gtest with ./mach gtest GCPostBarriers.nsTArray

More details

The key intuition is that the added Compact will cause the array to reallocate
and trying to access array items using an old address will lead to use-after-free error.

We came across this when we were working on something similar to a ChaosMode feature where we wanted to make array reallocation happen more often.

---

We have attached a test file with some print statements and the output file.

Our output file shows that the error happens when RunTest is first called with a nsTArray at line 96.

Our output file also shows that reallocation happened several times in the loop line 52-58,
which indicates that the extra reallocation triggered by Compact should not
affect later code.

Before being populated, the array was passed to JS_AddExtraGCRootsTracer
together with TraceArray at line 44.

In TraceArray, we found an unsafe use of addresses of array items.

template <class ArrayT>
static void TraceArray(JSTracer* trc, void* data) {
  ArrayT* array = static_cast<ArrayT*>(data);
  for (unsigned i = 0; i < array->Length(); ++i)
    JS::TraceEdge(trc, &array->ElementAt(i), "array-element");
}

Here, &array->ElementAt(i) will become an invalid address if the array gets reallocated.
We added a print statement in TraceArray but didn't see it in the output file.
This could mean that there are other similarly unsafe code in JS_GC.

We also dumped all the array addresses and saw that
the UAF read is accessing the address right before the last reallocation from Compact.
The UAF address is the last element in the array but we don't know if the last one
is different from other elements.

We think there might be unsafe use of array addresses in the GC code or
the way Compact reallocates array is problematic.

Comment 1

[Andrew McCreight [:mccr8]]

This is kind of XPCOM, but I think barriers generally fall under GC, so I'll move it there.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Zijie Zhao from comment #0)
Thank you for reporting this problem.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1670358 - Don't use realloc for shrinking nsTArrays and similar when RelocationStrategy::allowRealloc is false r?mccr8

My original patch handled the grow case but not the shrink case. When the
current and new allocation sizes are in different size classes jemalloc's
realloc will move the allocation when shrinking, not just truncate the existing
one.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1670358 - Add test for shrinking nsTArrays of JS::Heap<T> r=sg

Depends on D93654

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 9181792 [details]
Bug 1670358 - Don't use realloc for shrinking nsTArrays and similar when RelocationStrategy::allowRealloc is false r?mccr8

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Very difficult.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All of them
• If not all supported branches, which bug introduced the flaw?: Bug 877762
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Should be trivial.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely; this does the same in the shrink case as the grow case and that code has been there for years.

Comment 6

[Tom Ritter [:tjr]]

Comment on attachment 9182867 [details]
Bug 1670358 - Add test for shrinking nsTArrays of JS::Heap<T> r=sg

Approved to land and uplift. I don't see a reason to delay landing the test, so let's just do it now and avoid the possibility of forgetting. (And the associated cost of 'land this later'...)

Comment 7

[Tom Ritter [:tjr]]

Comment on attachment 9181792 [details]
Bug 1670358 - Don't use realloc for shrinking nsTArrays and similar when RelocationStrategy::allowRealloc is false r?mccr8

Approved to land and uplift.

Comment 8

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/autoland/rev/7e223284a9225c66b590aaad671c7448d1ff0b57
https://hg.mozilla.org/integration/autoland/rev/8a8d7fdc7038fb360d7c50f5943ccafd7f0bb829

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/7e223284a922
https://hg.mozilla.org/mozilla-central/rev/8a8d7fdc7038

Comment 10

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/09e0f1bf23b2
https://hg.mozilla.org/releases/mozilla-beta/rev/2ad663dc0b4d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/8faa3462b5b1
https://hg.mozilla.org/releases/mozilla-esr78/rev/41b1626448a6

Comment 12

[Daniel Veditz [:dveditz]]

Since we don't know of a case where this is reachable from the web (the POC required modifying C++ test code) sec-moderate seems a more appropriate rating.

Comment 13

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 14

[Tom Ritter [:tjr]]

Attachment: advisory.txt