Date.setTime sets slots without checking for Date class




10 years ago
9 years ago


(Reporter: igor, Assigned: igor)



Dependency tree / graph
Bug Flags:
blocking1.9.2 +
wanted1.9.0.x -

Firefox Tracking Flags

(status1.9.2 beta5-fixed, blocking1.9.1 .6+, status1.9.1 .6-fixed)


(Whiteboard: [sg:critical] fixed-in-tracemonkey)


(4 attachments, 1 obsolete attachment)



10 years ago
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');;
~/m/tm/js/src> ~/build/ ~/s/x.js
Assertion failure: STOBJ_GET_CLASS(obj) == &js_DateClass, at /home/igor/m/tm/js/src/jsdate.cpp:1227
Trace/breakpoint trap
Flags: blocking1.9.2?


10 years ago
blocking1.9.1: --- → ?

Comment 1

10 years ago
Posted patch fix v1 (obsolete) — Splinter Review
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.


10 years ago
Assignee: general → igor


10 years ago
Flags: blocking1.9.2? → blocking1.9.2+


10 years ago
Attachment #408034 - Flags: review?(brendan)
blocking1.9.1: ? → .5+
Flags: wanted1.9.0.x-
Whiteboard: [sg:critical?] too limited to exploit?
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, 
>+    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.

Attachment #408034 - Flags: review?(brendan) → review+

Comment 3

10 years ago
Posted patch fix v2Splinter Review
The new patch addresses the comment 2.
Attachment #408034 - Attachment is obsolete: true
Attachment #408219 - Flags: review+

Comment 4

10 years ago
Whiteboard: [sg:critical?] too limited to exploit? → [sg:critical?] too limited to exploit? fixed-in-tracemonkey

Comment 5

10 years ago
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

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


10 years ago
Depends on: 524671

Comment 7

10 years ago
this caused (I think)  Bug 524671


10 years ago
No longer depends on: 524671

Comment 8

10 years ago
Last Resolved: 10 years ago
Resolution: --- → FIXED
(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.


10 years ago
Depends on: 527027

Comment 10

10 years ago
See comment 5 regarding the danger of this bug.
Whiteboard: [sg:critical?] too limited to exploit? fixed-in-tracemonkey → [sg:critical] fixed-in-tracemonkey

Comment 11

10 years ago
Posted patch fix for 1.9.1Splinter Review
This backport to 1.9.1 includes the fix for the regression from the bug 527027.
Attachment #411194 - Flags: review+

Comment 12

10 years ago
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.


10 years ago
Attachment #411194 - Flags: approval1.9.0.16?
Comment on attachment 411194 [details] [diff] [review]
fix for 1.9.1

Approved for, a=dveditz for release-drivers
Attachment #411194 - Flags: approval1.9.0.16? → approval1.9.0.16+
I sometimes pipe a plain diff of two patches into grep -v '^[<>]   ' to show the essential differences.

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

Comment 17

10 years ago
(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.
Yeah, sorry. My query was messed up. :-/
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.
Attachment #411194 - Flags: approval1.9.0.16+ → approval1.9.1.6+
These bugs landed after b4 was cut. Moving flag out.

Comment 22

10 years ago
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?
Group: core-security
You need to log in before you can comment on or make changes to this bug.