Bug 221569 – nullptr deref in jsCryptoPrototypeFunction_getRandomValuesBody if out of memory

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

e.g.

0   com.apple.WebCore                   0x00000003223a4f81 WTF::RefPtr<JSC::ArrayBuffer, WTF::RawPtrTraits<JSC::ArrayBuffer>, WTF::DefaultRefDerefTraits<JSC::ArrayBuffer> >::operator!() const + 0 (RefPtr.h:85) [inlined]
1   com.apple.WebCore                   0x00000003223a4f81 JSC::ArrayBufferView::isDetached() const + 0 (ArrayBufferView.h:47) [inlined]
2   com.apple.WebCore                   0x00000003223a4f81 JSC::ArrayBufferView::isShared() const + 0 (ArrayBufferView.h:66) [inlined]
3   com.apple.WebCore                   0x00000003223a4f81 JSC::JSArrayBufferView::unsharedImpl() + 14 (JSArrayBufferViewInlines.h:79) [inlined]
4   com.apple.WebCore                   0x00000003223a4f81 JSC::JSArrayBufferView::toWrapped(JSC::VM&, JSC::JSValue) + 129 (JSArrayBufferViewInlines.h:127)
5   com.apple.WebCore                   0x000000032244159f WTF::RefPtr<JSC::ArrayBufferView, WTF::RawPtrTraits<JSC::ArrayBufferView>, WTF::DefaultRefDerefTraits<JSC::ArrayBufferView> > WebCore::Detail::BufferSourceConverter<WebCore::IDLArrayBufferView, (WebCore::Detail::BufferSourceConverterAllowSharedMode)1>::convert<WebCore::jsCryptoPrototypeFunction_getRandomValuesBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSCrypto*)::'lambda'(JSC::JSGlobalObject&, JSC::ThrowScope&)>(JSC::JSGlobalObject&, JSC::JSValue, WebCore::jsCryptoPrototypeFunction_getRandomValuesBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSCrypto*)::'lambda'(JSC::JSGlobalObject&, JSC::ThrowScope&)&&) + 20 (JSDOMConvertBufferSource.h:127) [inlined]
6   com.apple.WebCore                   0x000000032244159f WTF::RefPtr<JSC::ArrayBufferView, WTF::RawPtrTraits<JSC::ArrayBufferView>, WTF::DefaultRefDerefTraits<JSC::ArrayBufferView> > WebCore::Converter<WebCore::IDLArrayBufferView>::convert<WebCore::jsCryptoPrototypeFunction_getRandomValuesBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSCrypto*)::'lambda'(JSC::JSGlobalObject&, JSC::ThrowScope&)>(JSC::JSGlobalObject&, JSC::JSValue, WebCore::jsCryptoPrototypeFunction_getRandomValuesBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSCrypto*)::'lambda'(JSC::JSGlobalObject&, JSC::ThrowScope&)&&) + 20 (JSDOMConvertBufferSource.h:391) [inlined]
7   com.apple.WebCore                   0x000000032244159f WebCore::Converter<WebCore::IDLArrayBufferView>::ReturnType WebCore::convert<WebCore::IDLArrayBufferView, WebCore::jsCryptoPrototypeFunction_getRandomValuesBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSCrypto*)::'lambda'(JSC::JSGlobalObject&, JSC::ThrowScope&)>(JSC::JSGlobalObject&, JSC::JSValue, WebCore::jsCryptoPrototypeFunction_getRandomValuesBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSCrypto*)::'lambda'(JSC::JSGlobalObject&, JSC::ThrowScope&)&&) + 20 (JSDOMConvertBase.h:76) [inlined]
8   com.apple.WebCore                   0x000000032244159f WebCore::jsCryptoPrototypeFunction_getRandomValuesBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSCrypto*) + 42 (JSCrypto.cpp:219) [inlined]
9   com.apple.WebCore                   0x000000032244159f long long WebCore::IDLOperation<WebCore::JSCrypto>::call<&(WebCore::jsCryptoPrototypeFunction_getRandomValuesBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSCrypto*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) + 187 (JSDOMOperation.h:53) [inlined]
10  com.apple.WebCore                   0x000000032244159f WebCore::jsCryptoPrototypeFunction_getRandomValues(JSC::JSGlobalObject*, JSC::CallFrame*) + 207 (JSCrypto.cpp:228)
11  ???                                 0x00002a9fd14011d8 0 + 46865898803672
12  com.apple.JavaScriptCore            0x0000000328367a5d llint_entry + 108286 (LowLevelInterpreter.asm:1093)

<rdar://problem/74098645>

[Radar WebKit Bug Importer]

<rdar://problem/74109720>

[Ryosuke Niwa]

Created attachment 419613 [details]
Test

[Rob Buis]

Created attachment 421195 [details]
Patch

[Rob Buis]

The patch fixes the crash. If people agree this is not a security problem (looks like a null pointer crash to me) I will add the testcase.

[Ryosuke Niwa]

Comment on attachment 421195 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:79
> -    RELEASE_ASSERT(!result->isShared());
> +    RELEASE_ASSERT(!result || !result->isShared());

Why is it okay to return nullptr here?
Why are we getting nullptr from possiblySharedImpl?

[Rob Buis]

Created attachment 421308 [details]
Patch

[Rob Buis]

Created attachment 421314 [details]
Patch

[Rob Buis]

Created attachment 421318 [details]
Patch

[Rob Buis]

Comment on attachment 421195 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:79
>> +    RELEASE_ASSERT(!result || !result->isShared());
> 
> Why is it okay to return nullptr here?
> Why are we getting nullptr from possiblySharedImpl?

possiblySharedImpl is not able to allocate in OOM situation so nullptr is returned. I think it is okay to return nullptr, this will result in an Exception being thrown, which seems appropriate for me in this situation.

[Yusuke Suzuki]

Comment on attachment 421318 [details]
Patch

Yes, this is correct.

[EWS]

Committed r273373: <https://commits.webkit.org/r273373>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421318 [details].

[Ryan Haddad]

It looks like the new test added in https://commits.webkit.org/r273373 is frequently failing on the bots

https://results.webkit.org/?suite=layout-tests&test=crypto%2Fcrypto-random-values-oom.html

--- /Volumes/Data/slave/catalina-debug-tests-wk1/build/layout-test-results/crypto/crypto-random-values-oom-expected.txt
+++ /Volumes/Data/slave/catalina-debug-tests-wk1/build/layout-test-results/crypto/crypto-random-values-oom-actual.txt
@@ -1,3 +1,4 @@
+CONSOLE MESSAGE: Unhandled Promise Rejection: RangeError: Out of memory
 Test crypto.getRandomValues behavior in oom situation.
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

[Ryan Haddad]

Reverted r273373 for reason:

The test added with this change is frequently failing

Committed r273418 (234524@main): <https://commits.webkit.org/234524@main>

[Darin Adler]

This revert doesn’t make logical sense. It make the assertions stronger.

[Darin Adler]

Oh, I see, reverting the *test* is valuable.

Reverting the code change, not valuable, but it ends up carried along.

[Rob Buis]

Created attachment 421683 [details]
Patch

[Rob Buis]

Comment on attachment 421683 [details]
Patch

The test was unstable because of the console output, this new patch suppresses it.

[Ryosuke Niwa]

Comment on attachment 421683 [details]
Patch

Looks like the test is timing out on Windows. Perhaps we wanna skip it there.

[Rob Buis]

Created attachment 421741 [details]
Patch

[EWS]

Committed r273624: <https://commits.webkit.org/r273624>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421741 [details].