Fix potential reentry into DocumentViewer from call sites which includes nsAutoScriptBlocker.

RESOLVED FIXED in Firefox -esr52

Status

()

defect
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bevis, Assigned: heycam)

Tracking

({csectype-uaf, regression, sec-high})

unspecified
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr5255+ fixed, firefox53 wontfix, firefox54+ wontfix, firefox55+ fixed, firefox56+ fixed)

Details

(Whiteboard: [adv-main55+][adv-esr52.3+])

Attachments

(6 attachments, 1 obsolete attachment)

Bug 1356558 fixed a reentry from ::Hide() to ::Show().

Bug 1356558 comment 26 points out another one that re-enters into nsDocumentViewer functions potentially.

Need an expert from nsDocumentViewer to review the implicit use of nsAutoScriptBlocker to fix these possible reentries into nsDocumentViewer.
Blocks: 871747
Group: core-security → layout-core-security
Hi Astley,
Can you help take a look at this?
Flags: needinfo?(aschen)
Bevis, please include me into bug 1356558 so that I could have a better context for this issue. Thanks.
Flags: needinfo?(btseng)
done!
Flags: needinfo?(btseng)
heycam, could you help to look at this potential security issue?
Flags: needinfo?(aschen) → needinfo?(cam)
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
Timothy/Boris, following on from bug 1356558 comment 26, I'm not sure what the right thing to do here is.  The PresShell::Initialize() call itself can't be done while a script blocker is on the stack, because the BindingManager()->ProcessAttachedQueue() call that it does does will run script.  The code in nsDocumentViewer::InitPresentationStuff after the PresShell::Initialize() mostly null checks the member variables that it wants to use to continue the initialization process, but since we might have re-entrantly set some new pres shell etc. on the nsDocumentViewer, even if non-null they won't have expected values.

(Is it possible, by the way, for the nsDocumentViewer to be destroyed due to the script runners while we're in an nsDocumentViewer method?)

Should we instead be doing some explicit re-entrancy checks on all the public nsIContentViewer methods and just bailing out if we are called re-entrantly?
Flags: needinfo?(tnikkel)
Flags: needinfo?(bzbarsky)
It used to be that PresShell::Initialize did a synchronous reflow.  And before it did that it did XBL constructor processing and restyle processing etc.

We removed that synchronous reflow in bug 378975 but left the rest of that stuff.  But I'm not sure we really need the rest of it.  Specifically:

1)  I see no reason we need the sync ProcessPendingRestyles() call at all.  We'll process restyles before the next time we reflow, as needed.

2)  If we remove that ProcessPendingRestyles call, there's no reason to do a sync ProcessAttachedQueue call.  We can do that off a scriptrunner instead, for minimal change, or via whatever mechanism we normally use for running XBL ctors from frame construction if we're willing to tolerate a bit more risk.

And then I think we can in fact do PresShell::Initialize() under a scriptblocker.

> Is it possible, by the way, for the nsDocumentViewer to be destroyed due to the script runners while we're in an nsDocumentViewer method?

Maybe.  See bug 1365948.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #6)
> 2)  If we remove that ProcessPendingRestyles call, there's no reason to do a
> sync ProcessAttachedQueue call.  We can do that off a scriptrunner instead,
> for minimal change, or via whatever mechanism we normally use for running
> XBL ctors from frame construction if we're willing to tolerate a bit more
> risk.

OK.  Looks like that mechanism is just "call AddPendingBinding in the nsFrameConstructorState destructor", which causes us to run them next style flush.  Since this is meant to be running the XBL constructors for that (now to be removed) ProcessPendingRestyles call, we can just leave them to be run naturally in the next style flush.
Well, I got some docshell crashtest assertions when removing the ProcessAttachedQueue call entirely.  But using a script runner seems to work.
Flags: needinfo?(tnikkel)
Attachment #8873710 - Flags: review?(tnikkel) → review+
Attachment #8873711 - Flags: review?(tnikkel) → review+
Comment on attachment 8873710 [details] [diff] [review]
Part 1: Stop synchronously running XBL constructors and flushing style in PresShell::Initialize.

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

Not easily.


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

From this and the following patch, you can see that we making changes that prevent XBL destructors from running at certain times in nsDocumentViewer.  Comments do say that we do this to prevent re-entrant calls, and it's clear that this is "a bad thing", but it would require some work to determine if and how this is exploitable.


Which older supported branches are affected by this flaw?

I'll say all, in theory.  Bug 1356558, which this bug was spawned from, identified (and fixed) a specific case where we could cause these re-entrant calls.  This patch protects against more cases where we in theory could have these re-entrant calls, but without specific uses of XBL that would trigger them, they are latent problems rather than ones we know could be abused right now.


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

-


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

I don't, but I don't think this code is touched very often, so it shouldn't be hard or risky to.


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

Unsure.  The two major changes -- running the XBL destructors slightly later (in a script runner) and not synchronously flushing styles -- seem like they shouldn't cause regressions, since none of the callers to these functions should care that they aren't done synchronously now.  Existing tests pass.
Attachment #8873710 - Flags: sec-approval?
Comment on attachment 8873711 [details] [diff] [review]
Part 2: Add script blockers around remaining PresShell::{Destroy,Initialize} calls to protect against re-entrant nsDocumentViewer calls.

[Security approval request comment]
(as above)
Attachment #8873711 - Flags: sec-approval?
This has missed the Firefox 54 window. It's too late to land and make any betas. We're making the release build soon.

So, this can't land until June 27, two weeks into the Firefox 55 cycle. I'll give it sec-approval to land *then* but don't land it before June 27.
Whiteboard: [checkin on 6/27]
Attachment #8873710 - Flags: sec-approval? → sec-approval+
Attachment #8873711 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #13)
> So, this can't land until June 27, two weeks into the Firefox 55 cycle. I'll
> give it sec-approval to land *then* but don't land it before June 27.

Looks like you are suggesting it's a wontfix in FF54. Please correct me if I'm wrong.
Came across some crashes in nsDocumentViewer::Show on crashstats today that could only be explained by the issues being fixed by this patch. So the issue is definitely real and not theoretical.
This can land now, IIUC.
Keywords: checkin-needed
Part 2 needs a bit of rebasing.
Flags: needinfo?(cam)
Keywords: checkin-needed
Whiteboard: [checkin on 6/27]
I just tried applying both patches and they seemed to apply fine (part 1 with "offset 5 lines", part 2 cleanly).
Flags: needinfo?(cam)
Attachment #8881658 - Attachment is obsolete: true
Attached rebased patches in case it helps.  Could someone check this in to inbound and beta when the trees are open again?  Thanks.
Keywords: checkin-needed
I guess I need beta approval still...
Comment on attachment 8881660 [details] [diff] [review]
part 1 backport for beta

Approval Request Comment
[Feature/Bug causing the regression]: unsure
[User impact if declined]: potential (though so far unproven to be triggerable) security issue would not be protected against
[Is this code covered by automated tests?]: probably don't have great coverage here
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: Unsure.
[Why is the change risky/not risky?]: The two major changes -- running the XBL destructors slightly later (in a script runner) and not synchronously flushing styles -- seem like they shouldn't cause regressions, since none of the callers to these functions should care that they aren't done synchronously now.  Existing tests pass.
[String changes made/needed]: none
Attachment #8881660 - Flags: approval-mozilla-beta?
(And that approval request applies to both patches.)
I still had issues with the original trunk patches, but the beta ones mostly worked for inbound (modulo one easy to resolve conflict on part 2).

https://hg.mozilla.org/integration/mozilla-inbound/rev/2db98c5fe22c9f10f87ba118577c654a992e3eed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9b2f155996d7c2c22ac270042a7ed6e45275b1c

Also, part 2 will need a rebased patch for ESR52 when you get a chance. Part 1 grafts OK.
Flags: needinfo?(cam)
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #26)
> I still had issues with the original trunk patches, but the beta ones mostly
> worked for inbound (modulo one easy to resolve conflict on part 2).
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 2db98c5fe22c9f10f87ba118577c654a992e3eed
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> d9b2f155996d7c2c22ac270042a7ed6e45275b1c
> 
> Also, part 2 will need a rebased patch for ESR52 when you get a chance. Part
> 1 grafts OK.

sorry this caused bustage and had to backout for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=110323595&repo=mozilla-inbound
Comment on attachment 8882500 [details] [diff] [review]
part 1 backport for esr52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: 
User impact if declined: potential (though so far unproven to be triggerable) security issue would not be protected against
Fix Landed on Version: 56
Risk to taking this patch (and alternatives if risky): The two major changes -- running the XBL destructors slightly later (in a script runner) and not synchronously flushing styles -- seem like they shouldn't cause regressions, since none of the callers to these functions should care that they aren't done synchronously now.  Existing tests pass.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8882500 - Flags: approval-mozilla-esr52?
Group: layout-core-security → core-security-release
Target Milestone: --- → mozilla56
Comment on attachment 8881660 [details] [diff] [review]
part 1 backport for beta

sec-high, uaf, beta55+
Attachment #8881660 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8881659 - Flags: approval-mozilla-beta+
Attachment #8882500 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Depends on: 1377406
(In reply to Cameron McCormack (:heycam) from comment #24)
> [Is this code covered by automated tests?]: probably don't have great
> coverage here
> [Has the fix been verified in Nightly?]: no
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Cameron's assessment on manual testing needs.
Flags: qe-verify-
Whiteboard: [adv-main55+][adv-esr52.3+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.