Bugzilla

Reporter

Comment 1

•

6 years ago

Attached file fuzzer.js (obsolete) — Details

Daniel Veditz [:dveditz]

Comment 2

•

6 years ago

I don't see any WebGL in your testcase or the stack. How'd you end up picking the component?

I'm not sure where to put this. Most of the stack is CSS/Frames (Layout) which makes sense from your testcase. But I'm surprised to see anything in nsCSSFrameConstructor calling nsINode::doInsertChildAt. There is an appendChild early in your testcase (can you remove the lines after that, or are they necessary?) but it shouldn't have that stack. And there's no XBL at all in your testcase, though maybe that's just a thing we check the presence of and the node itself is corrupt.

I'm going to call this "Layout" on the preponderance of the stack, but I'm not sure. What happens in an ASAN build?

the null deref doesn't look like a security bug, but that stack is crazy so let's figure that out before unhiding the bug.

Group: core-security → layout-core-security

Component: Canvas: WebGL → Layout

Flags: needinfo?(jkratzer)

Reporter

Comment 3

•

6 years ago

Attached file trigger.html — Details

Attachment #8987708 - Attachment is obsolete: true

Flags: needinfo?(jkratzer)

Reporter

Comment 4

•

6 years ago

Attached file fuzzer.js (obsolete) — Details

Attachment #8987709 - Attachment is obsolete: true

Reporter

Comment 5

•

6 years ago

(In reply to Daniel Veditz [:dveditz] from comment #2)
> I don't see any WebGL in your testcase or the stack. How'd you end up
> picking the component?
> 
> I'm not sure where to put this. Most of the stack is CSS/Frames (Layout)
> which makes sense from your testcase. But I'm surprised to see anything in
> nsCSSFrameConstructor calling nsINode::doInsertChildAt. There is an
> appendChild early in your testcase (can you remove the lines after that, or
> are they necessary?) but it shouldn't have that stack. And there's no XBL at
> all in your testcase, though maybe that's just a thing we check the presence
> of and the node itself is corrupt.
> 
> I'm going to call this "Layout" on the preponderance of the stack, but I'm
> not sure. What happens in an ASAN build?
> 
> the null deref doesn't look like a security bug, but that stack is crazy so
> let's figure that out before unhiding the bug.

Looks like I really screwed up submitting this one.  This was supposed to be filed under Layout however my template defaults to WebGL.  Also, the testcase I submitted was for another bug entirely.  I've updated both attachments.

Reporter

Comment 6

•

6 years ago

Attached file fuzzer.js — Details

Attachment #8988526 - Attachment is obsolete: true

Comment 7

•

6 years ago

This feels a little more like a DOM bug, though maybe the actual bug is in nsCSSFrameConstructor::CreateGeneratedContentItem(). It sounds like something isn't getting unmark gray called on it.

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

Component: Layout → XBL

Assignee

Comment 8

•

6 years ago

This needs the fuzzing extension installed to reproduce, right?  Where's the current place one can get that?

Flags: needinfo?(jkratzer)

Assignee

Comment 9

•

6 years ago

(Ideally the steps to reproduce on fuzzing bugs would just include that information...)

Comment 10

•

6 years ago

There are no line numbers here, but doInsertChildAt calls ReparentWrappersInSubtree which calls GetXBLScope, so maybe that's how we end up there. It looks like ReparentWrappersInSubtree was recently added in bug 1466991.

Updated

•

6 years ago

Attachment #8988527 - Attachment mime type: application/javascript → text/plain

Comment 11

•

6 years ago

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #8)
> This needs the fuzzing extension installed to reproduce, right?  Where's the
> current place one can get that?

It looks like you don't actually need the extension. Instead, you'd need a --enable-fuzzing build with the pref fuzzing.enabled set to true, so that FuzzingFunctions is defined.

Assignee

Comment 12

•

6 years ago

Ah.  Yeah, definitely needs a steps to reproduce section...

Assignee

Updated

•

6 years ago

Assignee: nobody → bzbarsky

Reporter

Comment 13

•

6 years ago

Attached file prefs.js — Details

Flags: needinfo?(jkratzer)

Reporter

Comment 14

•

6 years ago

(In reply to Andrew McCreight [:mccr8] from comment #11)
> (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from
> comment #8)
> > This needs the fuzzing extension installed to reproduce, right?  Where's the
> > current place one can get that?
> 
> It looks like you don't actually need the extension. Instead, you'd need a
> --enable-fuzzing build with the pref fuzzing.enabled set to true, so that
> FuzzingFunctions is defined.

That's correct.  You can retrieve an ASAN fuzzing build using the fuzzfetch python module:
pip install fuzzfetch
python -m fuzzfetch --fuzzing -a

You can reproduce the testcase using ffpuppet found here:
https://github.com/MozillaSecurity/ffpuppet

With the following commands in the testcase directory:
python -m SimpleHTTPServer &
python -m ffpuppet -p prefs.js ~/build_path/firefox --xvfb -d -l log -u http://localhost:8000/trigger.html

Assignee

Comment 15

•

6 years ago

> You can retrieve an ASAN fuzzing build using the fuzzfetch python module

Will that give me full source so I can actually debug?

I tried doing an --enable-fuzzing debug build.  It fails to link, with error messages like this:

0:06.22 ../build/libjs_static.a(BinSource.o):../../../mozilla/js/src/frontend/BinSource.cpp:function sancov.module_ctor: error: undefined reference to '__sanitizer_cov_trace_pc_guard_init'
 0:06.36 ../build/libjs_static.a(BinSource.o):../../../mozilla/js/src/frontend/BinSource.cpp:function sancov.module_ctor: error: undefined reference to '__sanitizer_cov_trace_pc_guard_init'
 0:08.94 ../build/libjs_static.a(StructuredClone.o):../../../mozilla/js/src/vm/StructuredClone.cpp:function sancov.module_ctor: error: undefined reference to '__sanitizer_cov_trace_pc_guard_init'

Next step is to hack things so I just build FuzzingFunctions even if FUZZING is not defined...

Assignee

Comment 16

•

6 years ago

> Will that give me full source so I can actually debug?

And more importantly, how do I then test the fix?  ;)

Reporter

Comment 17

•

6 years ago

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #16)
> > Will that give me full source so I can actually debug?
> 
> And more importantly, how do I then test the fix?  ;)

No, unfortuantely not.  It's just a wrapper for pulling down taskcluster builds.  Also, I just realized the fuzzfetch args I gave you were incorrect.  To get a debug build you want to pass:
python -m fuzzfetch --fuzzing -d

If getting a fuzzing build is blocking you, you could try the fuzzpriv extension here:
https://github.com/MozillaSecurity/fuzzpriv

Then just replace the call to FuzzingFunctions.garbageCollect() with fuzzPriv.GC().  You can then use ffpuppet and the prefs.js included here to test:

python -m ffpuppet -p prefs.js -e ~/fuzzpriv/ ~/build_path/firefox --xvfb -d -l log -u http://localhost:8000/trigger.html

Assignee

Comment 18

•

6 years ago

OK, hacking things so that FuzzingFunctions is unconditionally exposed in a local build makes it pretty easy to reproduce. The sequence of events here is as follows:

1) Page is loaded.
2) New pageload starts, first page becomes zombie. It's still being shown, but is
not the current page anymore.
3) Script in the new page runs, triggers GC. The old Window is no longer directly
referenced from JS, so it gets marked gray (the C++ still owns it).
4) The animation on the zombie page changes overflow, triggering frame construction.
This creates some generated content for the "optgroup:before { content: attr(label); }"
bit in forms.css.
5) The generated content is added to the tree. The actual ::before element is set
as a native anonymous root before being added. But the textnode inside is not
thus marked, so it takes the new codepath added in bug 1466991.
6) That codepath always gets the XBL scope. I think I needed it at some point, then
figured out I actually don't but forgot to remove that line.
7) xpc::GetXBLScope does JS::AutoRealm on the passed-in global. This asserts it's not gray.
8) nsGlobalWindowInner::GetGlobalJSObject called FastGetGlobalJSObject which does not unmark gray.

Anyway, I think the right fix here is to get rid of the "get the xbl scope" bit, since it's not actually doing anything useful, and to pre-mark the anonymous content in the frame constructor before appending it, so we don't even enter this codepath for that stuff.

Assignee

Comment 19

•

6 years ago

I don't think there's a security issue here, btw.  We don't use the object that's being asserted not-gray, so there's no problem in practice in an opt build.

Blocks: 1466991

Keywords: sec-audit

Assignee

Comment 20

•

6 years ago

Attached patch Don't do extra work we don't need to in ReparentWrappersInSubtree — Details — Splinter Review

Attachment #8988626 - Flags: review?(bugs)

Olli Pettay [:smaug][bugs@pettay.fi]

Assignee

Comment 21

•

6 years ago

That said, the:

> Testcase bisects back further than a year

part is confusing to me, because the code in question is definitely new as of bug 1466991.  What does a failure look like on a build from before then?

Flags: needinfo?(jkratzer)

Comment 22

•

6 years ago

Comment on attachment 8988626 [details] [diff] [review]
Don't do extra work we don't need to in ReparentWrappersInSubtree

ops, I should have noticed the extra rootedGlobal thing in the other bug.

Attachment #8988626 - Flags: review?(bugs) → review+

Assignee

Comment 23

•

6 years ago

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

Flags: needinfo?(jkratzer)

Assignee

Comment 24

•

6 years ago

Er, comment 21 is still relevant.

Flags: needinfo?(jkratzer)

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

Assignee

Comment 25

•

6 years ago

Comment on attachment 8988626 [details] [diff] [review]
Don't do extra work we don't need to in ReparentWrappersInSubtree

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1466991
[User impact if declined]: Probably none; this is basically a debug-only
   problem, apart from the small perf hit.
[Is this code covered by automated tests?]:  Sort of.
[Has the fix been verified in Nightly?]: Yes.
[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?]: No.
[Why is the change risky/not risky?]: Just removes some unnecessary code.
[String changes made/needed]: None.

Attachment #8988626 - Flags: approval-mozilla-beta?

Comment 26

•

6 years ago

https://hg.mozilla.org/mozilla-central/rev/2cb8f2ddbf0e

Group: dom-core-security → core-security-release

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox63: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla63

Comment 27

•

6 years ago

Bug 1466991 is going to be landing on ESR60 during this cycle as well, so please nominate it for backport there also.

status-firefox61: --- → unaffected

status-firefox62: --- → affected

status-firefox-esr52: --- → unaffected

status-firefox-esr60: --- → affected

tracking-firefox62: --- → +

tracking-firefox63: --- → +

tracking-firefox-esr60: --- → ?

Comment 28

•

6 years ago

Comment on attachment 8988626 [details] [diff] [review]
Don't do extra work we don't need to in ReparentWrappersInSubtree

Approved for 62.0b5.

Attachment #8988626 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Reporter

Comment 29

•

6 years ago

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #24)
> Er, comment 21 is still relevant.

So it looks like comment 1 was for another bug entirely.  This testcase bisects back to the following range which includes bug 1466991:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dc997a4e045ec9c83288466a013a029cc475603c&tochange=0b5495dc100dd3bfda0886a4ad563a3c729c9b72

Sorry for all the confusion.

Flags: needinfo?(jkratzer)

Comment 30

•

6 years ago

uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/30c32c21df57

status-firefox62: affected → fixed

Assignee

Comment 31

•

6 years ago

> This testcase bisects back to the following range 

Perfect, now the world makes sense.  ;)

Assignee

Comment 32

•

6 years ago

Comment on attachment 8988626 [details] [diff] [review]
Don't do extra work we don't need to in ReparentWrappersInSubtree

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  Regression caused by bug 1466991.
User impact if declined: Fuzzing on ES a bit more complicated due to this
  assertion failure.
Fix Landed on Version: 61 and 62.
Risk to taking this patch (and alternatives if risky): Very low risk; just
  removes some code that's basically dead from the fix for bug 1466991.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Attachment #8988626 - Flags: approval-mozilla-esr60?

Sylvestre Ledru [:Sylvestre]

Updated

•

6 years ago

tracking-firefox-esr60: ? → 62+

Updated

•

6 years ago

Keywords: regression

Andrei Vaida [:avaida]

Updated

•

6 years ago

Flags: qe-verify-

Whiteboard: [post-critsmash-triage]

Comment 33

•

6 years ago

Comment on attachment 8988626 [details] [diff] [review]
Don't do extra work we don't need to in ReparentWrappersInSubtree

Approved for ESR 60.2.

Attachment #8988626 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+