Skip to content

Commit

Permalink
[JSC] Add extra hardening about incorrectly configured shared growabl…
Browse files Browse the repository at this point in the history
…e typed array view

https://bugs.webkit.org/show_bug.cgi?id=262338
rdar://116168654

Reviewed by Mark Lam.

This is adding extra hardening against wrongly configured shared growable typed array view materialization from SerializedScriptValue.
This pattern must not happen from normal execution. This happens only when the current process gets a bug which can emit arbitrary serialized
data. And since SharedArrayBuffer cannot be sent to the other process, this issue is confined in the current process. Given that the attacker
is already getting a way to create arbitrary serialized data, probably this does not add much additionally, but just adding hardening for now
as an extra safety.

* Source/JavaScriptCore/runtime/ArrayBufferView.h:
(JSC::ArrayBufferView::verifySubRangeLength):
* Source/JavaScriptCore/runtime/DataView.cpp:
(JSC::DataView::wrappedAs):
* Source/JavaScriptCore/runtime/GenericTypedArrayViewInlines.h:
(JSC::GenericTypedArrayView<Adaptor>::tryCreate):
(JSC::GenericTypedArrayView<Adaptor>::wrappedAs):
* Source/JavaScriptCore/runtime/JSDataView.cpp:
(JSC::JSDataView::create):
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:
(JSC::JSGenericTypedArrayView<Adaptor>::create):

Originally-landed-as: 267815.120@safari-7617-branch (ac9f4e0). rdar://119594133
Canonical link: https://commits.webkit.org/272091@main
  • Loading branch information
Constellation authored and robert-jenner committed Dec 15, 2023
1 parent f97d040 commit 409d5d9
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 7 deletions.
3 changes: 1 addition & 2 deletions Source/JavaScriptCore/runtime/ArrayBufferView.h
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,8 @@ class ArrayBufferView : public RefCounted<ArrayBufferView> {
}

// Helper to verify that a given sub-range of an ArrayBuffer is within range.
static bool verifySubRangeLength(const ArrayBuffer& buffer, size_t byteOffset, size_t numElements, unsigned elementSize)
static bool verifySubRangeLength(size_t byteLength, size_t byteOffset, size_t numElements, unsigned elementSize)
{
size_t byteLength = buffer.byteLength();
if (byteOffset > byteLength)
return false;
size_t remainingElements = (byteLength - byteOffset) / static_cast<size_t>(elementSize);
Expand Down
7 changes: 6 additions & 1 deletion Source/JavaScriptCore/runtime/DataView.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,12 @@ RefPtr<DataView> DataView::wrappedAs(Ref<ArrayBuffer>&& buffer, size_t byteOffse
// We do not check verifySubRangeLength for resizable buffer case since this function is only called from already created JS DataViews.
// It is possible that verifySubRangeLength fails when underlying ArrayBuffer is resized, but it is OK since it will be just recognized as OOB DataView.
if (!buffer->isResizableOrGrowableShared()) {
if (!ArrayBufferView::verifySubRangeLength(buffer.get(), byteOffset, byteLength.value_or(0), 1))
if (!ArrayBufferView::verifySubRangeLength(buffer->byteLength(), byteOffset, byteLength.value_or(0), 1))
return nullptr;
} else if (buffer->isGrowableShared()) {
// For growable buffer, we extra-check whether byteOffset and length are within maxByteLength.
// This does not hit in normal condition, just extra hardening.
if (!ArrayBufferView::verifySubRangeLength(buffer->maxByteLength().value(), byteOffset, byteLength.value_or(0), 1))
return nullptr;
}

Expand Down
9 changes: 7 additions & 2 deletions Source/JavaScriptCore/runtime/GenericTypedArrayViewInlines.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ RefPtr<GenericTypedArrayView<Adaptor>> GenericTypedArrayView<Adaptor>::tryCreate

ASSERT(length || buffer->isResizableOrGrowableShared());

if (!ArrayBufferView::verifySubRangeLength(*buffer, byteOffset, length.value_or(0), sizeof(typename Adaptor::Type)))
if (!ArrayBufferView::verifySubRangeLength(buffer->byteLength(), byteOffset, length.value_or(0), sizeof(typename Adaptor::Type)))
return nullptr;

if (!verifyByteOffsetAlignment(byteOffset, sizeof(typename Adaptor::Type)))
Expand All @@ -111,7 +111,12 @@ RefPtr<GenericTypedArrayView<Adaptor>> GenericTypedArrayView<Adaptor>::wrappedAs
// We do not check verifySubRangeLength for resizable buffer case since this function is only called from already created JS TypedArrays.
// It is possible that verifySubRangeLength fails when underlying ArrayBuffer is resized, but it is OK since it will be just recognized as OOB TypedArray.
if (!buffer->isResizableOrGrowableShared()) {
if (!ArrayBufferView::verifySubRangeLength(buffer.get(), byteOffset, length.value_or(0), sizeof(typename Adaptor::Type)))
if (!ArrayBufferView::verifySubRangeLength(buffer->byteLength(), byteOffset, length.value_or(0), sizeof(typename Adaptor::Type)))
return nullptr;
} else if (buffer->isGrowableShared()) {
// For growable buffer, we extra-check whether byteOffset and length are within maxByteLength.
// This does not hit in normal condition, just extra hardening.
if (!ArrayBufferView::verifySubRangeLength(buffer->maxByteLength().value(), byteOffset, length.value_or(0), sizeof(typename Adaptor::Type)))
return nullptr;
}

Expand Down
2 changes: 1 addition & 1 deletion Source/JavaScriptCore/runtime/JSDataView.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ JSDataView* JSDataView::create(

ASSERT(byteLength || buffer->isResizableOrGrowableShared());

if (!ArrayBufferView::verifySubRangeLength(*buffer, byteOffset, byteLength.value_or(0), sizeof(uint8_t))) {
if (!ArrayBufferView::verifySubRangeLength(buffer->byteLength(), byteOffset, byteLength.value_or(0), sizeof(uint8_t))) {
throwRangeError(globalObject, scope, "Length out of range of buffer"_s);
return nullptr;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ JSGenericTypedArrayView<Adaptor>* JSGenericTypedArrayView<Adaptor>::create(JSGlo

ASSERT(length || buffer->isResizableOrGrowableShared());

if (!ArrayBufferView::verifySubRangeLength(*buffer, byteOffset, length.value_or(0), elementSize)) {
if (!ArrayBufferView::verifySubRangeLength(buffer->byteLength(), byteOffset, length.value_or(0), elementSize)) {
throwException(globalObject, scope, createRangeError(globalObject, "Length out of range of buffer"_s));
return nullptr;
}
Expand Down

0 comments on commit 409d5d9

Please sign in to comment.