ASLR leak via pointer scrambling in ShapeTable

RESOLVED FIXED in Firefox -esr45

Status

()

defect
P1
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: RyanVM, Assigned: jorendorff)

Tracking

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

Trunk
mozilla53
Points:
---
Dependency tree / graph

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)

+++ 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.
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.
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.
Attachment #8826338 - Attachment is obsolete: true
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+
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?
> 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)
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)
MozReview-Commit-ID: 9kllbUYaXLv
MozReview-Commit-ID: 9kllbUYaXLv
MozReview-Commit-ID: 9kllbUYaXLv
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?
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?
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?
MozReview-Commit-ID: 9kllbUYaXLv
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+
https://hg.mozilla.org/mozilla-central/rev/26ed78caca3d
Status: NEW → RESOLVED
Closed: 3 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.