562866 - StatementParams::NewResolve mishandles JSVAL_IS_STRING(aId)

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[timeless]

118 AsyncStatementParams::NewResolve(
133   PRUint32 idx;
135   if (JSVAL_IS_INT(aId)) {
140   else if (JSVAL_IS_STRING(aId)) {
141     JSString *str = JSVAL_TO_STRING(aId);
142     jschar *nameChars = ::JS_GetStringChars(str);
143     size_t nameLength = ::JS_GetStringLength(str);
144 
145     // We are unable to tell if there's a parameter with this name and so
146     // we must assume that there is.  This screws the rest of the prototype
147     // chain, but people really shouldn't be depending on this anyways.
148     *_retval = ::JS_DefineUCProperty(aCtx, aScopeObj, nameChars, nameLength,
149                                      JSVAL_VOID, nsnull, nsnull, 0);
150     NS_ENSURE_TRUE(*_retval, NS_OK);
151   }
152   else {
155   }
156 
157   *_retval = ::JS_DefineElement(aCtx, aScopeObj, idx, JSVAL_VOID, nsnull,
158                                 nsnull, 0);

Comment 1

[timeless]

Attachment: proposal

Comment 2

[timeless]

asuth notes that this code was copied from the sync version

Comment 3

[timeless]

Attachment: patch

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 442633 [details] [diff] [review]
patch

>+  *_objp = (*_retval = ok) ? aScopeObj : nsnull;
nit: do the assignment to _retval on a separate line please.

>+    if (NS_SUCCEEDED(rv)) {
>+      ok = ::JS_DefineUCProperty(aCtx, aScopeObj, nameChars, nameLength,
>+                                 JSVAL_VOID, nsnull, nsnull, 0);
>+      NS_ASSERTION(ok, "eep");
>+    } else {
>+      ok = PR_FALSE;
>     }
nit: storage style says to have the else on a new line (see rest of file)

>+  if ((*_retval = ok))
nit: assignment on separate line please

r=sdwilsh with those changes

Comment 5

[Damon Sicore (:damons)]

Can we get a security rating here?

Comment 6

[timeless]

Attachment: patch

Comment 7

[timeless]

If the local variable can have web content, then I think it's possible for it to be promoted as a chrome object and used by storage, which would be code execution as chrome.

I'm not actually sure whether it's possible for an object to end up in this slot in the stack, and this is likely to be compiler/optimizer dependent. The more likely behavior is that we'd crash.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 443452 [details] [diff] [review]
patch

>+    ok = ::JS_DefineUCProperty(aCtx, aScopeObj, nameChars, nameLength,
>+                               JSVAL_VOID, nsnull, nsnull, 0);
>   }
>   else {
>     // We do not handle other types.
>-    return NS_OK;
>+    ok = PR_FALSE;
>   }
> 
>-  *_retval = ::JS_DefineElement(aCtx, aScopeObj, idx, JSVAL_VOID, nsnull,
>-                                nsnull, 0);
>-  if (*_retval)
>-    *_objp = aScopeObj;
>+  *_retval = ok;
>+  *_objp = *_retval ? aScopeObj : nsnull;

Doesn't this return false without setting an exception?

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> Doesn't this return false without setting an exception?
Yes, I missed that (we need static analysis for this stuff).

Comment 10

[timeless]

that's correct, but it was already broken, and that's not as critical of a problem. it just leads to an uncatchable exception.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

This or something else in mozilla::storage::AsyncStatementParams::NewResolve
causes JS to assert and basically prevent me to start debug builds
on 64bit linux.

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 443452 [details] [diff] [review]
patch

>   else {
>     // We do not handle other types.
>-    return NS_OK;
>+    ok = PR_FALSE;
>   }
Except that we used to check the prototype chain after this if we didn't handle it.  Returning NS_OK here should still be fine, right?

Comment 13

[Boris Zbarsky [:bzbarsky]]

To be clear, the reason I nominated this is that his code ends up using a random value (uninitialized stack variable) as an index to define a property at.  It also incidentally completely forgets return values in the string case (possibly losing error conditions).

Basically, if anyone tries to call this function with a string arg, they'll just lose.  Doubly so on 64-bit, where the random value is that much more likely to overflow jsval's 31-bit restriction.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Thanks, bz - setting blocking to "needed", so as soon as we have a reviewed patch it'll get picked up by that release.

Comment 15

[Brandon Sterne (:bsterne)]

Boris, is this an sg:critical bug?

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

I don't think this is sg:critical. The only bad effects of passing an unintialized idx to JS_DefineElement is that a random property will appear on the object. I don't think this can lead to arbitrary code execution though.

Comment 18

[Matt Woodrow (:mattwoodrow)]

I get a load time assertion crash from the index being out of range. Seems to be reproducible every time on Ubuntu 9.04

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Yeah -- JS assertions are fatal, but in an optimized build where the assertion doesn't exist, the effect will simply be an arbitrary property being created.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Note, the patch here doesn't seem to fix the JS assertion I have.
Assertion failure: INT_FITS_IN_JSVAL(i), at /home/smaug/mozilla/mozilla_cvs/hg/src/js/src/jsapi.h:252

Comment 21

[Jason Orendorff [:jorendorff]]

Attachment: v2 [checked in]

This line also strikes me as wrong, but I didn't want to fix it without knowing the details:

     if (idx >= mParamCount)

It seems like it should also check for idx < 0.

Comment 23

[Shawn Wilsher :sdwilsh]

(In reply to comment #21)
> This line also strikes me as wrong, but I didn't want to fix it without knowing
> the details:
> 
>      if (idx >= mParamCount)
idx is a PRUint32, so it cannot be less than 0.

Comment 24

[Shawn Wilsher :sdwilsh]

Comment on attachment 443881 [details] [diff] [review]
v2 [checked in]

>diff --git a/storage/src/mozStorageAsyncStatementParams.cpp b/storage/src/mozStorageAsyncStatementParams.cpp
>+  PRBool ok = PR_TRUE;
>+  PRBool resolved = PR_FALSE;
nit: use bool for resolved

>diff --git a/storage/src/mozStorageStatementParams.cpp b/storage/src/mozStorageStatementParams.cpp
>+  PRBool resolved = PR_FALSE;
>+  PRBool ok = PR_TRUE;
ditto

We could write a test for this, right?  I'd like to make sure this doesn't happen in the future.  (I can probably write the test if need be).

r=sdwilsh

Comment 25

[Jason Orendorff [:jorendorff]]

(In reply to comment #23)
> (In reply to comment #21)
> > This line also strikes me as wrong, but I didn't want to fix it without knowing
> > the details:
> > 
> >      if (idx >= mParamCount)
> idx is a PRUint32, so it cannot be less than 0.

Ah, I see. The code is correct.

Comment 26

[Jason Orendorff [:jorendorff]]

Yes, this is testable. Obtain an object of either class, then do

  obj.name

to cause the resolve hook to be called with a string id. If that doesn't assert right away, the next step is to check the object for bogus elements that the resolve hook inadvertently added:

  for (var p in obj)             // Note that p is always a string.
      if (/^-?[0-9]+$/.exec(p))  // Any property that looks like a number
          FAIL;                  // indicates there was a bug.

Thank you for volunteering to write the tests. You'll be much faster at it than I would--I'm not at all familiar with this code.

Comment 27

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/mozilla-central/rev/47d79146b3c2

Should the fix go into mozilla-1.9.2 ahead of the tests, or should we wait for them?

Comment 28

[Shawn Wilsher :sdwilsh]

(In reply to comment #27)
> http://hg.mozilla.org/mozilla-central/rev/47d79146b3c2
> 
> Should the fix go into mozilla-1.9.2 ahead of the tests, or should we wait for
> them?
I'll attach the tests by the end of the day.  Let's wait for them.

Comment 30

[Shawn Wilsher :sdwilsh]

(In reply to comment #26)
> Yes, this is testable. Obtain an object of either class, then do
This should happen all the time, right?  I can't seem to get my test case to fail:
function test_params_get()
{
  let stmt = createStatement(
    "SELECT * FROM test WHERE id IN (:a, :b, :c)"
  );

  // Make sure we do not assert in getting the value.
  let expected = ["a", "b", "c"];
  for (let name in expected) {
    stmt.params[name];
  }

  // Now make sure we didn't magically get any additional properties.
  let count = 0;
  for (let name in stmt.params) {
    if (/^-?[0-9]+$/.exec(name))
      do_throw("unexpected!");
    count++;
  }
  do_check_eq(expected.length, count);
}

Note: stmt.params is the object that we've patched.  I don't have your changes applied.

Comment 31

[Shawn Wilsher :sdwilsh]

Eh, and now it's failing.  Rebuilding with the changes to make sure it doesn't fail afterwards.

Comment 32

[Shawn Wilsher :sdwilsh]

OK, fun failure here.  I've got your fix compiled in now, but somehow we're getting an additional property on the params object that I don't expect.  The code is:
function test_params_gets_sync()
{
  // Added for bug 562886.
  let stmt = createStatement(
    "SELECT * FROM test WHERE id IN (:a, :b, :c)"
  );

  // Make sure we do not assert in getting the value.
  let expected = ["a", "b", "c"];
  for each (let name in expected) {
    stmt.params[name];
  }

  // Now make sure we didn't magically get any additional properties.
  let count = 0;
  for (let name in stmt.params) {
    print(name);
    count++;
  }
  do_check_eq(expected.length, count);
}

The print output looks like this:
a
b
c
test

I don't understand how test is getting in there at all.  If I add print statements in the for each, I get:
a
b
c

Any ideas?

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

for-in iterates over the prototype chain. Presumably, you have a 'test' property left over from an earlier test (testing that you don't shadow prototype properties perhaps?) that gets in the way here.

Comment 34

[Shawn Wilsher :sdwilsh]

(In reply to comment #33)
> for-in iterates over the prototype chain. Presumably, you have a 'test'
> property left over from an earlier test (testing that you don't shadow
> prototype properties perhaps?) that gets in the way here.
duh.  Thanks, that was it.

Comment 35

[Shawn Wilsher :sdwilsh]

Attachment: test v1.0

Tests.  Waldo may hate me, but it makes sure we uncomment this code when the stuff is added.

Comment 36

[Shawn Wilsher :sdwilsh]

Also refactored how the js helper tests work (obviously).

Comment 37

[Jeff Walden [:Waldo]]

Are you referring to the getOwnPropertyNames stuff?

You should use Object.getPrototypeOf(obj) in preference to obj.__proto__.

Comment 38

[Shawn Wilsher :sdwilsh]

(In reply to comment #37)
> Are you referring to the getOwnPropertyNames stuff?
Yes.

> You should use Object.getPrototypeOf(obj) in preference to obj.__proto__.
Will update.

Comment 39

[Shawn Wilsher :sdwilsh]

Attachment: test v1.1

Comment 40

[Shawn Wilsher :sdwilsh]

Not sure when I'll get to land this.  If someone else can, please do so.

Comment 41

[Damon Sicore (:damons)]

JST, can you land this?

Comment 42

[Jason Orendorff [:jorendorff]]

Just to recap the story so far here, the *fix* landed in mozilla-central:
  http://hg.mozilla.org/mozilla-central/rev/47d79146b3c2

What needs to be done now is that Shawn's *tests* need to land; and then both need to go into mozilla-1.9.2.

Comment 43

[Shawn Wilsher :sdwilsh]

(In reply to comment #42)
> What needs to be done now is that Shawn's *tests* need to land; and then both
> need to go into mozilla-1.9.2.
Test isn't useful on 1.9.2, so I would not land it there.

Comment 44

[Blake Kaplan (:mrbkap) (inactive)]

Removing critical markings. The only misbehavior from this bug that I can see is that we'll randomly define numeric properties on StatementParams objects. I don't even think we'll crash in optimized builds (do note: in debug builds, there is a possible (fatal) JS assert to hit if the uninitialized).

Comment 45

[Johnny Stenback (:jst)]

Tests checked in.

http://hg.mozilla.org/mozilla-central/rev/6cf2f1c48f5f

Shawn, anything else left to do here, or should this be marked fixed now?

Comment 46

[Shawn Wilsher :sdwilsh]

This is fixed.

Comment 47

[Daniel Veditz [:dveditz]]

1.9.1 is affected as well, but will need a merged patch because of comment/context differences (and because the Async version doesn't exist).

Guessing "sg:low" if the worst effect is creating random properties from uninitialized memory that might leak data. But is there any chance that property will be executable, such as a function?

Comment 48

[Blake Kaplan (:mrbkap) (inactive)]

No, the *contents* of the property are always going to be void (undefined in JS lang parlance). I suppose that one might be able to construct 4-byte snippets of random stack memory by finding the incorrectly-defined property's *name*, constituting a data leak, but that seems pretty far fetched to me.

Comment 49

[LegNeato]

Comment on attachment 443881 [details] [diff] [review]
v2 [checked in]

I'm going to deny this for 1.9.2.5 temporarily. Once we get a patch for 3.5.x please renominate and we'll approve both (assuming the 3.5 patch looks safe).