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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 1

[Gerry Chang [:gchang]]

Hi Astley,
Can you help take a look at this?

Comment 2

[Astley Chen (inactive)]

Bevis, please include me into bug 1356558 so that I could have a better context for this issue. Thanks.

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

done!

Comment 4

[Astley Chen (inactive)]

heycam, could you help to look at this potential security issue?

Comment 5

[Cameron McCormack (:heycam)]

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?

Comment 6

[Boris Zbarsky [: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.

Comment 7

[Cameron McCormack (:heycam)]

(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.

Comment 8

[Cameron McCormack (:heycam)]

Well, I got some docshell crashtest assertions when removing the ProcessAttachedQueue call entirely.  But using a script runner seems to work.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Part 1: Stop synchronously running XBL constructors and flushing style in PresShell::Initialize.

Comment 10

[Cameron McCormack (:heycam)]

Attachment: Part 2: Add script blockers around remaining PresShell::{Destroy,Initialize} calls to protect against re-entrant nsDocumentViewer calls.

Comment 11

[Cameron McCormack (:heycam)]

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.

Comment 12

[Cameron McCormack (:heycam)]

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)

Comment 13

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 14

[Astley Chen (inactive)]

(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.

Comment 15

[Timothy Nikkel (:tnikkel)]

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.

Comment 16

[Mats Palmgren (inactive)]

This can land now, IIUC.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Part 2 needs a bit of rebasing.

Comment 18

[Cameron McCormack (:heycam)]

I just tried applying both patches and they seemed to apply fine (part 1 with "offset 5 lines", part 2 cleanly).

Comment 19

[Cameron McCormack (:heycam)]

Attachment: part 1 backport for beta

Comment 20

[Cameron McCormack (:heycam)]

Attachment: part 2 backport for beta

Comment 21

[Cameron McCormack (:heycam)]

Attachment: part 1 backport for beta

Comment 22

[Cameron McCormack (:heycam)]

Attached rebased patches in case it helps.  Could someone check this in to inbound and beta when the trees are open again?  Thanks.

Comment 23

[Cameron McCormack (:heycam)]

I guess I need beta approval still...

Comment 24

[Cameron McCormack (:heycam)]

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

Comment 25

[Cameron McCormack (:heycam)]

(And that approval request applies to both patches.)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 27

[Carsten Book [:Tomcat]]

(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 28

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0bd19cf77dc72ce57aa83daa2b06a40c169ad6
https://hg.mozilla.org/integration/mozilla-inbound/rev/6abb5ce2b1eefd5c0d3cecbec4e721b72eecbd50

Comment 29

[Cameron McCormack (:heycam)]

Attachment: part 1 backport for esr52

Comment 30

[Cameron McCormack (:heycam)]

Attachment: part 2 backport for esr52

Comment 31

[Cameron McCormack (:heycam)]

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.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d0bd19cf77d
https://hg.mozilla.org/mozilla-central/rev/6abb5ce2b1ee

Comment 33

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8881660 [details] [diff] [review]
part 1 backport for beta

sec-high, uaf, beta55+

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/706e0da8ef14
https://hg.mozilla.org/releases/mozilla-beta/rev/388ed6dc3c87

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/057ed884ecb0
https://hg.mozilla.org/releases/mozilla-esr52/rev/dd7ed649b82f

Comment 36

[Andrei Vaida [:avaida]]

(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.