Closed Bug 1389974 Opened 2 years ago Closed 2 years ago

Static analysis missing a rooting hazard in NPAPI code?

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 56+ fixed
firefox55 --- wontfix
firefox56 + fixed
firefox57 + fixed

People

(Reporter: jandem, Assigned: sfink)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main56+][adv-esr52.4+])

Attachments

(9 files)

I've been hacking NPAPI code and I noticed something suspicious in CreateNPObjectMember:

--

static bool
CreateNPObjectMember(NPP npp, JSContext *cx, JSObject *obj, NPObject* npobj,
                     JS::Handle<jsid> id,  NPVariant* getPropertyResult,
                     JS::MutableHandle<JS::Value> vp)
{
    ...
    JSObject *memobj = ::JS_NewObject(cx, &sNPObjectMemberClass);
    ...
    obj = GetNPObjectWrapper(cx, obj);    
    ...

JS_NewObject can definitely GC. Then why is it okay to use the unrooted |obj| afterwards?

Maybe I'm missing something, but obj is not assigned to between these two lines, and this is not in a suppress-GC scope AFAICS...
Flags: needinfo?(sphink)
Note that mrgiggles says CreateNPObjectMember can GC because of

  FieldCall: NPClass.getProperty

Likely overzealous, but that getProperty call is also before the GetNPObjectWrapper call, so it's not just the JS_NewObject.
I am looking into this. You are definitely right that this should be reported as a hazard. It isn't the obvious things -- that code not getting compiled, or it getting compiled multiple times in different compilation units with different bodies, or something like that. And the CFG looks like a very straightforward hazard to my eye. I'll step through in the debugger.
Yowza. This turned out to be pretty bad. The problem is that the analysis does not want to report a hazard for something like

    JSObject* obj = NewObject();
    cangc();
    obj = NewObject();

because although that last statement mentions obj, it doesn't make use of its previous value. So obj is not live across (during) cangc(). But in the control flow graph, this looks very similar to

    JSObject* obj = NewObject();
    cangc();
    obj->foo = NewObject();

and so the analysis naively assumes that it *is* using the previous value, but adds in an extra "edgeKillsVariable" check that knows how to distinguish the two (it can tell you're completely overwriting obj here.) Except this goes badly wrong in this case, which is roughly

    JSObject* obj = NewObject();
    cangc();
    obj = CopyObject(obj);

This *does* kill obj, but it also uses its previous value first, and the analysis incorrectly thinks that because obj is being overwritten then it isn't live upon entry. I need to make the analysis smarter, but I haven't decided whether to handle

    JSObject* obj = NewObject();
    obj = CopyObject(obj);

Assuming that CopyObject() can GC, this should not report a hazard -- obj's initial value is live from just after NewObject() up to the call to CopyObject(), dead during CopyObject(), and then a new live range begins after it. (There may still be a rooting hazard if CopyObject keeps its parameter live across a GC call, but that hazard will already be reported within CopyObject.) It may be easier just to treat obj as live throughout this edge, though; the analysis mostly doesn't distinguish between live ranges of variables vs values, and I don't know how common relevant constructs are in the code.
Interesting analysis. I wonder if fixing this will reveal similar hazards elsewhere :) At least this one requires Flash to be enabled and CreateNPObjectMember is a bit of a special case (when a plugin object has both a property and method with the same name). Still bad, of course.
Fixing this showed a handful of new hazards, but it wasn't too bad. I'll post the fixes first. It's sort of overloading this bug for other purposes, but I don't think that's a big deal.

A hazard is reported where on one iteration through the loop over the arguments, obj is set to a JSObject*. Then on the next iteration, the analysis incorrectly things that the if test could skip overwriting obj, and instead call as() on the previous iteration's value. This turns out to be impossible because reaching as() requires the if condition to be false, which means that both branches of the or are evaluated, and the second branch assigns to obj.

It seems like the call to as() should be dominated by the obj assignment, but in practice the compiler assigns to a temporary boolean variable and then tests it, so in terms of control flow only the as() call is *not* dominated by the obj assignment.

This patch just roots obj.
Attachment #8903640 - Flags: review?(jcoppeard)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Blocks: 1143786
Flags: needinfo?(sphink)
This is totally unrelated but I noticed it because when running the analysis without filtering out just the hazards, this is responsible for a *ridiculous* number of "taking the address of an unrooted value" warnings in my test run. I don't actually understand why I get so many duplicates of the same warning. And this does not show up at all in the official analysis runs, which is odd. But it's trivial to fix so I'm going to sneak it in here. (And it's a little weird to do const Value& elem = someValue(); you're creating a reference to a temporary. I guess if it were a huge struct, you'd be avoiding a copy.)
Attachment #8903643 - Flags: review?(jcoppeard)
Fix the hazard originally reported, plus another that was uncovered by the analysis fixes.
Attachment #8903646 - Flags: review?(nfroyd)
Attachment #8903640 - Flags: review?(jcoppeard) → review+
Attachment #8903643 - Flags: review?(jcoppeard) → review+
Comment on attachment 8903646 [details] [diff] [review]
Rooting fixes for nsJSNPRuntime.cpp

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

rs=me based on my understanding of how rooting is supposed to work.
Attachment #8903646 - Flags: review?(nfroyd) → review+
Almost positive this bug is present everywhere.
I could annotate all of nsIScriptSecurityManager, and in fact start switching over to annotating a list of builtinclass interfaces (in advance of doing it properly with in-source attributes). But for now, I'll follow the narrowly scoped style of the other annotations here.

This fixes a false positive in the gecko profiler, in a place where we don't have anything to root with. It was revealed by the analysis fix in this bug.
Attachment #8903655 - Flags: review?(jcoppeard)
Attachment #8903655 - Flags: review?(jcoppeard) → review+
The tests are currently broken because I added in an additional annotation that ffi_call can GC, but I did it in a way that requires ffi_call to be seen. And the tests don't do anything with it.
Attachment #8903658 - Flags: review?(jcoppeard)
Attachment #8903658 - Flags: review?(jcoppeard) → review+
And now the actual fix. The core of it is that

  obj = foo(obj);

no longer truncates (well, begins, for a forward direction) the live range of obj. It is now precise -- it's ok if foo() GCs because obj's value is not live across it, but the statement ends one live range and begins another.

This required fixing up some other things, which then required fixing more stuff to avoid missing hazards. And while writing tests I came up with at least one other related thing that seemed important to fix. I can't remember the sequence or dependency chain, but some of the changes:

  - using the fact that constructors are the beginning of a live range
  - marking loop starts as preceded by their ends, to find cross-iteration hazards
  - handling the gen() and kill() of |obj = foo(obj)| properly so that it is not a hazard if foo can gc, but the live ranges before and after are considered
Attachment #8903730 - Flags: review?(bhackett1024)
Track 56+ as sec-high.
Attachment #8903730 - Flags: review?(bhackett1024) → review+
Attached patch Rollup patchSplinter Review
[Security approval request comment]
How easily could an exploit be constructed based on the patch?

difficult

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

it's a rooting hazard, which if you're familiar with gives a straightforward way to at least try to run into the problem (a UAF-ish thing). But this is in a pretty difficult area to make use of.

Which older supported branches are affected by this flaw?

all

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

generational GC? This is old.

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

I doubt this code has changed much. Some of the hazard analysis changes (which do not fix any problems) might not backport cleanly, eg the ffi_call one that wouldn't be needed until fairly recently.

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

unlikely
Attachment #8906396 - Flags: sec-approval?
Attachment #8906396 - Flags: review+
sec-approval+ for trunk.
We'll want Beta and ESR52 patches made and nominated ASAP.
Attachment #8906396 - Flags: sec-approval? → sec-approval+
Attached patch rollup for betaSplinter Review
Approval Request Comment
[Feature/Bug causing the regression]: old bug
[User impact if declined]: theoretical but unlikely crashes, exploits
[Is this code covered by automated tests?]: included in patch
[Has the fix been verified in Nightly?]: landed recently
[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?]: minor rooting changes
[String changes made/needed]: none

try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fd45d84b34ccf93235bddf0f1f8e89e33d8c03e
Attachment #8906721 - Flags: approval-mozilla-beta?
Comment on attachment 8906721 [details] [diff] [review]
rollup for beta

Sec-high issue, let's uplift this fix for beta 12.
Attachment #8906721 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8906723 [details] [diff] [review]
rollup for esr52

uaf fix, sec-high, esr52.4+
Attachment #8906723 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Group: javascript-core-security → core-security-release
(In reply to Steve Fink [:sfink] [:s:] from comment #16)
> [Needs manual test from QE? If yes, steps to reproduce]: no
Per Steve's assessment on manual testing needs, marking this issue as qe-verify-.
Flags: qe-verify-
Whiteboard: [adv-main56+][adv-esr52.4+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.