820862 - JSJitInfo needs to say what sort of function it is

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

JSJitInfo has a JSJitPropertyOp op in it, which has this signature:

typedef bool
(* JSJitPropertyOp)(JSContext *cx, JSHandleObject thisObj,
                    void *specializedThis, JS::Value *vp);

But for methods we reinterpret it as a JSJitMethodOp, which has this signature:

typedef bool
(* JSJitMethodOp)(JSContext *cx, JSHandleObject thisObj,
                  void *specializedThis, unsigned argc, JS::Value *vp);

The attached testcase causes us to erroneously treat a JSJitPropertyOp as a JSJitMethodOp, and in particular allows the caller to control the value of the "vp" pointer passed to the property op.  Just so you can avoid loading it and crashing, it looks like this:

    var xhr = new XMLHttpRequest();
    var desc = Object.getOwnPropertyDescriptor(xhr.__proto__, "readyState");
    xhr.__proto__.foopy = desc.get;
    for (var i = 0; i < 1000000; ++i)
      xhr.foopy(1,2,3,4,5,6,7,8);

This invokes get_readyState with the fourth argument set to 8.

Marking security-sensitive, since this seems to allow an attacker to write near-arbitrary values (by picking your getter properly) to near-arbitrary memory locations.

Furthermore, even though the signature for getters and setters is compatible the meaning of "vp" is not.  For a getter it's a pointer to uninitialized stack memory while for a setter it's a pointer to the value being set.  So we should really distinguish between the two kinds of JSJitPropertyOp too; otherwise it might be possible to read data off the stack, which is bad.  It would probably also be possible to trigger writes to stuff that shouldn't be written to (by treating a setter as a getter).

Anyway, patch coming up.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Boris Zbarsky (:bz) from comment #0)

> Marking security-sensitive, since this seems to allow an attacker to write
> near-arbitrary values (by picking your getter properly) to near-arbitrary
> memory locations.

sec-critical?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: JSJitInfo should say what sort of function it is.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> sec-critical?

I believe so, yes.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and this is ion-only, so esr17 not affected.  But 18 and up are affected.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 691373 [details] [diff] [review]
JSJitInfo should say what sort of function it is.

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

Great catch.

::: js/src/ion/IonBuilder.cpp
@@ +6303,5 @@
>      if (singleton->isFunction()) {
>          RootedFunction singletonFunc(cx, singleton->toFunction());
>          if (TestAreKnownDOMTypes(cx, unaryTypes.inTypes) &&
> +            TestShouldDOMCall(cx, unaryTypes.inTypes, singletonFunc,
> +                              JSJitInfo::eMethod))

Nit: fits on one line (SpiderMonkey limit is 99 characters for code, 80 for comments).

@@ +6370,5 @@
>      MDefinition *obj = current->pop();
>      RootedFunction getter(cx, commonGetter);
>  
> +    if (isDOM && TestShouldDOMCall(cx, unaryTypes.inTypes, getter,
> +                                   JSJitInfo::eGetter)) {

Same here.

::: js/src/jsfriendapi.h
@@ +1424,5 @@
>                    void *specializedThis, unsigned argc, JS::Value *vp);
>  
>  struct JSJitInfo {
> +    enum OpType {
> +        eGetter,

Nit: SpiderMonkey style is either GETTER or Getter (and it's what other enums in this file seem to use).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to review comments

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 691417 [details] [diff] [review]
Updated to review comments

[Security approval request comment]
How easily can the security issue be deduced from the patch?  Probably fairly
   easily for anyone familiar with ECMAScript.  They'd have to find the cases
   we use this functionality in, but other than that...

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

Which older supported branches are affected by this flaw? 18, 19.  No shipping
   browsers yet.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  I have backports for Aurora and Beta.  Will
   attach them in a sec.

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Aurora 19 patch

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Beta 18 patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 691426 [details] [diff] [review]
Aurora 19 patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 747288
User impact if declined: Security problem.
Testing completed (on m-c, etc.): Passes attached testcase, local performance
   tests.
Risk to taking this patch (and alternatives if risky): Extremely low.
String or UUID changes made by this patch: None.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 691417 [details] [diff] [review]
Updated to review comments

sec-approval+. Let's get this in for 18 and 19 if possible to avoid shipping this issue.

Comment 13

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/55b2d38ac36c
https://hg.mozilla.org/releases/mozilla-aurora/rev/e3819aecff29
https://hg.mozilla.org/releases/mozilla-aurora/rev/e3819aecff29

Comment 14

[Boris Zbarsky [:bzbarsky]]

Note: I did NOT check in the test.  Should I?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Pretty sure you meant to post a beta link there.
https://hg.mozilla.org/releases/mozilla-beta/rev/865b57171950

Also, *please* don't land things on aurora/beta until they're safely on m-c. (I type this as I see that your push got backed out of inbound).

Comment 16

[Boris Zbarsky [:bzbarsky]]

This didn't get backed out of inbound, and it would have been _very_ odd for it to have failed there.

On the other hand, we do want to get this on branches ASAP so we get it out to users....

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Yes, I'm aware that it was the other patch in your push that got backed out. But it did illustrate the point that landing on inbound and across branches can potentially lead to messy backouts. With the exception of nightly testers, getting this on Aurora/Beta one day earlier isn't going to make a significant difference as to when users get it.

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/55b2d38ac36c

(I'm with Ryan on this one. Please make sure the inbound/m-c run is green before pushing to branches, just in case.)

Comment 19

[Al Billings [:abillings - ex-MoCo]]

(In reply to Boris Zbarsky (:bz) from comment #14)
> Note: I did NOT check in the test.  Should I?

Only if it makes it onto 18 and 19, otherwise, we expose those releases.

Comment 20

[Boris Zbarsky [:bzbarsky]]

This is already landed on 18 and 19.  Should I wait for the next beta or something, or just land the test everywhere?

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Release management, do you see a reason not to land the test right now?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/865b57171950