Closed
Bug 1330769
Opened 8 years ago
Closed 8 years ago
ASLR leak via pointer scrambling in ShapeTable
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
People
(Reporter: RyanVM, Assigned: jorendorff)
References
Details
(Keywords: csectype-disclosure, csectype-sop, sec-high, Whiteboard: [adv-main51+][adv-esr45.7+] Disclosure date ~Jan 21 2017 (note Fx51 scheduled for Jan 24))
Attachments
(5 files, 1 obsolete file)
12.26 KB,
patch
|
jorendorff
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
12.28 KB,
patch
|
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
12.27 KB,
patch
|
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
12.27 KB,
patch
|
lizzard
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
12.04 KB,
patch
|
lizzard
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
+++ 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•8 years ago
|
Assignee: nobody → jorendorff
Updated•8 years ago
|
Comment 1•8 years ago
|
||
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•8 years ago
|
||
Attachment #8826338 -
Flags: review?(jdemooij)
Comment 3•8 years ago
|
||
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+
Comment 4•8 years ago
|
||
(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 | ||
Comment 5•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8826338 -
Attachment is obsolete: true
Assignee | ||
Comment 6•8 years ago
|
||
Assignee | ||
Comment 7•8 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•8 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•8 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•8 years ago
|
||
FWIW, looks like this'll need a bit of rebasing for uplifts.
Comment 12•8 years ago
|
||
(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)
Comment 13•8 years ago
|
||
Giving sec-approval+ after talking to Liz.
Updated•8 years ago
|
Attachment #8826742 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/26ed78caca3d0ffa819c31f066cfc5d03dde7a95
Bug 1330769 - Avoid using Symbol addresses in hash codes. r=jandem.
Assignee | ||
Comment 15•8 years ago
|
||
Assignee | ||
Comment 16•8 years ago
|
||
Assignee | ||
Comment 17•8 years ago
|
||
Updated•8 years ago
|
Flags: needinfo?(dveditz)
Assignee | ||
Comment 18•8 years ago
|
||
MozReview-Commit-ID: 9kllbUYaXLv
Assignee | ||
Comment 19•8 years ago
|
||
MozReview-Commit-ID: 9kllbUYaXLv
Assignee | ||
Comment 20•8 years ago
|
||
MozReview-Commit-ID: 9kllbUYaXLv
Assignee | ||
Comment 21•8 years ago
|
||
Assignee | ||
Comment 22•8 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•8 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•8 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•8 years ago
|
||
MozReview-Commit-ID: 9kllbUYaXLv
Assignee | ||
Comment 26•8 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?
Updated•8 years ago
|
Attachment #8827629 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
Attachment #8827630 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•8 years ago
|
Attachment #8827634 -
Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Comment 27•8 years ago
|
||
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 28•8 years ago
|
||
Reporter | ||
Comment 29•8 years ago
|
||
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(rkothari)
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•8 years ago
|
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)
Updated•8 years ago
|
Group: javascript-core-security → core-security-release
Updated•8 years ago
|
Alias: CVE-2017-5397
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•