ASLR leak via pointer scrambling in ShapeTable

RESOLVED FIXED in Firefox -esr45

Status

()

defect
P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: RyanVM, Assigned: jorendorff)

Tracking

({csectype-disclosure, csectype-sop, sec-high})

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox-esr4551+ fixed, firefox50 wontfix, firefox51blocking fixed, firefox52+ fixed, firefox53+ fixed)

Details

(Whiteboard: [adv-main51+][adv-esr45.7+] Disclosure date ~Jan 21 2017 (note Fx51 scheduled for Jan 24))

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
+++ 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.
(Reporter)

Updated

2 years ago
Assignee: nobody → jorendorff
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.
(Assignee)

Comment 2

2 years ago
Attachment #8826338 - Flags: review?(jdemooij)
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
Attachment #8826338 - Flags: review?(jdemooij) → review+
(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.
(Assignee)

Updated

2 years ago
Attachment #8826338 - Attachment is obsolete: true
(Assignee)

Comment 7

2 years ago
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+.
Attachment #8826742 - Flags: review+
(Assignee)

Comment 8

2 years ago
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.
Attachment #8826742 - Flags: sec-approval?
(Assignee)

Comment 9

2 years ago
> 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.
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!
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
(Reporter)

Comment 11

2 years ago
FWIW, looks like this'll need a bit of rebasing for uplifts.
(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.
Flags: needinfo?(abillings) → needinfo?(rkothari)
Giving sec-approval+ after talking to Liz.
Attachment #8826742 - Flags: sec-approval? → sec-approval+
Flags: needinfo?(dveditz)
(Assignee)

Comment 18

2 years ago
MozReview-Commit-ID: 9kllbUYaXLv
(Assignee)

Comment 19

2 years ago
MozReview-Commit-ID: 9kllbUYaXLv
(Assignee)

Comment 20

2 years ago
MozReview-Commit-ID: 9kllbUYaXLv
(Assignee)

Comment 22

2 years ago
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
Attachment #8827629 - Attachment description: Avoid using Symbol addresses in hash codes → Avoid using Symbol addresses in hash codes (aurora)
Attachment #8827629 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 23

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

see comment 22
Attachment #8827630 - Attachment description: Avoid using Symbol addresses in hash codes → Avoid using Symbol addresses in hash codes (beta)
Attachment #8827630 - Flags: approval-mozilla-beta?
(Assignee)

Comment 24

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

see comment 22
Attachment #8827631 - Attachment description: Avoid using Symbol addresses in hash codes → Avoid using Symbol addresses in hash codes (release)
Attachment #8827631 - Flags: approval-mozilla-release?
(Assignee)

Comment 25

2 years ago
MozReview-Commit-ID: 9kllbUYaXLv
(Assignee)

Comment 26

2 years ago
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
Attachment #8827634 - Attachment description: Avoid using Symbol addresses in hash codes → Avoid using Symbol addresses in hash codes (esr45)
Attachment #8827634 - Flags: approval-mozilla-esr45?
Attachment #8827629 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8827630 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8827634 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Comment on attachment 8827631 [details] [diff] [review]
Avoid using Symbol addresses in hash codes (release)

For the RC2 build.
Attachment #8827631 - Flags: approval-mozilla-release? → approval-mozilla-release+
(Reporter)

Comment 29

2 years ago
https://hg.mozilla.org/mozilla-central/rev/26ed78caca3d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(rkothari)
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Alias: CVE-2017-5397
Whiteboard: Disclosure date ~Jan 21 2017 (note Fx51 scheduled for Jan 24) → [adv-main51+][adv-esr45.7+] Disclosure date ~Jan 21 2017 (note Fx51 scheduled for Jan 24)
Group: javascript-core-security → core-security-release
Alias: CVE-2017-5397
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.