Bug 164123 – [JSC] JSON.stringify should handle Proxy which is non JSArray but isArray is true

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Kamil Frankowicz]

Created attachment 293147 [details]
POC to trigger SEGFAULT (jsc)

Affected SVN revision: 208042

To reproduce the problem:
./jsc wekbit_string_builder.js

ASAN Output:

ASAN:DEADLYSIGNAL
=================================================================
==16295==ERROR: AddressSanitizer: SEGV on unknown address 0x6041000021a3 (pc 0x7fa5adf38ee4 bp 0x0000ffffffff sp 0x7ffd4d2f6790 T0)
==16295==The signal is caused by a READ memory access.
    #0 0x7fa5adf38ee3 in WTF::StringBuilder::operator[](unsigned int) const XYZ/webkit/Source/WTF/wtf/text/StringBuilder.h:247:20
    #1 0x7fa5adf38ee3 in JSC::Stringifier::Holder::appendNextProperty(JSC::Stringifier&, WTF::StringBuilder&) XYZ/webkit/Source/JavaScriptCore/runtime/JSONObject.cpp:465
    #2 0x7fa5adf35501 in JSC::Stringifier::appendStringifiedValue(WTF::StringBuilder&, JSC::JSValue, JSC::JSObject*, JSC::PropertyNameForFunctionCall const&) XYZ/webkit/Source/JavaScriptCore/runtime/JSONObject.cpp:384:37
    #3 0x7fa5adf31deb in JSC::Stringifier::stringify(JSC::Handle<JSC::Unknown>) XYZ/webkit/Source/JavaScriptCore/runtime/JSONObject.cpp:262:9
    #4 0x7fa5adf406c4 in JSC::JSONProtoFuncStringify(JSC::ExecState*) XYZ/webkit/Source/JavaScriptCore/runtime/JSONObject.cpp:786:57
    #5 0x7fa565afe027  (<unknown module>)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV XYZ/webkit/Source/WTF/wtf/text/StringBuilder.h:247:20 in WTF::StringBuilder::operator[](unsigned int) const
==16295==ABORTING


Regards,
Kamil Frankowicz

[Radar WebKit Bug Importer]

<rdar://problem/29015948>

[Yusuke Suzuki]

More easy way is `JSON.parse(new Proxy([undefined], {}));`. investigating.

[Yusuke Suzuki]

s/parse/stringify/

[Yusuke Suzuki]

Created attachment 293328 [details]
Patch

[Mark Lam]

Comment on attachment 293328 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293328&action=review

> Source/JavaScriptCore/ChangeLog:11
> +        When JSON.stringify encounter the value like undefined, which results depend
> +        on the context. If it is produced under the object property context, we ignore
> +        that property. On the other hand, if it is produced under the array element
> +        context, we produce "null".

Can you quote the relevant urls to the spec here?

> Source/JavaScriptCore/runtime/JSONObject.cpp:265
> +    Holder root(Holder::RootHolder, vm, object);

Why can't you just use:
    Holder root(vm, m_exec, object);

> Source/JavaScriptCore/runtime/JSONObject.cpp:426
> +    , m_isArray(JSC::isArray(exec, object))

m_isJSArray is uninitialized here.  Is that intentional?

> Source/JavaScriptCore/runtime/JSONObject.cpp:440
> +inline Stringifier::Holder::Holder(RootHolderTag, VM& vm, JSObject* object)
> +    : m_object(vm, object)
> +    , m_isArray(false)
> +    , m_isJSArray(false)
>      , m_index(0)
>  #ifndef NDEBUG
>      , m_size(0)

Why do we need this version?  Won't the Holder(VM& vm, ExecState* exec, JSObject* object) work just as well?

[Yusuke Suzuki]

Comment on attachment 293328 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293328&action=review

Thanks!

>> Source/JavaScriptCore/ChangeLog:11
>> +        context, we produce "null".
> 
> Can you quote the relevant urls to the spec here?

Yup! I'll include it.
https://tc39.github.io/ecma262/#sec-serializejsonarray we write "null".
But https://tc39.github.io/ecma262/#sec-serializejsonobject, we skip the property in 8-b.

>> Source/JavaScriptCore/runtime/JSONObject.cpp:265
>> +    Holder root(Holder::RootHolder, vm, object);
> 
> Why can't you just use:
>     Holder root(vm, m_exec, object);

I don't think using it directly is good. It calls `isArray` for the given object and isArray() could raise exceptions for some objects.
Of course, here, the exception never happens. But I would like to explicitly say that this Holder does not cause any observable behaviors.
That's why I did not include ExecState* in this constructor.

>> Source/JavaScriptCore/runtime/JSONObject.cpp:426
>> +    , m_isArray(JSC::isArray(exec, object))
> 
> m_isJSArray is uninitialized here.  Is that intentional?

Yes, that is intentional. And this function is original one and it is not changed.
This m_isJSArray will be initialized when we call appendNextProperty for this holder.

>> Source/JavaScriptCore/runtime/JSONObject.cpp:440
>>      , m_size(0)
> 
> Why do we need this version?  Won't the Holder(VM& vm, ExecState* exec, JSObject* object) work just as well?

Yeah, it works. But I don't think it is good manner.
And since we do not call appendNextProperty for this root Holder, some of the fields (m_isJSArray) remains uninitialized.
I don't think it is good thing.

[Yusuke Suzuki]

Notable example is,

JSON.stringify({ value: undefined }) => "{}"
JSON.stringify([undefined]) => "[null]"

[Yusuke Suzuki]

Comment on attachment 293328 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293328&action=review

>>> Source/JavaScriptCore/runtime/JSONObject.cpp:426
>>> +    , m_isArray(JSC::isArray(exec, object))
>> 
>> m_isJSArray is uninitialized here.  Is that intentional?
> 
> Yes, that is intentional. And this function is original one and it is not changed.
> This m_isJSArray will be initialized when we call appendNextProperty for this holder.

But, I think initializing it is better. Since isJSArray() is not observable one, we can just call it here.
I'll change this part.

[Yusuke Suzuki]

Created attachment 293335 [details]
Patch

[Mark Lam]

Comment on attachment 293335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293335&action=review

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        When JSON.stringify encounter the value like undefined, which results depend

/the value like undefined/the undefined value/
/which results depend/the result depends/

> Source/JavaScriptCore/ChangeLog:14
> +            // https://tc39.github.io/ecma262/#sec-serializejsonobject

I suggest adding "section 8.b" after the spec url above.

> Source/JavaScriptCore/ChangeLog:18
> +            // https://tc39.github.io/ecma262/#sec-serializejsonarray

I suggest adding "section 8.b" after the spec url above.

> JSTests/stress/json-stringify-with-non-jsarray-array.js:24
> +shouldBe(JSON.stringify(new Proxy([function() { }], {})), `[null]`);

Can you also add the test case for JSON.stringify(new Proxy({ value: undefined }, {}))?

[Yusuke Suzuki]

Comment on attachment 293335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293335&action=review

>> Source/JavaScriptCore/ChangeLog:8
>> +        When JSON.stringify encounter the value like undefined, which results depend
> 
> /the value like undefined/the undefined value/
> /which results depend/the result depends/

Oops, fixed.

>> Source/JavaScriptCore/ChangeLog:14
>> +            // https://tc39.github.io/ecma262/#sec-serializejsonobject
> 
> I suggest adding "section 8.b" after the spec url above.

Yeah, nice. Fixed.

>> Source/JavaScriptCore/ChangeLog:18
>> +            // https://tc39.github.io/ecma262/#sec-serializejsonarray
> 
> I suggest adding "section 8.b" after the spec url above.

Fixed.

>> JSTests/stress/json-stringify-with-non-jsarray-array.js:24
>> +shouldBe(JSON.stringify(new Proxy([function() { }], {})), `[null]`);
> 
> Can you also add the test case for JSON.stringify(new Proxy({ value: undefined }, {}))?

Yup, added.

[Yusuke Suzuki]

Committed r208123: <http://trac.webkit.org/changeset/208123>

[Darin Adler]

Comment on attachment 293335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293335&action=review

> Source/JavaScriptCore/runtime/JSONObject.cpp:431
> +    , m_index(0)
> +#ifndef NDEBUG
> +    , m_size(0)
> +#endif

It would be better to initialize these in the class definition.

> Source/JavaScriptCore/runtime/JSONObject.cpp:441
> +    , m_isArray(false)
> +    , m_isJSArray(false)
>      , m_index(0)
>  #ifndef NDEBUG
>      , m_size(0)

It would be better to initialize these in the class definition.

[Yusuke Suzuki]

Comment on attachment 293335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293335&action=review

Thanks, I'll create a follow-up patch.

>> Source/JavaScriptCore/runtime/JSONObject.cpp:431
>> +#endif
> 
> It would be better to initialize these in the class definition.

Sounds nice. I'll move this function to the class definition.
And use default initialization for some members (m_index for example).

>> Source/JavaScriptCore/runtime/JSONObject.cpp:441
>>      , m_size(0)
> 
> It would be better to initialize these in the class definition.

Ditto.

[Kamil Frankowicz]

This is CVE-2016-10222.