Bugzilla

Andrew McCreight [:mccr8]

Updated

•

7 years ago

Group: core-security → dom-core-security

Comment 3

•

7 years ago

Can you take a look, heycam? Thanks.

Group: dom-core-security → layout-core-security

Component: DOM: CSS Object Model → CSS Parsing and Computation

Flags: needinfo?(cam)

Andrew McCreight [:mccr8]

Comment 4

•

7 years ago

Though I do see some SMIL here so maybe it is more of an SVG issue than a CSS issue, per se.

Comment hidden (obsolete)

INFO: Last good revision: 06ec956f753346236bf89f2bead2066d22778fc2
INFO: First bad revision: 5115e2dea29a22fbaac7ad9db8723de762ec4ddd
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=06ec956f753346236bf89f2bead2066d22778fc2&tochange=5115e2dea29a22fbaac7ad9db8723de762ec4ddd

Only reproduces with Stylo enabled, so setting the status for 56 to disabled even though the regressing change landed there.

Comment 6

•

7 years ago

Sorry, that regression range is bogus (it was actually the commit prior, which was just "Build Stylo on Windows").

Comment 7

•

7 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)
> Sorry, that regression range is bogus (it was actually the commit prior,
> which was just "Build Stylo on Windows").

Hm interesting. So this is a regression from stylo, despite being in the old style system code?

Nils, does this reproduce with/without the layout.css.servo.enabled pref set?

Flags: needinfo?(nils)

Comment 8

•

7 years ago

On Linux, I was only able to bisect down to the range below before mozregression started erroring out on me :(
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=edb7e1ddd9b61e2af2a75cfe5baa0f92a54a2716&tochange=594cc32b632396a867ef1f98428968b224d82151

Comment 9

•

7 years ago

If I manually set layout.css.servo.enabled to false, no crash on today's Windows nightly.

Also, I managed to get mozregression working again. Bisected down to (doubly-verified!):
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f3e0f0a4ab7b48483dcb6c6283220282b7aec651&tochange=ecf7e90cecf61941db73659203ea09c28ef7a417

Blocks: 1376805
No longer blocks: 1377169

Comment 10

•

7 years ago

Ok, so this is presumably a mismatch between places where we're using the old and new style systems.

Priority: -- → P2

Summary: heap-use-after-free in nsRuleNode::Transition → stylo: heap-use-after-free in nsRuleNode::Transition

Andrew McCreight [:mccr8]

Comment 11

•

7 years ago

Xidorn has been some of these issues recently. Xidorn, can you have a look?

Flags: needinfo?(xidorn+moz)

Flags: needinfo?(nils)

Flags: needinfo?(cam)

Updated

•

7 years ago

Keywords: csectype-uaf, regression, sec-critical

Comment 12

•

7 years ago

In a debug build, we hit this fatal assertion:
> Assertion failure: IsGecko(), at ../../dist/include/mozilla/DeclarationBlockInlines.h:15

(I'm not sure if the assertion-failure is before or after the UAF, because my build isn't ASAN-enabled, but I'm guessing it's before.  The assertion failure happens in style resolution inside of nsCSSFrameConstructor::ContentInserted. I'll attach a backtrace.)

Comment 13

•

7 years ago

Attached file backtrace of "Assertion failure: IsGecko()" — Details

Comment 14

•

7 years ago

Attached image (helper SVG file for testcase 2) — Details

Manish Goregaokar [:manishearth]

Comment 15

•

7 years ago

Attached file testcase 2 (triggers a fatal assertion in debug builds) — Details

Comment 16

•

7 years ago

IIRC the about:blank patch showed up in another bisection for a different bug that had to do with cross-style-system iframing or something.

Comment 17

•

7 years ago

See the source of "testcase 2" (which reproduces the aforementioned assert for me, in my debug build) -- it's much easier to follow than the original testcase.

Basically, the testcase makes us transplant the root element of a stylo-enabled iframe into a different presumably-non-stylo-enabled iframe (where the second iframe's contents are set via ".srcdoc", which I guess leaves it at an about: flavored page, based on the regression source).

And that triggers the "nsCSSFrameConstructor::ContentInserted [...] Assertion failure: IsGecko" backtrace shown in  attachment 8909496 [details].

Updated

•

7 years ago

Attachment #8909510 - Attachment description: testcase 2 → testcase 2 (triggers a fatal assertion in debug builds)

Comment 18

•

7 years ago

Ugh, this is similar to bug 1389300.

I think we should do two things here:
(1) Figure out how content is able to trigger a non-servo-backed document, and prevent that.
(2) Lazily check for parsed style attributes with the wrong style backend in BindToTree, and fix them up somehow. This will give us belt-and-suspenders protection in the case that we miss other cases of (1).

Boris Zbarsky [:bzbarsky]

Assignee

Comment 19

•

7 years ago

OK, I'll investigate this.

Assignee: nobody → xidorn+moz

Flags: needinfo?(xidorn+moz)

Comment 20

•

7 years ago

> (1) Figure out how content is able to trigger a non-servo-backed document, and prevent that.

Because the check in nsIDocument::UpdateStyleBackendType that was added in bug 1376805 is wrong.  It's checking the document URI.  For srcdoc documents, the document URI is "about:srcdoc", per spec.

I _think_ the other about: URIs we have right now are not really able to be same-origin with web pages, but chrome scripts can probably still move nodes between them and content documents..

Boris Zbarsky [:bzbarsky]

Assignee

Comment 21

•

7 years ago

I think there are several approaches we can fix this. The simplest one would be that we just add an additional check and use servo backend for about:srcdoc as well.

In addition to that, it seems to me that an about:blank page with fragment in the url can potentially trigger the same problem, since NS_IsAboutBlank returns false in that case. But I cannot write a reliable testcase to trigger it that way. (I think I succeeded once, but failed to reproduce it anymore.) So probably we want to just check the path of the URI when it's in about scheme.

Alternatively, we can somehow inherit the style backend from the parent document. My initial thought is that we can just call GetParentDocument(), and if there is any, we simply inherit it and return. This would stop this kind of issues as a whole. But the comment of GetParentDocument() says
> Note that this parent chain may cross chrome boundaries.
I'm confused, and a bit concerned whether we would effectively disable stylo in some case if we inherit backend type.

bz, what do you think about inheriting backend type from parent document? Do we need any extra check if using that approach?

Flags: needinfo?(bzbarsky)

Comment 22

•

7 years ago

Just inheriting from parent document would work for about:srcdoc, and seems like a good idea in case we have a srcdoc in chrome somewhere.

For about:blank things are more complicated because you can have window.open("about:blank") and in that case ideally you'd inherit from your opener...

One option that I sort of like is to use the principal instead of the document URI and disable stylo for now for system principal or "about:*" codebase principal.  That would automatically handle about:blank and srcdoc, since those inherit principals in the cases we need to worry about.

If you use GetParentDocument() then it depends on what you're using it on.  It _can_ chross chrome boundaries, e.g. in a non-e10s window GetParentDocument for a root content page is the browser.xul document.  And I don't know what situations webextensions can create with content pages in the chrome process...  If you just use GetParentDocument() for about:srcdoc, that should be fine, I'd think, at least for inheriting stylo-enabled state.

Flags: needinfo?(bzbarsky)

Assignee

Comment 23

•

7 years ago

Attached file simplified testcase — Details

Also, a separate svg file is not necessary. A in-tree svg element is enough to trigger this bug.

Assignee

Comment 24

•

7 years ago

Attached patch patch — Details — Splinter Review

This patch uses principal for deciding whether we use stylo for a document.

We blacklist documents basically just for unimplemented XUL bits. And whether a document can use XUL is decided by nsIDocument::AllowXULXBL [1] which calls into nsDocument::InternalAllowXULXBL [2] when the flag isn't specified, and nsDocument::InternalAllowXULXBL calls nsContentUtils::AllowXULXBLForPrincipal [3] with the node principal of the document.

AllowXULXBLForPrincipal checks three things:
* if it is system principal, allow, otherwise
* if scheme of principal is file, and file is allowed to use xul, allow, otherwise
* whether the site has allowXULXBL permission in permission manager

The second thing is controlled by pref dom.allow_XUL_XBL_for_file, which is off by default but on for reftest [4], so it doesn't matter, and we shouldn't respect it.

The allowXULXBL permission isn't mentioned anywhere other than tests and this very place which checks it [5], so I believe it doesn't matter either.

XUL support can also be enabled via calling nsIDocument::ForceEnableXULXBL [6], which is only called when a document is created by some system principal thing.

Based on the analysis above, I think it is mostly safe to do this change. It can still have some risk, but the positive side is that we would have lower risk to see this kind of bugs again, because content page can never get system principal, and thus we should never have a content page which can operate anything cross backend.


[1] https://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/dom/base/nsIDocument.h#2340-2344
[2] https://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/dom/base/nsDocument.cpp#5243-5253
[3] https://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/dom/base/nsContentUtils.cpp#7064-7077
[4] https://searchfox.org/mozilla-central/search?q=dom.allow_XUL_XBL_for_file
[5] https://searchfox.org/mozilla-central/search?q=allowXULXBL&case=true
[6] https://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/dom/base/nsIDocument.h#2346-2348

Attachment #8909654 - Flags: review?(bobbyholley)

Assignee

Comment 25

•

7 years ago

try push looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48ad943488efd80c71352ca8c5207fd11e079951

(I know that we generally shouldn't do try push for security bugs, but this change doesn't really reveal anything about this bug, so I guess it's fine.)

Assignee

Comment 26

•

7 years ago

I also checked with this patch, that it doesn't regress bug 1376659.

Comment 27

•

7 years ago

Comment on attachment 8909654 [details] [diff] [review]
patch

Review of attachment 8909654 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, glad to see something simple.

Can we also add release-mode (or at least diagnostic) assertions against adopting an element that has a pre-existing style attribute with the wrong backend?

Attachment #8909654 - Flags: review?(bobbyholley) → review+

Assignee

Comment 29

•

7 years ago

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #27)
> Can we also add release-mode (or at least diagnostic) assertions against
> adopting an element that has a pre-existing style attribute with the wrong
> backend?

OK, I'll try to write a patch. But I'm going to land this patch first.

Keywords: leave-open

Assignee

Comment 30

•

7 years ago

https://hg.mozilla.org/integration/autoland/rev/18723c6944b1

Assignee

Comment 31

•

7 years ago

FWIW, when working with the assertion, I found another case that XSLTProcessor seems to be able to generate Gecko-backed document, which is also fixed by the patch landed here.

Assignee

Comment 32

•

7 years ago

Attached patch patch for the assertion (obsolete) — Details — Splinter Review

Attachment #8910130 - Flags: review?(bobbyholley)

Comment 33

•

7 years ago

Comment on attachment 8910130 [details] [diff] [review]
patch for the assertion

Review of attachment 8910130 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have time to check before going to bed, but does nsIDocument::AdoptNode get called for all the implicit adoption cases that happen via CloneAndAdopt etc?

::: dom/base/nsDocument.cpp
@@ +7792,5 @@
>    }
>  }
>  
> +// Recursively check whether this node or its descendants contain any
> +// pre-existing style declaration with it.

I think we also want to check for the element flags, because those have different meaning across the style backends, which was the cause of the assertion in the bug I duped to this one.

Attachment #8910130 - Flags: review?(bobbyholley)

Assignee

Comment 34

•

7 years ago

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #33)
> Comment on attachment 8910130 [details] [diff] [review]
> patch for the assertion
> 
> Review of attachment 8910130 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't have time to check before going to bed, but does
> nsIDocument::AdoptNode get called for all the implicit adoption cases that
> happen via CloneAndAdopt etc?

nsNodeUtils::CloneAndAdopt recursively calls itself for children of the node, so I suppose nsIDocument::AdoptNode isn't recursive.

> ::: dom/base/nsDocument.cpp
> @@ +7792,5 @@
> >    }
> >  }
> >  
> > +// Recursively check whether this node or its descendants contain any
> > +// pre-existing style declaration with it.
> 
> I think we also want to check for the element flags, because those have
> different meaning across the style backends, which was the cause of the
> assertion in the bug I duped to this one.

Hmm, okay.

Assignee

Comment 35

•

7 years ago

Attached patch patch for the assertion — Details — Splinter Review

Attachment #8910130 - Attachment is obsolete: true

Attachment #8910142 - Flags: review?(bobbyholley)

Comment 36

•

7 years ago

https://hg.mozilla.org/mozilla-central/rev/18723c6944b1

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox57: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla57

Comment 37

•

7 years ago

Bah.

Status: RESOLVED → REOPENED

status-firefox57: fixed → affected

Resolution: FIXED → ---

Target Milestone: mozilla57 → ---

Assignee

Comment 38

•

7 years ago

I put the leave-open keyword... well, it's fine to mark it FIXED, though, since it is fixed :)

Status: REOPENED → RESOLVED

Closed: 7 years ago → 7 years ago

status-firefox57: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla57

Al Billings [:abillings - ex-MoCo]

Assignee

Comment 39

•

7 years ago

Wait... I didn't change the flag... it seems bugzilla somehow confused on the conflict :/

Status: RESOLVED → REOPENED

Resolution: FIXED → ---

Updated

•

7 years ago

tracking-firefox57: ? → ---

Comment 40

•

7 years ago

Comment on attachment 8910142 [details] [diff] [review]
patch for the assertion

Review of attachment 8910142 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that one change. Can you move this patch into another, non-P2 bug so we can resolve this one right now? It's just belt-and-suspenders, and I'm trying to keep track of all the unresolved P2s before the merge and so trying to minimize noise. Thanks!

::: dom/base/nsDocument.cpp
@@ +7800,5 @@
> +  if (aNode->IsElement()) {
> +    Element* elem = aNode->AsElement();
> +    if (elem->GetInlineStyleDeclaration() ||
> +        elem->GetSMILOverrideStyleDeclaration() ||
> +        elem->HasAnyOfFlags(ELEMENT_SHARED_RESTYLE_BITS)) {

Also check HasServoData?

Attachment #8910142 - Flags: review?(bobbyholley) → review+

Assignee

Comment 41

•

7 years ago

OK.

Status: REOPENED → RESOLVED

Closed: 7 years ago → 7 years ago

Resolution: --- → FIXED

Assignee

Updated

•

7 years ago

Keywords: leave-open

Updated

•

7 years ago

Group: layout-core-security → core-security-release

Assignee

Updated

•

7 years ago

Depends on: 1402094

Matt Wobensmith [:mwobensmith][:matt:]

Assignee

Updated

•

7 years ago

Blocks: 1391141

Comment 42

•

7 years ago

Confirmed issue on Nightly Fx57.0a1, 2017-09-16.
Verified fixed on Fx57.0b14.

Flags: qe-verify+

Whiteboard: [post-critsmash-triage]

Al Billings [:abillings - ex-MoCo]

Updated

•

6 years ago

Flags: sec-bounty?

Updated

•

6 years ago

Flags: sec-bounty? → sec-bounty+