Possible missing read barrier when preserving a wrapper object

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

({sec-audit})

unspecified
mozilla58
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 fixed)

Details

(Whiteboard: [adv-main58-][post-critsmash-triage])

Attachments

(1 attachment)

Assignee

Description

2 years ago
Looking at the wrapper cache, I noticed that we don't have a read barrier when when we preserve a wrapper object.  This could be problematic if this happens in an incremental GC after we have already marked the preserved wrappers.  

To add this we would just call ExposeObjectToActiveJS() when preserving the wrapper.

I'm not sure if this is a real problem.  It may be that we only preserve wrappers that have already been exposed or are known to be live.  The paths I checked were all OK.  And I don't know how bad it is if a supposedly-preserved wrapper is collected. 

Interestingly, in debug builds we end up exposing the wrapper anyway because nsWrapperCache::CheckCCWrapperTraversal() calls GetWrapper().  This might be hiding problems.
Assignee

Comment 1

2 years ago
Here's a patch to add the read barrier.  What do you think?
Attachment #8925026 - Flags: review?(bugs)
Group: javascript-core-security → dom-core-security
(In reply to Jon Coppeard (:jonco) from comment #0)
> Interestingly, in debug builds we end up exposing the wrapper anyway because
> nsWrapperCache::CheckCCWrapperTraversal() calls GetWrapper().  This might be
> hiding problems.

That should almost certainly be GetWrapperPreserveColor() instead.
Attachment #8925026 - Flags: review?(bugs) → review+
Assignee

Comment 3

2 years ago
Comment on attachment 8925026 [details] [diff] [review]
bug1414279-expose-preserved-wrapper

[Security approval request comment]
How easily could an exploit be constructed based on the patch? 

I'd say very difficult.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? 

No.

Which older supported branches are affected by this flaw? 

Everything back to when we added incremental GC.

If not all supported branches, which bug introduced the flaw?

All supported branches.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Should be trivial.

How likely is this patch to cause regressions; how much testing does it need?

We're already doing this expose call in debug builds, so doing it in opt builds too is unlikely to cause problems.
Attachment #8925026 - Flags: sec-approval?
Comment on attachment 8925026 [details] [diff] [review]
bug1414279-expose-preserved-wrapper

sec-approval=dveditz
Attachment #8925026 - Flags: sec-approval? → sec-approval+
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/36da5e7e5ad7
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Group: dom-core-security → core-security-release
This doesn't fit the ESR52 uplift criteria AFAICT. Feel free to set the status back to affected and nominate it for approval if you feel strongly otherwise, however.
Whiteboard: [adv-main58-]
Flags: qe-verify-
Whiteboard: [adv-main58-] → [adv-main58-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.