Closed Bug 562866 Opened 10 years ago Closed 10 years ago

StatementParams::NewResolve mishandles JSVAL_IS_STRING(aId)

Categories

(Toolkit :: Storage, defect, blocker)

defect
Not set
blocker

Tracking

()

RESOLVED FIXED
Tracking Status
blocking1.9.2 --- -
status1.9.2 --- wanted
blocking1.9.1 --- -
status1.9.1 --- wanted

People

(Reporter: timeless, Assigned: jorendorff)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: coverity, Whiteboard: [sg:low])

Attachments

(2 files, 4 obsolete files)

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);
Attached patch proposal (obsolete) — Splinter Review
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #442628 - Flags: review?(bugmail)
asuth notes that this code was copied from the sync version
Summary: AsyncStatementParams::NewResolve mishandles JSVAL_IS_STRING(aId) → StatementParams::NewResolve mishandles JSVAL_IS_STRING(aId)
Attached patch patch (obsolete) — Splinter Review
Attachment #442628 - Attachment is obsolete: true
Attachment #442633 - Flags: review?(mrbkap)
Attachment #442628 - Flags: review?(bugmail)
Attachment #442633 - Flags: review?(mrbkap) → review+
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
Can we get a security rating here?
Attached patch patch (obsolete) — Splinter Review
Attachment #442633 - Attachment is obsolete: true
Attachment #443452 - Flags: review+
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.
Keywords: checkin-needed
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?
(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).
that's correct, but it was already broken, and that's not as critical of a problem. it just leads to an uncatchable exception.
This or something else in mozilla::storage::AsyncStatementParams::NewResolve
causes JS to assert and basically prevent me to start debug builds
on 64bit linux.
blocking1.9.2: --- → ?
blocking2.0: --- → ?
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?
Attachment #443452 - Flags: review+ → review-
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.
Thanks, bz - setting blocking to "needed", so as soon as we have a reviewed patch it'll get picked up by that release.
blocking1.9.2: ? → needed
Boris, is this an sg:critical bug?
Severity: major → blocker
Duplicate of this bug: 564090
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.
I get a load time assertion crash from the index being out of range. Seems to be reproducible every time on Ubuntu 9.04
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.
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
Attached patch v2 [checked in]Splinter Review
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.
Assignee: timeless → jorendorff
Attachment #443452 - Attachment is obsolete: true
Attachment #443881 - Flags: review?(sdwilsh)
Duplicate of this bug: 564184
(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 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
Attachment #443881 - Flags: review?(sdwilsh) → review+
(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.
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.
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?
(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.
Duplicate of this bug: 564267
(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.
Eh, and now it's failing.  Rebuilding with the changes to make sure it doesn't fail afterwards.
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?
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.
(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.
Attached patch test v1.0 (obsolete) — Splinter Review
Tests.  Waldo may hate me, but it makes sure we uncomment this code when the stuff is added.
Attachment #444125 - Flags: review?(jorendorff)
Attachment #444125 - Flags: review?(bugmail)
Also refactored how the js helper tests work (obviously).
Attachment #444125 - Flags: review?(jorendorff) → review+
Are you referring to the getOwnPropertyNames stuff?

You should use Object.getPrototypeOf(obj) in preference to obj.__proto__.
Attachment #444125 - Flags: review?(bugmail) → review+
Whiteboard: [sg:critical][critsmash:patch]
(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.
Attached patch test v1.1Splinter Review
Attachment #444125 - Attachment is obsolete: true
Attachment #443881 - Attachment description: v2 → v2 [checked in]
Not sure when I'll get to land this.  If someone else can, please do so.
Keywords: checkin-needed
JST, can you land this?
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.
(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.
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).
Whiteboard: [sg:critical][critsmash:patch]
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?
This is fixed.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #443881 - Flags: approval1.9.2.5?
blocking2.0: ? → ---
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?
Whiteboard: [sg:low]
blocking1.9.1: --- → ?
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.
blocking1.9.1: ? → -
blocking1.9.2: needed → -
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).
Attachment #443881 - Flags: approval1.9.2.5? → approval1.9.2.5-
Depends on: 576458
Group: core-security
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.