Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Check array element length in ValueDeserializer #18565

Merged
merged 5 commits into from Jun 6, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions patches/common/v8/.patches
Expand Up @@ -24,3 +24,4 @@ turbofan_fix_wrong_typing_of_speculativesafeintegersubtract.patch
turbofan_restrict_redundancy_elimination_from_widening_types.patch
parser_literalbuffer_expandbuffer_always_grows.patch
valueserializer_report_if_buffer_expansion_fails_during.patch
check_array_element_length_in_valuedeserializer.patch
@@ -0,0 +1,112 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Milan Burda <milan.burda@gmail.com>
Date: Sat, 1 Jun 2019 23:36:46 +0200
Subject: Check array element length in ValueDeserializer

Backports:
- https://chromium-review.googlesource.com/c/v8/v8/+/1339699
- https://chromium-review.googlesource.com/c/v8/v8/+/1340283
- https://chromium-review.googlesource.com/c/v8/v8/+/1362046

diff --git a/src/value-serializer.cc b/src/value-serializer.cc
index 8c85bd9f5466612937a71f8bfdcb82120f7fb9c7..e624641c315f0d2c3423462928d8e1e900027031 100644
--- a/src/value-serializer.cc
+++ b/src/value-serializer.cc
@@ -1150,6 +1150,7 @@ void ValueDeserializer::TransferArrayBuffer(
}

MaybeHandle<Object> ValueDeserializer::ReadObject() {
+ DisallowJavascriptExecution no_js(isolate_);
MaybeHandle<Object> result = ReadObjectInternal();

// ArrayBufferView is special in that it consumes the value before it, even
@@ -1474,6 +1475,11 @@ MaybeHandle<JSArray> ValueDeserializer::ReadDenseJSArray() {
// hole. Past version 11, undefined means undefined.
if (version_ < 11 && element->IsUndefined(isolate_)) continue;

+ // Safety check.
+ if (i >= static_cast<uint32_t>(elements->length())) {
+ return MaybeHandle<JSArray>();
+ }
+
elements->set(i, *element);
}

@@ -1595,8 +1601,12 @@ MaybeHandle<JSMap> ValueDeserializer::ReadJSMap() {
}

Handle<Object> argv[2];
- if (!ReadObject().ToHandle(&argv[0]) || !ReadObject().ToHandle(&argv[1]) ||
- Execution::Call(isolate_, map_set, map, arraysize(argv), argv)
+ if (!ReadObject().ToHandle(&argv[0]) || !ReadObject().ToHandle(&argv[1])) {
+ return MaybeHandle<JSMap>();
+ }
+
+ AllowJavascriptExecution allow_js(isolate_);
+ if (Execution::Call(isolate_, map_set, map, arraysize(argv), argv)
.is_null()) {
return MaybeHandle<JSMap>();
}
@@ -1631,8 +1641,10 @@ MaybeHandle<JSSet> ValueDeserializer::ReadJSSet() {
}

Handle<Object> argv[1];
- if (!ReadObject().ToHandle(&argv[0]) ||
- Execution::Call(isolate_, set_add, set, arraysize(argv), argv)
+ if (!ReadObject().ToHandle(&argv[0])) return MaybeHandle<JSSet>();
+
+ AllowJavascriptExecution allow_js(isolate_);
+ if (Execution::Call(isolate_, set_add, set, arraysize(argv), argv)
.is_null()) {
return MaybeHandle<JSSet>();
}
@@ -1981,7 +1993,7 @@ Maybe<uint32_t> ValueDeserializer::ReadJSObjectProperties(
bool success;
LookupIterator it = LookupIterator::PropertyOrElement(
isolate_, object, key, &success, LookupIterator::OWN);
- if (!success ||
+ if (!success || it.state() != LookupIterator::NOT_FOUND ||
JSObject::DefineOwnPropertyIgnoreAttributes(&it, value, NONE)
.is_null()) {
return Nothing<uint32_t>();
@@ -2015,7 +2027,7 @@ Maybe<uint32_t> ValueDeserializer::ReadJSObjectProperties(
bool success;
LookupIterator it = LookupIterator::PropertyOrElement(
isolate_, object, key, &success, LookupIterator::OWN);
- if (!success ||
+ if (!success || it.state() != LookupIterator::NOT_FOUND ||
JSObject::DefineOwnPropertyIgnoreAttributes(&it, value, NONE)
.is_null()) {
return Nothing<uint32_t>();
@@ -2062,7 +2074,7 @@ static Maybe<bool> SetPropertiesFromKeyValuePairs(Isolate* isolate,
bool success;
LookupIterator it = LookupIterator::PropertyOrElement(
isolate, object, key, &success, LookupIterator::OWN);
- if (!success ||
+ if (!success || it.state() != LookupIterator::NOT_FOUND ||
JSObject::DefineOwnPropertyIgnoreAttributes(&it, value, NONE)
.is_null()) {
return Nothing<bool>();
diff --git a/test/unittests/value-serializer-unittest.cc b/test/unittests/value-serializer-unittest.cc
index bc738c84fc8b15bdfcdb0cc995f8002362292a4c..d81a4d2cc590e265221804bfa23a7a5967a3d042 100644
--- a/test/unittests/value-serializer-unittest.cc
+++ b/test/unittests/value-serializer-unittest.cc
@@ -1877,6 +1877,18 @@ TEST_F(ValueSerializerTest, DecodeDataView) {
ExpectScriptTrue("Object.getPrototypeOf(result) === DataView.prototype");
}

+TEST_F(ValueSerializerTest, DecodeArrayWithLengthProperty1) {
+ InvalidDecodeTest({0xff, 0x0d, 0x41, 0x03, 0x49, 0x02, 0x49, 0x04,
+ 0x49, 0x06, 0x22, 0x06, 0x6c, 0x65, 0x6e, 0x67,
+ 0x74, 0x68, 0x49, 0x02, 0x24, 0x01, 0x03});
+}
+
+TEST_F(ValueSerializerTest, DecodeArrayWithLengthProperty2) {
+ InvalidDecodeTest({0xff, 0x0d, 0x41, 0x03, 0x49, 0x02, 0x49, 0x04,
+ 0x49, 0x06, 0x22, 0x06, 0x6c, 0x65, 0x6e, 0x67,
+ 0x74, 0x68, 0x6f, 0x7b, 0x00, 0x24, 0x01, 0x03});
+}
+
TEST_F(ValueSerializerTest, DecodeInvalidDataView) {
// Byte offset out of range.
InvalidDecodeTest(