Closed
Bug 1159321
Opened 10 years ago
Closed 10 years ago
Well-known symbols in jsid's should not fire pre-barriers
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox37 | --- | wontfix |
firefox38 | --- | wontfix |
firefox39 | + | fixed |
firefox38.0.5 | --- | wontfix |
firefox40 | + | fixed |
firefox41 | --- | fixed |
firefox-esr31 | --- | unaffected |
firefox-esr38 | 39+ | fixed |
b2g-v1.4 | --- | unaffected |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.1S | --- | unaffected |
b2g-v2.2 | --- | fixed |
b2g-master | --- | fixed |
People
(Reporter: terrence, Assigned: terrence)
References
(Blocks 1 open bug)
Details
(Keywords: sec-high, Whiteboard: [adv-main39+][adv-esr38.1+])
Attachments
(2 files)
(deleted),
patch
|
jonco
:
review+
Sylvestre
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
lmandel
:
approval-mozilla-release-
lmandel
:
approval-mozilla-esr38+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
Like permanent atoms, I think we need to exclude these from barriers as they may be used from different runtimes.
Comment 1•10 years ago
|
||
Why is this a security problem, and how severe is it?
Updated•10 years ago
|
Flags: needinfo?(terrence)
Assignee | ||
Comment 2•10 years ago
|
||
This would allow an attacker to mangle the mark bits of a string/symbol, potentially stripping gray bits, leading to a number of nasty UAF, information leaks, etc. This is hopefully quite hard to spot -- I noticed this when backporting bug 1152177 and it is possible that someone following sec-checkins might see the same thing.
Unfortunately, a direct patch for this would be an incredibly obvious pointer to the problem, so I've folded it into a rewrite in bug 1159428. I'd like to land and test that patch, then use this bug for uplifting more targeted fixes to branches.
Luckily, symbols are a fairly recent addition, so we may not have to uplift far. I'll do some digging to see how far back we need to take this.
Flags: needinfo?(terrence)
Keywords: sec-high
Assignee | ||
Comment 3•10 years ago
|
||
Bug 1159428 is in and green for a day, so it's time to uplift. Well-known symbols went in in 36.
status-b2g-v1.4:
--- → unaffected
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → affected
status-firefox37:
--- → affected
status-firefox38:
--- → affected
status-firefox38.0.5:
--- → affected
status-firefox39:
--- → affected
status-firefox-esr31:
--- → unaffected
status-firefox-esr38:
--- → affected
Assignee | ||
Comment 4•10 years ago
|
||
Bug 1159428 removed all of our specialized Value and jsid marking in favor of type-based dispatch to the common T::writeBarrierPre. Since the type itself is now handling the marking, we get the proper exclusions for permanent atoms and well known symbols for free. Unfortunately, this patch is extremely complex -- and therefore risky -- and relies heavily on mechanisms which have not been uplifted.
Luckily, however, we just finished uplifting an almost-identical fix for PermanentAtoms all the way to 30, so we know exactly what needs to be guarded and where. This patch is a simple, targeted fix drafting on our recent IsPermanentAtom changes to do the same for WellKnownSymbol. It's not very well tested itself, but it's trivial enough and similar enough to our PermanentAtoms fix that I feel confident uplifting it with only minimal testing.
Attachment #8601654 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8601654 [details] [diff] [review]
aurora_backport_1159321-v0.diff
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: It should be classified as sec-high.
User impact if declined: Intermittent crashes and potential exploits.
Fix Landed on Version: 40
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.
Approval Request Comment
[Feature/regressing bug #]: Well known symbols, which landed in 36.
[User impact if declined]: Crashes and potential exploits.
[Describe test coverage new/current, TreeHerder]: Local testing.
[Risks and why]: Low, despite the lack of solid testing. The patch is small and mirrors a similar change from two weeks ago.
[String/UUID change made/needed]: None
Attachment #8601654 -
Flags: approval-mozilla-esr38?
Attachment #8601654 -
Flags: approval-mozilla-beta?
Attachment #8601654 -
Flags: approval-mozilla-b2g37?
Attachment #8601654 -
Flags: approval-mozilla-aurora?
Comment 6•10 years ago
|
||
Comment on attachment 8601654 [details] [diff] [review]
aurora_backport_1159321-v0.diff
This should have had sec-approval, no? And the 38 RC build has already been spun, so I think this missed the Fx38 boat.
https://wiki.mozilla.org/Security/Bug_Approval_Process
Attachment #8601654 -
Flags: approval-mozilla-b2g37?
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 7•10 years ago
|
||
Comment on attachment 8601654 [details] [diff] [review]
aurora_backport_1159321-v0.diff
Though maybe we could get this on mozilla-release for 38.0.5 still. Will leave that to Release Management to decide.
Attachment #8601654 -
Flags: approval-mozilla-beta? → approval-mozilla-release?
Updated•10 years ago
|
Attachment #8601654 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #6)
> Comment on attachment 8601654 [details] [diff] [review]
> aurora_backport_1159321-v0.diff
>
> This should have had sec-approval, no?
I not really sure what the case is here. It was a simplification we wanted to make anyway that's very similar to a large number of other patches I've been landing recently. The only difference with this one is that I noticed it happens to also solve this security issue as a side effect. I'm fairly certain that most of the work I've been doing recently falls into this boat, even if in most cases I don't know exactly where the holes are. :-/
Terence, can you request sec-approval for this before we uplift it to aurora?
This probably will not get into ESR until 39 goes to release, for 38.1.0. ESR 38 and 38.0 builds were done this past Monday and we are hoping not to respin ESR when 38.0.5 is released.
Flags: needinfo?(terrence)
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8601654 [details] [diff] [review]
aurora_backport_1159321-v0.diff
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Probably not easily, but it would be less work than typical pwn2own bugs.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes, although it's not clear how to get from there to a useful exploit.
Which older supported branches are affected by this flaw?
Well-known symbols landed in 36.
If not all supported branches, which bug introduced the flaw?
Well-known symbols
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
It should be simple to backport.
How likely is this patch to cause regressions; how much testing does it need?
This is unlikely to cause regressions. If it builds it should work fine.
Flags: needinfo?(terrence)
Attachment #8601654 -
Flags: sec-approval?
Comment 11•10 years ago
|
||
Comment on attachment 8601654 [details] [diff] [review]
aurora_backport_1159321-v0.diff
Giving sec-approval+ because, well, this is already exposed anyway.
Attachment #8601654 -
Flags: sec-approval? → sec-approval+
Comment on attachment 8601654 [details] [diff] [review]
aurora_backport_1159321-v0.diff
Approved for uplift to beta (39). This already landed on 40, so doesn't need uplift to aurora.
Attachment #8601654 -
Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Comment 13•10 years ago
|
||
I'm marking 38.0.5 as wontfix. As this impacts ESR38 and we are not intending to push an ESR release along with the 38.0.5 release, we should take this fix in 39.
Comment 14•10 years ago
|
||
Comment on attachment 8601654 [details] [diff] [review]
aurora_backport_1159321-v0.diff
Approved for ESR38 but not for Firefox 38.0 or 38.0.5.
Attachment #8601654 -
Flags: approval-mozilla-release?
Attachment #8601654 -
Flags: approval-mozilla-release-
Attachment #8601654 -
Flags: approval-mozilla-esr38?
Attachment #8601654 -
Flags: approval-mozilla-esr38+
Updated•10 years ago
|
tracking-firefox39:
--- → +
tracking-firefox40:
--- → +
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8601654 [details] [diff] [review]
aurora_backport_1159321-v0.diff
I was mistaken: my fix on tip did not actually fix the problem correctly. I'd still like to uplift this to aurora. Same request as before.
Attachment #8601654 -
Flags: approval-mozilla-aurora?
Comment 16•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/86f093c2d995
This doesn't apply cleanly to Aurora, FWIW.
Flags: needinfo?(terrence)
Assignee | ||
Comment 17•10 years ago
|
||
Oh right, sorry, I should have mentioned that we'll need a separate backport for Aurora. Will get that as soon as I can land it on m-i.
Flags: needinfo?(terrence)
Comment 18•10 years ago
|
||
tracking-firefox-esr38:
--- → ?
Comment 19•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
A backport for aurora, once that gets approval.
Attachment #8608268 -
Flags: review+
Assignee | ||
Comment 22•10 years ago
|
||
Note: I've verified using the STR in bug 116518 that the aurora backport fixes the bug.
Comment 23•10 years ago
|
||
Reopening and adjusting the status flags to better reflect that something still needs to land on trunk/aurora here.
Status: RESOLVED → REOPENED
status-firefox41:
--- → affected
Resolution: FIXED → ---
Target Milestone: mozilla40 → ---
Comment 24•10 years ago
|
||
Comment on attachment 8601654 [details] [diff] [review]
aurora_backport_1159321-v0.diff
Approving for 40 too.
Attachment #8601654 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 25•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/e1f8abe63e21
This landed on trunk elsewhere already, so re-closing the bug.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 26•9 years ago
|
||
Is manual testing needed for this fix? If yes, could you please provide some testing steps to verify this?
Flags: needinfo?(terrence)
Updated•9 years ago
|
Whiteboard: [adv-main39+][adv-esr38.1+]
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•