Closed Bug 1332569 Opened 3 years ago Closed 3 years ago

Assertion failure: !mDocElementContainingBlock (Shouldn't have a doc element containing block here), at nsCSSFrameConstructor.cpp:2389

Categories

(Core :: XSLT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- wontfix
firefox51 --- wontfix
firefox52 + fixed
firefox53 + fixed
firefox54 + fixed

People

(Reporter: cbook, Assigned: tnikkel)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: assertion, csectype-uaf, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main52+] possible UAF)

Attachments

(2 files)

Found via bughunter and reproduced with latest tinderbox debug build trunk on windows.

Steps to reproduce:
-> Load http://cmarcp.or.ke/ (had to reload this several times)

--> Assertion failure (a assertion warning first and then the assertion failure)

[4048] ###!!! ASSERTION: asked to construct a frame for a node that already has a frame: '!child->GetPrimaryFrame() || child->GetPrimaryFrame()->GetContent() != child', file c:/builds/moz2_slave/m-aurora-w32-d-000000000000000/build/src/layout/base/nsCSSFrameConstructor.cpp, line 7695
[4048] ###!!! ASSERTION: root element frame already created: 'nullptr == mRootElementFrame', file c:/builds/moz2_slave/m-aurora-w32-d-000000000000000/build/src/layout/base/nsCSSFrameConstructor.cpp, line 7740

and then

Assertion failure: !mDocElementContainingBlock (Shouldn't have a doc element containing block here), at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:2389
Affects Beta -> Trunk debug builds according to the data. Jet do you know who could take a look ?
Flags: needinfo?(bugs)
Attached file stack
The document that we assert is in an iframe in the page (http://live.mystocks.co.ke/ticker/tape$). The first time that we call ConstructDocElementFrame is via this stack

#01: nsCSSFrameConstructor::ConstructDocElementFrame
#02: nsCSSFrameConstructor::ContentRangeInserted
#03: nsCSSFrameConstructor::ContentInserted
#04: mozilla::PresShell::Initialize
#05: nsDocumentViewer::InitPresentationStuff
#06: nsDocumentViewer::Show()
#07: nsDocShell::SetVisibility(bool)
#08: non-virtual thunk to nsDocShell::SetVisibility(bool)
#09: nsFrameLoader::Show(int, int, int, int, nsSubDocumentFrame*)
#10: nsSubDocumentFrame::ShowViewer()
#11: AsyncFrameInit::Run()
#12: nsContentUtils::RemoveScriptBlocker()
#13: mozilla::PresShell::DidCauseReflow()
#14: nsAutoCauseReflowNotifier::~nsAutoCauseReflowNotifier()
#15: nsAutoCauseReflowNotifier::~nsAutoCauseReflowNotifier()
#16: mozilla::PresShell::Initialize(int, int)

where the bottom PresShell::Initialize is for the main doc. Then nsXMLContentSink::OnTransformDone sends a ContentInserted notification for the root element, and then it calls nsContentSink::StartLayout. Since the PresShell was created before the StartLayout call the ContentInserted notification makes its way to the presshell, which already found the root element when Initialize was called. So nsXMLContentSink::OnTransformDone is relying on the PresShell not being created at this point.

(This also points out a third way that PresShell::Initialize can be called: 1) after all pending style sheets have been loaded 2) if a script or something else causes a flush, and what happens in this bug 3) the subdocument frame containing the document is created.)
Flags: needinfo?(bzbarsky)
nsDocumentViewer::Show asks the document if it MayStartLayout(), and that being true is what allows PresShell::Initialize to be called. nsIDocument::StartDocumentLoad is what should be setting MayStartLayout() to false, but StartDocumentLoad only gets called on the source xml document, never on the output html doc. It looks like txMozillaXMLOutput::createResultDocument is doing similar work to what StartDocumentLoad would be doing (setting readystate from uninitialized to loading).

So either we should be calling SetMayStartLayout in txMozillaXMLOutput::createResultDocument, or we should rework things so that StartDocumentLoad is called for the resulting document for transformed documents.

Call SetMayStartLayout seems to fix it, but I'm not sure if that's the right fix.

(In reply to Timothy Nikkel (:tnikkel) from comment #3)
> (This also points out a third way that PresShell::Initialize can be called:
> 1) after all pending style sheets have been loaded 2) if a script or
> something else causes a flush, and what happens in this bug 3) the
> subdocument frame containing the document is created.)

So this isn't actually a third way, it's a bug that should be allowed.
(In reply to Timothy Nikkel (:tnikkel) from comment #4)
> So this isn't actually a third way, it's a bug that should be allowed.

_shouldn't_ be allowed.
Component: CSS Parsing and Computation → XSLT
The simple fix (calling SetMayStartLayout in txMozillaXMLOutput::createResultDocument) is green on try FWIW

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94c4da66e33f536c65f5c26ab3ae365a2d683f96
FWIW, I think it's incorrect for mMayStartLayout to default to true in any class nsIDocument, if we're to believe the comment above it:

  // If true, whoever is creating the document has gotten it to the
  // point where it's safe to start layout on it.
  bool mMayStartLayout : 1;

It seems safer to start out false, then set to true only for the documents where we know we're going to layout.
Flags: needinfo?(bugs)
We could try that; I expect us to run into issues around documents created via CreateAboutBlankContentViewer, at the very least.

That is, we can start off as false, but then we have to find all the places that really do want true... otherwise the failure mode is no layout in various cases.

It probably makes the most sense to do the XSLT change for branches and the larger fix for trunk in a followup....
Flags: needinfo?(bzbarsky)
Attached patch easy patchSplinter Review
Assignee: nobody → tnikkel
Attachment #8829644 - Flags: review?(bzbarsky)
Comment on attachment 8829644 [details] [diff] [review]
easy patch

r=me
Attachment #8829644 - Flags: review?(bzbarsky) → review+
Depends on: 1333222
I filed bug 1333222 for the followup, I'm leaving it to those who know dom/ better.
I think this might be a security issue. I don't think anything will destroy the "extra" frame tree, and so when the presshell goes away things could have references to the "extra" frame tree and the "extra" frame tree could still have pointers to things that are dead. Haven't verified any of this, but being cautious.
Group: core-security
Comment on attachment 8829644 [details] [diff] [review]
easy patch

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

see next answer

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

the patch would point an attacker in the direction of XSL, but there is still a ways to go after that

Which older supported branches are affected by this flaw?

probably all supported branches

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

probably all supported branches

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

trivial

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

should be fairly safe
Attachment #8829644 - Flags: sec-approval?
Group: core-security → dom-core-security
[Tracking Requested - why for this release]:

marking per comment #13
[Tracking Requested - why for this release]:

Sounds like it might be possible to trigger a UAF when the presShell is destroyed, but we haven't actually captured that happening so calling this sec-moderate for now rather than sec-high. But it would be good to get it into beta Firefox 52 (the next ESR) just in case.
Comment on attachment 8829644 [details] [diff] [review]
easy patch

sec-approval=dveditz

I'll assume this patch works for aurora since we just merged, and approve that there. This is likely to work as-is for beta too.
Attachment #8829644 - Flags: sec-approval?
Attachment #8829644 - Flags: sec-approval+
Attachment #8829644 - Flags: approval-mozilla-beta+
Attachment #8829644 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/b6829cff38ee
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Group: dom-core-security → core-security-release
Flags: qe-verify+
Whiteboard: possible UAF → [post-critsmash-triage] possible UAF
Whiteboard: [post-critsmash-triage] possible UAF → [post-critsmash-triage][adv-main52+] possible UAF
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.