1330769 - ASLR leak via pointer scrambling in ShapeTable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Ryan VanderMeulen [:RyanVM]]

+++ This bug was initially created as a clone of Bug #1312001 +++

Bug 1312001 fixed many of the ASLR leak issues, but ShapeTable still has similar issues that need fixing.

Comment 1

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

Marking this blocking 51 release for now, to make sure we follow up -- because we landed a bunch of work for bug 1312001 and I'm not sure what we have left still to fix or what the security risks there are.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: ASLR leak via pointer scrambling in ShapeTable

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8826338 [details] [diff] [review]
ASLR leak via pointer scrambling in ShapeTable

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

Looks good, but we also have to fix HashId for this to work. Something like:

  if (JSID_IS_SYMBOL(id))
      return JSID_TO_SYMBOL(id)->hash();

::: js/src/jscntxt.cpp
@@ +1076,5 @@
>          compartment_->mark();
>  }
>  
> +HashNumber
> +JSContext::randomHashCode() {

Nit: { on next line

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> Looks good, but we also have to fix HashId

While you're there, would you mind adding a brief comment explaining why we do this? I didn't add it initially to avoid drawing attention, but there are similar comments elsewhere now and it's pretty obvious from the patch.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Avoid using Symbol addresses in hash codes

Comment 6

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecfa3a8aaaaf

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8826742 [details] [diff] [review]
Avoid using Symbol addresses in hash codes

I changed some dumb little details about how this worked:

* addressed comment 3 and comment 4
* fixing HashId to use Symbol::hash() required moving it to another file
* moved the randomHashCode() method to JSCompartment
* backtracked on Symbol::new_() having an extra HashNumber argument;
  now it generates a random hash code itself, instead.
  This means the well-known symbols no longer get constant hash codes.

Still, the patch is essentially the same, so carrying forward jandem's r+.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 8826742 [details] [diff] [review]
Avoid using Symbol addresses in hash codes

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Worst-case here would be discovering a Symbol's address, and I think that would be easy for anyone who knew vm/Shape.h well enough.

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?

Yes.

> Which older supported branches are affected by this flaw?

All. I think this bug existed in hg revision 1.

> Do you have backports for the affected branches? If not, how different,
> hard to create, and risky will they be?

No. Very easy.

> How likely is this patch to cause regressions; how much testing does it
> need?

Very unlikely. Ordinary automated testing is sufficient.

Comment 9

[Jason Orendorff [:jorendorff]]

> I think this bug existed in hg revision 1.

To clarify: the exact problems in hg revision 1 were already fixed in bug 1317936. Symbols didn't exist back then. But this bug is as old as symbols -- definitely all supported branches are affected.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Al, Dan, do you think we might be able to get this one landed before we gtb RC2 (later this week but as early as tomm)? Thanks!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

FWIW, looks like this'll need a bit of rebasing for uplifts.

Comment 12

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

(In reply to Ritu Kothari (:ritu) from comment #10)
> Hi Al, Dan, do you think we might be able to get this one landed before we
> gtb RC2 (later this week but as early as tomm)? Thanks!

I'm happy to give it sec-approval+ if you want to take it for RC2.

Comment 13

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

Giving sec-approval+ after talking to Liz.

Comment 14

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26ed78caca3d0ffa819c31f066cfc5d03dde7a95
Bug 1330769 - Avoid using Symbol addresses in hash codes. r=jandem.

Comment 15

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38f32daf0f51

Comment 16

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46b1b668dfbf

Comment 17

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a45e844c455

Comment 18

[Jason Orendorff [:jorendorff]]

Attachment: Avoid using Symbol addresses in hash codes (aurora)

MozReview-Commit-ID: 9kllbUYaXLv

Comment 19

[Jason Orendorff [:jorendorff]]

Attachment: Avoid using Symbol addresses in hash codes (beta)

MozReview-Commit-ID: 9kllbUYaXLv

Comment 20

[Jason Orendorff [:jorendorff]]

Attachment: Avoid using Symbol addresses in hash codes (release)

MozReview-Commit-ID: 9kllbUYaXLv

Comment 21

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ef4ba6401f9

Comment 22

[Jason Orendorff [:jorendorff]]

Comment on attachment 8827629 [details] [diff] [review]
Avoid using Symbol addresses in hash codes (aurora)

Approval Request Comment
[Feature/Bug causing the regression]: bug 645416 (but really the underlying issue is ancient)
[User impact if declined]: sec-high risk
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: beta, release, esr45
[Is the change risky?]: no
[Why is the change risky/not risky?]: small, fairly superficial changes to how Symbols are created
[String changes made/needed]: none

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 8827630 [details] [diff] [review]
Avoid using Symbol addresses in hash codes (beta)

see comment 22

Comment 24

[Jason Orendorff [:jorendorff]]

Comment on attachment 8827631 [details] [diff] [review]
Avoid using Symbol addresses in hash codes (release)

see comment 22

Comment 25

[Jason Orendorff [:jorendorff]]

Attachment: Avoid using Symbol addresses in hash codes (esr45)

MozReview-Commit-ID: 9kllbUYaXLv

Comment 26

[Jason Orendorff [:jorendorff]]

Comment on attachment 8827634 [details] [diff] [review]
Avoid using Symbol addresses in hash codes (esr45)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
    it's sec:high
User impact if declined:
    sec:high risk
Fix Landed on Version:
    ff50 (current m-c)
Risk to taking this patch (and alternatives if risky):
    low risk of bugs involving symbols
String or UUID changes made by this patch:
    none

Comment 27

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

Comment on attachment 8827631 [details] [diff] [review]
Avoid using Symbol addresses in hash codes (release)

For the RC2 build.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/417ac0789e97
https://hg.mozilla.org/releases/mozilla-beta/rev/c5b999111882
https://hg.mozilla.org/releases/mozilla-release/rev/c5b999111882
https://hg.mozilla.org/releases/mozilla-esr45/rev/747085a1cd93

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26ed78caca3d