Last Comment Bug 452786 - Crash due to insufficient class checking in Date class
: Crash due to insufficient class checking in Date class
Status: VERIFIED FIXED
[sg:critical?] post 1.8 branch
: regression, verified1.9.0.4
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
http://people.freenet.de/kuebart/test...
Depends on:
Blocks: 340992
  Show dependency treegraph
 
Reported: 2008-08-29 07:34 PDT by Joachim Kuebart
Modified: 2010-08-27 01:45 PDT (History)
9 users (show)
dveditz: blocking1.9.0.4+
dveditz: wanted1.8.1.x-
asac: wanted1.8.0.x-
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1 (4.29 KB, patch)
2008-08-29 08:01 PDT, Igor Bukanov
mrbkap: review+
dveditz: approval1.9.0.4+
Details | Diff | Review
testcase crashes trunk (260 bytes, text/html)
2008-08-29 11:11 PDT, Brandon Sterne (:bsterne)
no flags Details
fix for 1.9.0 (4.54 KB, patch)
2008-10-27 10:29 PDT, Igor Bukanov
igor: review+
Details | Diff | Review

Description Joachim Kuebart 2008-08-29 07:34:11 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: 

This bug crashes the browser without any possibility for user intervention. This is exploitable in the browser. Do *not* click the given link if you want to keep anything in your current Firefox session.


Reproducible: Always

Steps to Reproduce:

  (new Date()).getMonth.call(new Function())


Actual Results:  
jsshell (or Firefox) crash.

Expected Results:  
Exception.


The built-in Date class doesn't check the class of its "this" object sufficiently. Affected methods are getYear(), getMonth(), getDate() and so on.

This affects all of Date's methods that internally call GetLocalTime() (in jsdate.c) which doesn't check the class of object passed in (compare with JS_InstanceOf() call in GetUTCTime()). The object passed in needs to have at least 2 private slots, like

 a) the global object
 b) Function objects
 c) Iterator objects

The value from the private slot is passed to JSVAL_TO_DOUBLE() without further checks in GetLocalTime(), leading to bogus pointer dereference for objects not of class Date.

In addition to adding this to the test suite, this sort of thing could be added to jsparsefuzz.js (i.e. the fuzzer should be modified to use <method>.call() with random arguments if it's not already doing this).

I don't have time right now to check out a current tree and post a patch and will not get around to it until some time next week.
Comment 1 Igor Bukanov 2008-08-29 08:01:33 PDT
Created attachment 336054 [details] [diff] [review]
fix v1

The essence of the fix is the addition of the missing instanceof check for GetAndCacheLocalTime. I also took an opportunity in the patch to replace heavy JS_(Get|Set)ReservvedSlot calls with direct slot access for simpler code.
Comment 2 Joachim Kuebart 2008-08-29 08:34:33 PDT
Igor's patch should fix the issue AFAICS. If needed I can report back next week when I get a chance to build trunk and verify.
Comment 3 Brandon Sterne (:bsterne) 2008-08-29 11:11:54 PDT
Created attachment 336077 [details]
testcase crashes trunk

Preserving a copy of the testcase in Bugzilla.
Comment 4 Brendan Eich [:brendan] 2008-08-29 12:36:42 PDT
Much nicer -- thanks, Igor.

/be
Comment 5 Igor Bukanov 2008-09-01 02:07:51 PDT
landed on trunk - http://hg.mozilla.org/mozilla-central/rev/42b53b1f04e6
Comment 6 Igor Bukanov 2008-09-01 02:30:29 PDT
Comment on attachment 336054 [details] [diff] [review]
fix v1

The patch applies to 1.9.0 branch as-is.
Comment 7 Igor Bukanov 2008-09-01 02:47:30 PDT
The bug only exists on 1.9.0 and later and is an regression from the bug 340992.
Comment 8 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-09-01 14:44:19 PDT
Is it worth doing a version of the patch that only has the fix and not the cleanup, for risk reduction on 1.9.0.x?
Comment 9 Brendan Eich [:brendan] 2008-09-01 15:57:47 PDT
(In reply to comment #8)
> Is it worth doing a version of the patch that only has the fix and not the
> cleanup, for risk reduction on 1.9.0.x?

There's no risk reduction in messing with a reviewed patch that cuts out overhead and doubt about reserved slot complexities that do not affect fslots' dimension or lifetime. Making more variation between trees adds tiny risk too, probably more than any risk in sticking this patch in as-is. But it's too tiny to estimate or measure, or to be worth arguing much about.

/be
Comment 10 Daniel Veditz [:dveditz] 2008-09-05 11:19:30 PDT
1.9.0.2 is in release testing, this will have to sail with the next ship.
Comment 11 Daniel Veditz [:dveditz] 2008-09-10 15:07:41 PDT
Comment on attachment 336054 [details] [diff] [review]
fix v1

Approved for 1.9.0.3, a=dveditz for release-drivers
Comment 12 Igor Bukanov 2008-10-27 10:29:21 PDT
Created attachment 344937 [details] [diff] [review]
fix for 1.9.0

It turned out the patch required a trivial change for 1.9.0. It applies as-is, but the trunk version uses const to define constant names. But that does not work with in C sources as in C such constants are not real constant expressions. So I replaced constants with the preprocessor macros. 

Here is the plain diff between the trunk and the branch patches:

diff  /home/igor/s/fix_date.patch /home/igor/s/fix_date.1.9.0.patch
7c7
< +++ jsdate.c	27 Oct 2008 17:22:47 -0000
---
> +++ jsdate.c	27 Oct 2008 17:21:59 -0000
19,20c19,20
< +const uint32 JSSLOT_UTC_TIME    = JSSLOT_PRIVATE;
< +const uint32 JSSLOT_LOCAL_TIME  = JSSLOT_PRIVATE + 1;
---
> +#define JSSLOT_UTC_TIME         JSSLOT_PRIVATE
> +#define JSSLOT_LOCAL_TIME       (JSSLOT_PRIVATE + 1)
22c22
< +const uint32 DATE_RESERVED_SLOTS = 2;
---
> +#define DATE_RESERVED_SLOTS     2
Comment 13 Igor Bukanov 2008-10-27 10:31:51 PDT
I landed the patch from the comment 12 on 1.9.0 branch:

Checking in jsdate.c;
/cvsroot/mozilla/js/src/jsdate.c,v  <--  jsdate.c
new revision: 3.112; previous revision: 3.111
done
Comment 14 Al Billings [:abillings] 2008-10-28 15:38:39 PDT
We now get the exception "TypeError on line 1: Date.prototype.getMonth called on incompatible Function"

Verified for 1.9.0.4 with  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4pre) Gecko/2008102804 GranParadiso/3.0.4pre.

Same behavior in Trunk as well with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081028 Minefield/3.1b2pre.
Comment 15 Bob Clary [:bc:] 2008-11-27 03:42:05 PST
Checking in ecma_3/Date/regress-452786.js;
/cvsroot/mozilla/js/tests/ecma_3/Date/regress-452786.js,v  <--  regress-452786.js
initial revision: 1.1
done
Comment 16 Shawn Wilsher :sdwilsh 2008-11-27 11:11:55 PST
http://hg.mozilla.org/mozilla-central/rev/39f2f17c31ee

Note You need to log in before you can comment on or make changes to this bug.