fix: Check array element length in ValueDeserializer

electron · Jun 1, 2019 · f8a95fe · f8a95fe
1 parent a6117ab
commit f8a95fe
Show file tree

Hide file tree

Showing 2 changed files with 113 additions and 0 deletions.
diff --git a/patches/common/v8/.patches b/patches/common/v8/.patches
@@ -19,3 +19,4 @@ disable-warning-win.patch
 expose_mksnapshot.patch
 build-torque-with-x64-toolchain-on-arm.patch
 do_not_run_arm_arm64_mksnapshot_binaries.patch
+check_array_element_length_in_valuedeserializer.patch
diff --git a/patches/common/v8/check_array_element_length_in_valuedeserializer.patch b/patches/common/v8/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(