524121 - Date.setTime sets slots without checking for Date class

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

My changes for bug 412296 removed the check for Date class in the implementation of Date.prototype.setTime method. This allows to overwrite slots with jsNaN value in arbitrary object. The following test case demonstrates the problem:

~/m/tm/js/src> cat ~/s/x.js
var x = new Function('return 10');
Date.prototype.setTime.call(x);
print(x());
~/m/tm/js/src> ~/build/js32.tm.dbg/js ~/s/x.js
Assertion failure: STOBJ_GET_CLASS(obj) == &js_DateClass, at /home/igor/m/tm/js/src/jsdate.cpp:1227
Trace/breakpoint trap

Comment 1

[Igor Bukanov]

Attachment: fix v1

The fix is rather non-minimal but I wanted to ensure that I have not missed any more cases like this bug. The extra change removes double-boxing of time as jsdouble jsval in SetUTCTime callers and removal of date_constructor as the new SetUTCTime can serve its function just as well.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 408034 [details] [diff] [review]
fix v1

>@@ -1707,32 +1692,36 @@ date_makeDate(JSContext *cx, uintN maxar
>     uintN i;
>     jsdouble lorutime; /* local or UTC version of *date */
>     jsdouble args[3], *argp, *stop;
>     jsdouble year, month, day;
>     jsdouble result;
> 
>     obj = JS_THIS_OBJECT(cx, vp);
>     if (!GetUTCTime(cx, obj, vp, &result))
>-        return JS_FALSE;
>+        return false;
> 
>     /* see complaint about ECMA in date_MakeTime */
>-    if (argc == 0)
>-        return SetDateToNaN(cx, vp);
>+    if (argc == 0) {
>+        SetDateToNaN(cx, obj, NULL);

No need for trailing NULL.

>@@ -1813,46 +1798,46 @@ date_setYear(JSContext *cx, uintN argc, 
[snip]
>+    if (!JSDOUBLE_IS_FINITE(year)) {
>+        SetDateToNaN(cx, obj, vp);
>+        return true;
>+    }
> 
>     year = js_DoubleToInteger(year);
> 
>     if (!JSDOUBLE_IS_FINITE(result)) {
>         t = +0.0;
>     } else {
>         t = LocalTime(result);
>     }

While you are here, may as well remove overbracing or even switch to y = ... ? ... : ...;

>@@ -2586,17 +2525,17 @@ js_DateSetYear(JSContext *cx, JSObject *
>                               MonthFromTime(local),
>                               DateFromTime(local),
>                               HourFromTime(local),
>                               MinFromTime(local),
>                               SecFromTime(local),
>                               msFromTime(local));
> 
>     /* SetUTCTime also invalidates local time cache. */
>-    SetUTCTime(cx, obj, NULL, UTC(local));
>+    SetUTCTime(cx, obj, UTC(local));

Lame that these ancient (Netscape 2 era, for the server-side JS in "LiveWire") Friend APIs are void. File a followup bug?

Good catch on the double boxing.

/be

Comment 3

[Igor Bukanov]

Attachment: fix v2

The new patch addresses the comment 2.

Comment 4

[Igor Bukanov]

https://hg.mozilla.org/tracemonkey/rev/d31988919334

Comment 5

[Igor Bukanov]

Regarding the severity of this bug: it allows, for example, to overwrite the slots that stores array's length and count with a pointer to jsdouble containing NaN. Typically such pointer, when reinterpreted, would represent relatively big number that significantly exceeds array bounds. So the bug allows to bypass array bound checks and read-write pretty much arbitrary value after malloced memory containing array's elements.

Comment 6

[Igor Bukanov]

Attachment: test case that date methods throw on incompatible objects

The test case checks that all Date.prototype methods throw a TypeError when called with an incompatible this object.

Comment 7

[Bob Clary [:bc] (inactive)]

this caused (I think)  Bug 524671

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/d31988919334

Comment 9

[Samuel Sidler (old account; do not CC)]

(In reply to comment #7)
> this caused (I think)  Bug 524671

(It didn't, per that bug.)

Is this patch ready for 1.9.1? Can you please request approval if so? Code freeze is scheduled for November 10 at 11:59pm.

Comment 10

[Igor Bukanov]

See comment 5 regarding the danger of this bug.

Comment 11

[Igor Bukanov]

Attachment: fix for 1.9.1

This backport to 1.9.1 includes the fix for the regression from the bug 527027.

Comment 12

[Igor Bukanov]

Attachment: delta between 192 and 191

This is a proof that a backport was trivial one and patch for 1.9.2 failed to apply to 1.9.1 due to code movements.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 411194 [details] [diff] [review]
fix for 1.9.1

Approved for 1.9.1.6, a=dveditz for release-drivers

Comment 14

[Brendan Eich [:brendan]]

I sometimes pipe a plain diff of two patches into grep -v '^[<>]   ' to show the essential differences.

/be

Comment 15

[Igor Bukanov]

https://hg.mozilla.org/releases/mozilla-1.9.1/rev/18fb758d910b

Comment 16

[Samuel Sidler (old account; do not CC)]

Can we get this landed before code freeze tonight at 11:59pm PST?

Comment 17

[Igor Bukanov]

(In reply to comment #16)
> Can we get this landed before code freeze tonight at 11:59pm PST?

The bug has already landed on 1.9.1, see comment 15.

Comment 18

[Samuel Sidler (old account; do not CC)]

Yeah, sorry. My query was messed up. :-/

Comment 19

[Samuel Sidler (old account; do not CC)]

Comment on attachment 411194 [details] [diff] [review]
fix for 1.9.1

Ah ha! My query wasn't messed up. Dan approved this for 1.9.0 instead of 1.9.1. Switching approval flags to clean up queries.

Comment 20

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/98b566cb970d

Comment 21

[Samuel Sidler (old account; do not CC)]

These bugs landed after b4 was cut. Moving flag out.

Comment 22

[chris hofmann]

can we get some eyes on  Bug 535550  to figure out if that new crash in 3.5.6 is a possible regression from this fix?