696489 - Crash with __proto__, GC, InstallTrigger

Status: RESOLVED DUPLICATE of bug 719841

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires extension for GC; crashes when loaded)

1. Install https://www.squarefree.com/extensions/domFuzzLite2.xpi
2. Load the testcase

Crash [@ js::IsWrapper] with thousands of xpc::WrapperFactory::WaiveXray on the stack.

Curiously, this is a null deref crash, _not_ a "hit the guard page at the end of the stack" crash.  Maybe we hit a JS_CHECK_RECURSION, causing something to return null to a caller that wasn't expecting null?

There may be two bugs here: the recursion and the null deref.

Security-sensitive for now because this involves our old friends GC, InstallTrigger, and wrappers.

My DOM fuzzer found this bug by expanding on the testcase in bug 417852 (from a safe distance). That's not quite what I designed the fuzzer to do, but it found a way to do it anyway. Good job, fuzzer.

Comment 1

[Jesse Ruderman]

Attachment: stack trace from gdb

Breakpad is kinda useless here: bp-22090715-06c9-4eda-a353-852da2111021

Comment 2

[Johnny Stenback (:jst)]

Unclear whether this is a sg bug or not. Bobby, care to dig in?

Comment 3

[Bobby Holley (:bholley)]

This seems pretty bad. Basically, the code that detects cyclic prototype chains doesn't properly handle inner/outer objects. So the testcase does the following:

1 - Null out window.__proto__, to make it seem like a dead-end.

2 - Set Object.prototype.__proto__ = window, which passes the cycle check because window appears to be a dead end.

3 - Trigger a GC, which is necessary to clear various cached waivers.

4 - Do something (in this case, accessing InstallTrigger) that causes us to to call WaiveXray, which tries to recursively waive Xray on the prototype chain while outer-izing any objects it finds. Since we just GCed, we have to do this all from scratch.

5 - Marvel at the infinite recursion loop until we corrupt memory (or worse?) and the browser crashes.

FWIW, I'd imagine there are other ways we'd screw up with this kind of indirect cycle, not just waiving Xray. I think the fix probably needs to be in the js engine.

I'll dig in more tomorrow and see if I can figure out the proper fix.

Comment 4

[Jeff Walden [:Waldo]]

We haven't historically considered infinite recursion alone to be a vulnerability, or memory corruption as we've traditionally defined it.  Stack overflow just means you try to read to or write from the guard page at the end of the stack, the kernel says no and kills your process.  That's safe.  Or rather, if it's not, it's not our problem to solve, and it's fairly hard for us to definitively address it.

This said, usually we do try to fix infinite recursion scenarios when we find them.  But they're not high-priority security vulnerability fixes.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Jeff Walden (remove +bmo to email) from comment #4)
> We haven't historically considered infinite recursion alone to be a
> vulnerability, or memory corruption as we've traditionally defined it. 
> Stack overflow just means you try to read to or write from the guard page at
> the end of the stack, the kernel says no and kills your process.  That's
> safe.  Or rather, if it's not, it's not our problem to solve, and it's
> fairly hard for us to definitively address it.
> 
> This said, usually we do try to fix infinite recursion scenarios when we
> find them.  But they're not high-priority security vulnerability fixes.

Yeah, you're right. I was conflating various ideas while writing comment 3 Step 5 and wrote something that didn't make sense. :\

However, I've still got this vague feeling that a prototype chain loop can be used to do something nasty. I'm not sure exactly what though. If anyone has any ideas/experience about it, I'd be interested to hear.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Traverse inner->outer windows during proto cycle check. v1

Attaching a patch. Flagging jorendorff for review.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Here's a version of the testcase. I'm not sure if it gives away too much, or whether it doesn't matter. jorendorff, what do you think?

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 589306 [details] [diff] [review]
Tests. v1

Definitely OK in my opinion.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 589305 [details] [diff] [review]
Traverse inner->outer windows during proto cycle check. v1

As written, I think this patch fixes the test case at hand, but introduces a stack overflow of its own! See if you can trigger it.

I think only global objects are allowed to implement outerObject, so this can be written with a loop (no recursion needed). You can add an assertion to that effect here, if you like.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 589305 [details] [diff] [review]
Traverse inner->outer windows during proto cycle check. v1

(In reply to Jason Orendorff [:jorendorff] from comment #9)
> I think only global objects are allowed to implement outerObject, so this
> can be written with a loop (no recursion needed).

Wait-- this isn't true.

OK, so just add a JS_CHECK_RECURSION call in CycleCheck and we should be all set. Sorry for the noise.

r=me with that change.

Comment 11

[Bobby Holley (:bholley)]

jorendorff and I decided on IRC that bug 719841 is a better way to fix this issue. Hang tight while we do that.

Comment 12

[Bobby Holley (:bholley)]

jorendorff and I fixed this over in bug 719841. Resolving duplicate. \o/