1480521 - js::Shape is not Compacting-GC-safe (32-bit builds)

Status: RESOLVED FIXED

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

Description

[Ted Campbell [:tcampbell]]

While working on Bug 1479900, I discovered the Shape::propid_ field aliases RelocationOverlay::magic but is unable to guarantee no collision. This only applies to 32-bit builds.

STR:
  Add |MOZ_ASSERT(!gc::IsForwarded(shape))| to Shape::new_
  Run |var x = { 1567120744: "Bad" }| in the resulting SpiderMonkey

Sec-critical since that property name will cause GC corruption in any context on all 32-bit builds!

Regressed while adding patches for nursery strings:
https://searchfox.org/mozilla-central/diff/34decd09c6e9e3e931a4bd78abf7bac6a49ce833/js/src/gc/RelocationOverlay.h#38

Comment 1

[Ted Campbell [:tcampbell]]

So, my work on Bug 1479900 will resolve this and make it harder to accidentally repeat in the future. My concern is what code to use for uplifts.

Comment 2

[Ted Campbell [:tcampbell]]

JSScript looks unsafe as well. The magic_ will end up on jitCodeSkipArgCheck_ which can be an arbitrary and unaligned pointer. This is much more difficult to exploit because it requires allocating a JSScript at an exact memory address on 32-bit.

RegExpShared, BaseShape are currently safe, but are footguns because the magic aliases flags and there are no checks or comments explaining the limitations of legal flag values.

Symbol, ObjectGroup, and BigInt would be unsafe, but we don't allow compacting on them so they aren't a problem.

Comment 3

[Ted Campbell [:tcampbell]]

The initial patches on Bug 1479900 are my refactoring of RelocationOverlay which ended up solving this bug as a side-effect. Should be use them as the basis for uplifts or should we have a more targeted fix?

Comment 4

[Ted Campbell [:tcampbell]]

The minimal fix is just to re-order the fields of js::Shape to avoid the problem. That unfortunately paints the bullseye pretty well on what the problem is. Arguably, we could let the patches on Bug 1479900 ride on nightly for a bit since they don't point out a particular problem and then do uplifts in a batch?

Comment 5

[Ted Campbell [:tcampbell]]

Dan, do you have any advise on how to proceed? The issue seems pretty serious. 32-bit builds (FF60+) that use that specific property name anywhere can corrupt the GC and lead to replacing an object with arbitrary memory.

Comment 6

[Daniel Veditz [:dveditz]]

bug 1479900 seems a bit large/risky to push back onto ESR. Ted assures me the minimal fix is a safe couple of lines so let's go with that for the back-ports.

Let bug 1479900 ride the train and hold off on the back-ports until a week or so before the cut-off date. Request sec-approval on the patches and abillings will let you know when that is.

Comment 7

[Ted Campbell [:tcampbell]]

Attachment: Minimal Patch for ESR60 / Release / Beta

The problem was we assumed the jsid field was pointer-like for this all to work. This patch reorders the fields so an actual pointer is in the relocation magic offset.

Will request uplifts once review is stamped.

Comment 8

[Ted Campbell [:tcampbell]]

Comment on attachment 8997547 [details] [diff] [review]
Minimal Patch for ESR60 / Release / Beta

Ugh.. This breaks the static asserts in... relocation code. And this does the wrong thing on 64-bit...

Comment 9

[Ted Campbell [:tcampbell]]

Attachment: Minimal Patch for ESR60 / Release / Beta

Actually tested it this time!

This moves another pointer up the start to meet the what seems the current invariant that first two fields must be pointers.

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8997582 [details] [diff] [review]
Minimal Patch for ESR60 / Release / Beta

Review of attachment 8997582 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, so after messing this up, I wrote a gdb python script that would allow me to look at what overlaps a given offset of any type so I don't need to trust my own inspection. So for example:

(gdb) overlap 4 js::Shape
overlap at 4..7 with this.propid_ : js::PreBarrieredId
overlap at 4..7 with this.propid_.<base> : js::WriteBarrieredBase<jsid>
overlap at 4..7 with this.propid_.<base>.<base> : js::BarrieredBase<jsid>
overlap at 4..7 with this.propid_.<base>.<base>.value : jsid
overlap at 4..7 with this.propid_.<base>.<base>.value.asBits : size_t

I should perhaps have an option to suppress the noise from all the intermediate types. Findings:

 - JS::Symbol has a HashNumber where the magic_ goes. As far as I can tell, there's nothing to stop it from being 0xbad0bad1. But symbols are not moved.
 - js::BaseShape has a flags field, but as you have noted, it doesn't look like you could get that exact flag value.
 - js::Shape is handled in this patch
 - JS::String and friends are scary, but known -- the length high bit cannot be set
 - JSScript has the jitCodeSkipArgCheck_ field as you described above
 - I don't see why ObjectGroup would be a problem? magic_ overlaps proto_, which is an aligned pointer.
 - see below for RegExpShared. I think the bool canStringMatch that takes a full byte should be enough to prevent a collision.
 - (on 64bit) js::Scope has a paddedKind_ field that is intentionally fully initialized to avoid collisions
 - (on 64bit) js::jit::JitCode contains a uint8_t* code_ pointer. I believe we're saved here because our JS::Value packing already assumes that the 17 high bits of pointers will be zero.

This is on 32-bit:
(gdb) overlap 4 js::RegExpShared
overlap at 4..4 with this.flags : js::RegExpFlag
overlap at 5..5 with this.canStringMatch : bool
overlap at 6..9 with this.parenCount : size_t

Comment 11

[Steve Fink [:sfink] [:s:]]

Oh right, JitCode does not get moved either.

Comment 12

[Ted Campbell [:tcampbell]]

ObjectGroup is a TaggedProto which for a dynamic (via Proxy) prototype will set low bits. It is currently not moved though.

The result of Bug 1479900 will be that all Cell-derived types will now be compatible with Relocations even if moving is disallowed for the type for other reasons.

Comment 13

[Ted Campbell [:tcampbell]]

Comment on attachment 8997582 [details] [diff] [review]
Minimal Patch for ESR60 / Release / Beta

This patch is intended for FF-ESR60 / FF-62. Nightly will be fixed by Bug 1479900 instead.

[Approval Request Comment]
[Feature/Bug causing the regression]:
Regressed in Bug 903519.
[If this is not a sec:{high,crit} bug, please state case for ESR consideration]:
Sec-Critical
[User impact if declined]:
GC stability at minimum, arbitrary memory access at worst. 
[Is this code covered by automated tests?]:
General GC test suites hit these code paths. The targeted test case is not include as this is a sec issue.
[Has the fix been verified in Nightly?]:
Nightly is receiving a rewrite in Bug 1479900.
[Needs manual test from QE? If yes, steps to reproduce]: 
No.
[List of other uplifts needed for the feature/fix]:
Stand-alone fix.
[Risk to taking this patch (and alternatives if risky)]:
Risk is reasonably low. We understand why the current code fails and this spot fix moves a "safer" field into the offset of concern.
[String or UUID changes made by this patch]: 
None

Comment 14

[Ted Campbell [:tcampbell]]

Comment on attachment 8997582 [details] [diff] [review]
Minimal Patch for ESR60 / Release / Beta

Recommended plan after discussion with :dveditz is:
- Land Bug 1479900 normally to fix Nightly63
- Land this spot fix against FF60 / FF62 in final week of beta.
- FF61 will be wontfix.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
If someone pieced together that GC Relocation is affected by field order, it is easy to determine what the problem with jsid is. How to leverage faked relocation data into an attack beyond just crashing the GC through corruption requires knowledge of mounting attacks against Garbage Collectors. (I myself am not sure how to pull this off).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
In this variant of the patch, the focus is on the wrong field and there is no indication that relocations are involved. On the other hand, it is curious that a sec bug is fixed by field order.

Which older supported branches are affected by this flaw?
FF60+

If not all supported branches, which bug introduced the flaw?
Regressed in Bug 903519

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch is the backport for FF60ESR / FF62.

How likely is this patch to cause regressions; how much testing does it need?
Regressions should be unlikely. Tests pass locally for me on 32-bit and 64-bit builds.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8997582 [details] [diff] [review]
Minimal Patch for ESR60 / Release / Beta

sec-approval+ for trunk. I'll let release management determine when they want to approve landing on ESR60 and Beta based on suggestions above.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, so this isn't waiting to land on m-c because that's covered by the patches that landed already in bug 1479900. For the timing to land the work in this bug on beta and esr60, how about for next week's beta 18 build (mid-week?)  That gives us time to spot and fix regressions/crash issues.

Comment 17

[Ted Campbell [:tcampbell]]

Bug 1479900 has landed on m-c and closes the security-hole on nightly.

After some discussion with Liz, I'd prefer to keep the original timeline and not pull it forward. If people are watch sec uplifts to beta, they would see a bizarre fix. If they looked at other work I've been doing publicly, it is a tiny leap to connect the dots. The javascript one-liner is enough to destabilize the browser right now. Leveraging that is less trivial but within toolbox of researchers.

Comment 18

[Ted Campbell [:tcampbell]]

(I should clarify this is content-process only)

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, let's hold off till next week (beta 20). Sounds like this is a case where we have a good reason to do so.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

It is a week later. Is this going in before Thursdays beta?

(Marking that this is fixed in nightly in flags...)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

I was planning to approve and uplift tomorrow ahead of Thursday's b20 build, yes.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8997582 [details] [diff] [review]
Minimal Patch for ESR60 / Release / Beta

Time to get this landed for 62.0b20 and ESR 60.2.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dd432f7b82d6
https://hg.mozilla.org/releases/mozilla-esr60/rev/bf26310ec595