1443233 - stylo: heap write hazard in Gecko_GetAnonymousContentForElement: external call to nsCanvasFrame::AppendAnonymousContentTo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Steve Fink [:sfink] [:s:]]

Here's the hazard:

Error: External function
Location: _ZN13nsCanvasFrame24AppendAnonymousContentToER8nsTArrayIP10nsIContentEj$void nsCanvasFrame::AppendAnonymousContentTo(nsTArray<nsIContent*>*, uint32) ### SafeArguments: <arg0> <arg1>
Stack Trace:
_ZL38AppendNativeAnonymousChildrenFromFrameP8nsIFrameR8nsTArrayIP10nsIContentEj$nsContentUtils.cpp:void AppendNativeAnonymousChildrenFromFrame(nsIFrame*, class nsTArray<nsIContent*>*, uint32) @ dom/base/nsContentUtils.cpp#10528 ### SafeArguments: aKids aFlags
_ZN14nsContentUtils29AppendNativeAnonymousChildrenEPK10nsIContentR8nsTArrayIPS0_Ej$void nsContentUtils::AppendNativeAnonymousChildren(nsIContent*, class nsTArray<nsIContent*>*, uint32) @ dom/base/nsContentUtils.cpp#10548 ### SafeArguments: aKids aFlags
Gecko_GetAnonymousContentForElement @ layout/style/ServoBindings.cpp#177

The problem is that nsCanvasFrame::AppendAnonymousContentTo(nsTArray<nsIContent*>*, uint32) is incorrectly considered to be unknown, because the callgraph only shows nsCanvasFrame::AppendAnonymousContentTo(class nsTArray<nsIContent*>*, uint32) (notice the "class " prefix.)

I think this will probably be in sixgill, and I even have a memory of changing something related. (It's possible the reporting error led me to falsely thinking that my change was ok.)

Comment 1

[Steve Fink [:sfink] [:s:]]

This is happening for virtual function resolution. The JS code iterates over all function members of relevant types, each of which is associated with a stringified type. It's that type that is missing the "class ". These are stored directly in src_comp.xdb, so it does appear to be a sixgill issue.

Comment 2

[Steve Fink [:sfink] [:s:]]

Ugh. So I fixed this, but it requires updating sixgill, which is a toolchain job that has to pull down gcc and some things it depends on, which includes mpc-0.8.2, which has a signature that our build script checks, and that signature seems to only exist on the original download server (http://www.multiprecision.org), and that server is down. :-(

glandium: https://taskcluster-artifacts.net/eKECsheNRdymvIliGmCPOQ/0/public/logs/live_backing.log shows the failure fetching http://www.multiprecision.org/downloads/mpc-0.8.2.tar.gz

It's not hard to find another copy floating around on the web, and some of them even have checksums (from the same server). If I happened to already have a known-good copy, I would hack the build script to rely on a checksum committed to the tree instead of the gpg signature. But I don't, and it seems weird to say "the security mechanism here is unavailable so I'll download some random thing from the web and trust it."

We probably need to do something about this single point of failure.

Comment 3

[Steve Fink [:sfink] [:s:]]

Oops, meant to needinfo glandium for the previous comment.

Comment 4

[Mike Hommey [:glandium]]

Well, it works, now.

Comment 5

[Mike Hommey [:glandium]]

As for the SPoF, we're probably going to cache those upstream files.

Comment 6

[Daniel Veditz [:dveditz]]

This is reporting a bug in sixgill (false positive), or a security bug in Firefox that sixgill should have caught? I'm not sure how to rate/triage this as a hidden security bug.

Comment 7

[Steve Fink [:sfink] [:s:]]

It's a bug in sixgill, very probably a false positive, but it's one that could be concealing other true positives. I would say sec-audit.

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: decl_as_string_flags.patch

Ok, I could have sworn I submitted a review request for this already. I must have written up the comment but then lost it somehow.

This is https://hg.mozilla.org/users/sfink_mozilla.com/sixgill/rev/739c064cd73c

Note that I'm planning on landing several other sixgill changes with this one; I would be updating from 59b74c2e21bd to 39b87ac48871 on https://hg.mozilla.org/users/sfink_mozilla.com/sixgill so feel free to look at any of these changes:

0b44324477fa Cross-check the compile-time plugin headers vs the runtime host gcc
4c4b8a423acb Improve lambda check (there can be <lambda(foo)>)
f5d7d11116db File-qualify anonymous structs
90be728e1624 Hack for lambdas that return their own type
aeabb6e70a8c minor unsigned comparison fix
5e83950f12de Handle the case where DECL_CONTEXT(returnval) is null
739c064cd73c Pass same flags to decl_as_string for unnamed CSUs as used for all other _as_string calls
935a5a61a15c wrong header for VOID_CST check, at least for newer gcc
3fc74e1b5b05 Refactor debug logging for basecc
635449e1c280 Handle a few more compile flags

Want to set up a github repo for this?

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: sixgill-changes.patch

I guess I'll ask for blanket review of all of those changes with this. Note that something in here is a prerequisite for gcc6 as well.

Comment 10

[Brian Hackett [Laid off!]]

Comment on attachment 8958312 [details] [diff] [review]
sixgill-changes.patch

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

This looks like the wrong patch.

Comment 11

[Steve Fink [:sfink] [:s:]]

(In reply to Brian Hackett (:bhackett) from comment #10)
> 
> This looks like the wrong patch.

It's the right patch, it just doesn't have any of the relevant code. See comment 8 for links to see the actual code. (It's a whole series of patches; I'm not sure which ones you'd find worth reviewing.)

But I'm inclined to push this and address review comments later.

I know. I'll push this, but create a non-security bug for the rest of the sixgill changes and attach the patches directly there.

Comment 12

[Steve Fink [:sfink] [:s:]]

Forked off bug 1445553.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26dacf2a27a7566534d9c312b72cb9d1202db755
https://hg.mozilla.org/mozilla-central/rev/26dacf2a27a7