Closed Bug 1159321 Opened 9 years ago Closed 9 years ago

Well-known symbols in jsid's should not fire pre-barriers

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

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)

Like permanent atoms, I think we need to exclude these from barriers as they may be used from different runtimes.
Why is this a security problem, and how severe is it?
Flags: needinfo?(terrence)
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
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)
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 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?
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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?
Attachment #8601654 - Flags: review?(jcoppeard) → review+
(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)
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 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+
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 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+
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?
https://hg.mozilla.org/releases/mozilla-beta/rev/86f093c2d995

This doesn't apply cleanly to Aurora, FWIW.
Flags: needinfo?(terrence)
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)
A backport for aurora, once that gets approval.
Attachment #8608268 - Flags: review+
Note: I've verified using the STR in bug 116518 that the aurora backport fixes the bug.
Reopening and adjusting the status flags to better reflect that something still needs to land on trunk/aurora here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla40 → ---
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/e1f8abe63e21

This landed on trunk elsewhere already, so re-closing the bug.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Is manual testing needed for this fix? If yes, could you please provide some testing steps to verify this?
Flags: needinfo?(terrence)
No, manual testing is not needed.
Flags: needinfo?(terrence)
Whiteboard: [adv-main39+][adv-esr38.1+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.