bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

"Assertion failure: !argc || JSVAL_IS_NULL(argv[0]) || JSVAL_IS_VOID(argv[0])"

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
12 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: Igor Bukanov)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla1.9.2a1
x86
Mac OS X
assertion, testcase, verified1.9.0.9, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
wanted1.9.0.x +
wanted1.8.1.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [needs 1.8.? landing: needs approval(s)] [sg:dos] null deref fixed-in-tracemonkey)

Attachments

(7 attachments)

(Reporter)

Description

12 years ago
js> new Object({valueOf: /a/})
Assertion failure: !argc || JSVAL_IS_NULL(argv[0]) || JSVAL_IS_VOID(argv[0]), at jsobj.c:1736
Seems almost harmless in opt builds.  (The object returned is missing its valueOf, which doesn't happen if valueOf is a number or a function.)
(Reporter)

Updated

11 years ago
No longer blocks: 349611
(Reporter)

Updated

11 years ago
Blocks: 349611
new Object({valueOf: /a/})

Assertion failure: !argc || JSVAL_IS_NULL(argv[0]) || JSVAL_IS_VOID(argv[0]), at ../jsobj.cpp:1981

asserts with TM tip, latest 1.9.0.x branch. I tried finding a regression window for this, besides discovering that this issue has existed since 01-01-2003 debug js shell, I couldn't compile a js shell for anything before that, so this is really ancient.
Keywords: crash → assertion
Hardware: PowerPC → x86
Created attachment 366248 [details]
backtrace, normal and full
(Assignee)

Updated

10 years ago
Assignee: general → igor
(Assignee)

Comment 3

10 years ago
The assert in js_Object enforces the assumption that js_ValueToObject can only return null when the passed value is either null or undefined, see http://hg.mozilla.org/tracemonkey/file/5374efa7d144/js/src/jsobj.cpp#l1969 .

But this is not the case when the function is applied to { valueOf: /a/ }. Here js_ValueToObject, http://hg.mozilla.org/tracemonkey/file/5374efa7d144/js/src/jsobj.cpp#l5362 , calls OBJ_DEFAULT_VALUE that eventually extracts the value of the "valueOf" property and try to call it as a function. Since the value is a regexp object which define a call operator, an attempt to call it succeeds and returns the result of executing the regexp. The result would be null which will also becomes the result of OBJ_DEFAULT_VALUE. After that js_ValueToObject uses JSVAL_IS_OBJECT to check the result. Since null passes the check, it is returned instead of the original object.

Another way to demonstrate the assert is to replace {valueOf: /a/} with {valueOf: function() { return null; }}.

So to fix this it is sufficient to replace JSVAL_IS_OBJECT with !JSVAL_IS_PRIMITIVE.

Here is the test case to check that js_ValueToObject should return the original object if OBJ_DEFAULT_VALUE returns a primitive value:

var primitiveValues = [
    true, false, 0, 1, -2, 0.1, -2e100, 0/0, 1/0, -1/1,  "", "xxx",
    undefined, null
];

for (var i = 0; i != primitiveValues; ++i) {
    var v = primitiveValues[i];
    var obj = { valueOf: function() { return v; } };
    var obj2 = Object(obj);
    if (obj !== obj2)
        throw "Bad conversion for "+uneval(v);
}

With a debug build of js shell the test case asserts and with an optimized js shell it throws the exception about bad conversion.
(Assignee)

Comment 4

10 years ago
(In reply to comment #3)
> Here is the test case to check that js_ValueToObject should return the original
> object if OBJ_DEFAULT_VALUE returns a primitive value:
> 
> var primitiveValues = [
>     true, false, 0, 1, -2, 0.1, -2e100, 0/0, 1/0, -1/1,  "", "xxx",
>     undefined, null
> ];
> 
> for (var i = 0; i != primitiveValues; ++i) {


This is wrong, it should use primitiveValues.length, not just primitiveValues. So the whole test case reads:

var primitiveValues = [
    true, false, 0, 1, -2, 0.1, -2e100, 0/0, 1/0, -1/1,  "", "xxx",
    undefined, null
];

for (var i = 0; i != primitiveValues.length; ++i) {
    var v = primitiveValues[i];
    var obj = { valueOf: function() { return v; } };
    var obj2 = Object(obj);
    if (obj !== obj2)
        throw "Bad conversion for "+uneval(v);
}
(Assignee)

Comment 5

10 years ago
Created attachment 366295 [details] [diff] [review]
v1

As explained in the above comments, the patch uses !JSVAL_IS_PRIMITIVE, not JSVAL_IS_OBJECT to filter out null values.
Attachment #366295 - Flags: review?(crowder)
(Assignee)

Comment 6

10 years ago
The bug is not harmless. It leads to crash through dereference of zero address in array_join_sub as the function also assumes that js_ValueToObject can only return null for null or undefined argument:

@watson~> cat ~/s/x.js
var a = [ { valueOf: function() { return null; } } ];
a.toLocaleString();
@watson~> ~/build/js32.tm.dbg/js ~/s/x.js
Segmentation fault


I mark the bug as sensitive until it can be shown that null dereference is the worst possible scenario.
Group: core-security
Comment on attachment 366295 [details] [diff] [review]
v1

This code goes back to the dawn of time. ECMA does not convert object to null, but the ancient world allowed it (bugs and all).

I don't see how this could be other than a null deref.

/be
Attachment #366295 - Flags: review?(crowder) → review+

Updated

10 years ago
Flags: blocking1.9.1?

Updated

10 years ago
Flags: blocking1.9.1? → blocking1.9.1+
Should this (very ancient) issue then be considered necessary for backport to 1.9.0.x and/or 1.8.1.x ?

(Wow, resurrection after initial discovery some 2.5 years ago of a bug from the age of dinosaurs)
Flags: blocking1.9.0.8?
Flags: blocking1.8.1.next?
We'll approve branch patches for this, but not blocking if we don't think it's worse than a null deref.
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: blocking1.9.0.8?
Flags: blocking1.8.1.next?
Whiteboard: [sg:nse] null deref
(Assignee)

Comment 10

10 years ago
(In reply to comment #7)
> I don't see how this could be other than a null deref.

This bug itself shows that it can "be other than a null deref"! In an optimized build due to the bug the code takes a different path in js_Object without any null derefs.

So I have to be clear: the patch from the comment 6 changes the sementic of js_ValueToObject and backporting it to the branches may lead to regressions. The risk can be extremely small, but I do not want to speculate about probabilities here. What is clear though, is that if there is currently code that relies on js_ValueToObject calling valueOf property, that code may well assume the behavior with the patch applied. And for this reason I would recommend applying the patch to the branches.   

Also note that from a quick look at mxr.mozilla.org over JS_ValueToObject usage in xpconnect I am not 100% sure that the bug would lead at worst to a null deref and not to a privilege escalation.
(Assignee)

Comment 11

10 years ago
(In reply to comment #8)
> (Wow, resurrection after initial discovery some 2.5 years ago of a bug from the
> age of dinosaurs)

This also shows the danger of ignoring apparently debug-only problems.
Created attachment 366345 [details] [diff] [review]
*tested* 1.9.0.x patch backport

Here's the backport to 1.9.0.x branch, it is still untested though.

Should I nominate blocking1.9.0.8 again due to the reasons Igor lists in comment #10?
Attachment #366345 - Flags: review?(igor)
Here's a diff (just-in-case):

$ diff ~/Desktop/190patch-v1.diff ~/Desktop/valuetoobject.patch 
1c1
< Index: js/src/jsobj.c
---
> Index: tm/js/src/jsobj.cpp
3,8c3,5
< RCS file: /cvsroot/mozilla/js/src/jsobj.c,v
< retrieving revision 3.479
< diff -u -8 -p -r3.479 jsobj.c
< --- js/src/jsobj.c    8 Jan 2009 07:18:31 -0000       3.479
< +++ js/src/jsobj.c    9 Mar 2009 18:43:36 -0000
< @@ -4721,17 +4721,17 @@ js_ValueToObject(JSContext *cx, jsval v,
---
> --- tm.orig/js/src/jsobj.cpp  2009-03-09 15:13:07.000000000 +0100
> +++ tm/js/src/jsobj.cpp       2009-03-09 15:13:37.000000000 +0100
> @@ -5365,17 +5365,17 @@ js_ValueToObject(JSContext *cx, jsval v,
(Assignee)

Updated

10 years ago
Attachment #366345 - Flags: review?(igor) → review+
Comment on attachment 366345 [details] [diff] [review]
*tested* 1.9.0.x patch backport

I tested the patch again to be doubly triply sure.

With fix in 1.9.0.x:

$ ./js
js> new Object({valueOf: /a/})
[object Object]
js> 
js> 

Without fix in 1.9.0.x:

$ ./js
js> new Object({valueOf: /a/})
Assertion failure: !argc || JSVAL_IS_NULL(argv[0]) || JSVAL_IS_VOID(argv[0]), at jsobj.c:1772
Attachment #366345 - Attachment description: untested 1.9.0.x patch backport → *tested* 1.9.0.x patch backport
Attachment #366345 - Flags: approval1.9.0.8?
(In reply to comment #10)
> (In reply to comment #7)
> > I don't see how this could be other than a null deref.
> 
> This bug itself shows that it can "be other than a null deref"! In an optimized
> build due to the bug the code takes a different path in js_Object without any
> null derefs.

Yes, it's bad -- but how would this lead to an FMR or other exploitable memory safety bug? We can't prove it doesn't (we don't prove negatives except by static analysis or by-hand proofs), but I was asking if you had proof that it does.

/be
(Assignee)

Comment 16

10 years ago
(In reply to comment #15)
> Yes, it's bad -- but how would this lead to an FMR or other exploitable memory
> safety bug?

In xpconnect the result of js_ValueToObject in few places is passed to functions asserting that the object is not null. Then the code uses defensive programming apparently to allow such null argument. On the first look I could not judge that this defensive programming is sound and could not be used to direct flow control to skip, for example, security checks.

> We can't prove it doesn't (we don't prove negatives except by
> static analysis or by-hand proofs),

Yes, a good doctor would never assert that a test has found evidence of no disease. Instead he would just say the test has found no evidence of disease.  


> but I was asking if you had proof that it
> does.

At this point I have neither proof that it does nor that it does not ;)
(Assignee)

Comment 17

10 years ago
landed to TM - http://hg.mozilla.org/tracemonkey/rev/5226430e2cdd
Whiteboard: [sg:nse] null deref → [sg:nse] null deref fixed-in-tracemonkey
XPConnect should be using js_ValueToNonNullObject if it can't stand a null result for any input. Is it really defensive to a null output in the case of a null or undefined input?

The js_ValueTo{,NonNull}Object split is ancient and intentional. Changing it so OBJ_DEFAULT_VALUE cannot "convert" obj to null may be a good thing, but the fact remains that js_ValueToObject can return a null result.

/be
(Assignee)

Comment 19

10 years ago
(In reply to comment #18)
> but the
> fact remains that js_ValueToObject can return a null result.

Sure, and that is not a problem. The bug happens because the code assumes that if the input is not null or undefined (after been filtered out with other checks), then the result of js_ValueToObject cannot be null.
(In reply to comment #19)
> (In reply to comment #18)
> > but the
> > fact remains that js_ValueToObject can return a null result.
> 
> Sure, and that is not a problem. The bug happens because the code assumes that
> if the input is not null or undefined (after been filtered out with other
> checks), then the result of js_ValueToObject cannot be null.

Yeah, I got that -- just making sure nothing is being missed by anyone (most of all me). This is really about ECMA conformance, after all these years.

/be

Comment 21

10 years ago
http://hg.mozilla.org/mozilla-central/rev/5226430e2cdd
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 22

10 years ago
Created attachment 367071 [details]
js1_5/Regress/regress-355829-01.js

Comment 23

10 years ago
Created attachment 367072 [details]
js1_5/Regress/regress-355829-02.js

Comment 24

10 years ago
Created attachment 367073 [details]
js1_5/Regress/regress-355829-03.js

Updated

10 years ago
Flags: in-testsuite+
Comment on attachment 366345 [details] [diff] [review]
*tested* 1.9.0.x patch backport

Approved for 1.9.0.8, a=dveditz for release-drivers
Attachment #366345 - Flags: approval1.9.0.8? → approval1.9.0.8+
Adding checkin-needed for landing on both 1.9.1 and 1.9.0.x (more for the latter than the former, and after a period of baking)
Keywords: checkin-needed
(Assignee)

Comment 27

10 years ago
landed to 1.9.0 branch:

Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.481; previous revision: 3.480
Keywords: fixed1.9.0.8
(In reply to comment #27)
> landed to 1.9.0 branch:
> 
> Checking in jsobj.c;
> /cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
> new revision: 3.481; previous revision: 3.480

Did this make the 3.0.8 freeze? Else it should be fixed1.9.0.9...
Yes, it made it.

Comment 30

9 years ago
v 1.9.0, 1.9.2 and 1.9.1 tracemonkey. still need check into 1.9.1.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.0.8 → verified1.9.0.8
Comment on attachment 371868 [details] [diff] [review]
1.8.0 version

Can you please review this one?
(Assignee)

Updated

9 years ago
Attachment #371868 - Flags: review?(igor) → review+
Group: core-security
Whiteboard: [sg:nse] null deref fixed-in-tracemonkey → [sg:dos] null deref fixed-in-tracemonkey
Bob, the patch has been checked in for 1.9.1. Is it looking good to verify?
Keywords: checkin-needed
Whiteboard: [sg:dos] null deref fixed-in-tracemonkey → [needs 1.8.? landing: needs approval(s)] [sg:dos] null deref fixed-in-tracemonkey
Target Milestone: --- → mozilla1.9.2a1

Comment 35

9 years ago
js1_5/Regress/regress-355829-01.js
js1_5/Regress/regress-355829-02.js
js1_5/Regress/regress-355829-03.js
http://hg.mozilla.org/tracemonkey/rev/fec7b4e5792e

Comment 36

9 years ago
v 1.9.1, 1.9.2
Keywords: fixed1.9.1 → verified1.9.1

Comment 37

9 years ago
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-355829-01.js,v  <--  regress-355829-01.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-355829-02.js,v  <--  regress-355829-02.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-355829-03.js,v  <--  regress-355829-03.js
initial revision: 1.1
You need to log in before you can comment on or make changes to this bug.