[Extensions] Don't allow gin::Define to be overridden
Use DefineOwnProperty instead of Set in for gin, including gin::Define.
Replace Set in v8_helpers as well, to avoid the same problem.
Also update callsites from JS to CHECK expected arguments, rather
than DCHECK (since receiving unexpected arguments likely means
executing untrusted code).
BUG=549986
Review URL: https://codereview.chromium.org/1433293004
Cr-Commit-Position: refs/heads/master@{#359460}
diff --git a/chrome/browser/extensions/extension_bindings_apitest.cc b/chrome/browser/extensions/extension_bindings_apitest.cc
index 81e9b92..d99e574 100644
--- a/chrome/browser/extensions/extension_bindings_apitest.cc
+++ b/chrome/browser/extensions/extension_bindings_apitest.cc
@@ -138,6 +138,32 @@
EXPECT_EQ("success", result);
}
+IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest, NoGinDefineOverriding) {
+ ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+
+ // We need to create runtime bindings in the web page. An extension that's
+ // externally connectable will do that for us.
+ ASSERT_TRUE(LoadExtension(
+ test_data_dir_.AppendASCII("bindings")
+ .AppendASCII("externally_connectable_everywhere")));
+
+ ui_test_utils::NavigateToURL(
+ browser(),
+ embedded_test_server()->GetURL(
+ "/extensions/api_test/bindings/override_gin_define.html"));
+ ASSERT_FALSE(
+ browser()->tab_strip_model()->GetActiveWebContents()->IsCrashed());
+
+ // See chrome/test/data/extensions/api_test/bindings/override_gin_define.html.
+ std::string result;
+ EXPECT_TRUE(content::ExecuteScriptAndExtractString(
+ browser()->tab_strip_model()->GetActiveWebContents(),
+ "window.domAutomationController.send("
+ "document.getElementById('status').textContent.trim());",
+ &result));
+ EXPECT_EQ("success", result);
+}
+
IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest, HandlerFunctionTypeChecking) {
ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
ui_test_utils::NavigateToURL(
diff --git a/chrome/test/data/extensions/api_test/bindings/override_gin_define.html b/chrome/test/data/extensions/api_test/bindings/override_gin_define.html
new file mode 100644
index 0000000..3aaf8eeb
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/bindings/override_gin_define.html
@@ -0,0 +1,69 @@
+<!doctype html>
+<html>
+<body>
+<span id="status"></span>
+</body>
+<script>
+var error = '';
+var addError = function(newError) {
+ error += newError;
+ document.getElementById('status').textContent = error;
+};
+
+var succeed = function() {
+ if (error != '')
+ return; // Don't overwrite an existing error.
+ document.getElementById('status').textContent = 'success';
+}
+
+// Repro from crbug.com/549986.
+Object.prototype.__defineSetter__('define', function(v) {
+ if (typeof v == 'function') {
+ addError('Leaked gin define');
+ leakedDefine = v;
+ }
+ Object.defineProperty(this, 'define', {value: v});
+});
+
+var leakedBinding;
+Object.defineProperty(Object.prototype, 'create', {set: function(v) {
+ if (typeof(v) == 'function') {
+ Object.defineProperty(this, 'create', {value: function(name) {
+ result = v(name);
+ if (name == 'runtime') {
+ try {
+ leakedDefine('foo', ['test'], function(){} );
+ } catch (e) { }
+ } else if (name == 'test') {
+ addError('Leaked test');
+ leakedBinding = result;
+ }
+ return result;
+ }, configurable: true});
+ }
+}});
+
+// Bindings are lazily initialized. Poke it.
+chrome.runtime;
+// If the runtime bindings aren't created, we didn't test anything.
+if (!chrome.runtime)
+ addError('chrome.runtime was not created.\n');
+
+if (leakedBinding) {
+ leakedFunctions = {};
+ leakedBinding.customHooks_[0](
+ {apiFunctions: {setHandleRequest: function(name, fun) {
+ leakedFunctions[name] = fun;
+ } }, compiledApi: {} });
+
+ leakedFunctions.runWithNativesEnabled(function() {
+ addError('Calling activityLogger.LogEvent');
+ leakedFunctions.getModuleSystem(window).requireNative('activityLogger')
+ .LogEvent('', '', 0xDEADBAD);
+ });
+}
+
+// All's well.
+succeed();
+</script>
+</html>
diff --git a/extensions/renderer/api_activity_logger.cc b/extensions/renderer/api_activity_logger.cc
index 826abb0c..f92ce53 100644
--- a/extensions/renderer/api_activity_logger.cc
+++ b/extensions/renderer/api_activity_logger.cc
@@ -38,10 +38,10 @@
void APIActivityLogger::LogInternal(
const CallType call_type,
const v8::FunctionCallbackInfo<v8::Value>& args) {
- DCHECK_GT(args.Length(), 2);
- DCHECK(args[0]->IsString());
- DCHECK(args[1]->IsString());
- DCHECK(args[2]->IsArray());
+ CHECK_GT(args.Length(), 2);
+ CHECK(args[0]->IsString());
+ CHECK(args[1]->IsString());
+ CHECK(args[2]->IsArray());
std::string ext_id = *v8::String::Utf8Value(args[0]);
ExtensionHostMsg_APIActionOrEvent_Params params;
diff --git a/extensions/renderer/blob_native_handler.cc b/extensions/renderer/blob_native_handler.cc
index d6e757a3..12068c3 100644
--- a/extensions/renderer/blob_native_handler.cc
+++ b/extensions/renderer/blob_native_handler.cc
@@ -14,7 +14,7 @@
// Expects a single Blob argument. Returns the Blob's UUID.
void GetBlobUuid(const v8::FunctionCallbackInfo<v8::Value>& args) {
- DCHECK_EQ(1, args.Length());
+ CHECK_EQ(1, args.Length());
blink::WebBlob blob = blink::WebBlob::fromV8Value(args[0]);
args.GetReturnValue().Set(
v8::String::NewFromUtf8(args.GetIsolate(), blob.uuid().utf8().data()));
@@ -38,10 +38,10 @@
// a separate flow to avoid leaking Blobs if the script context is destroyed.
void BlobNativeHandler::TakeBrowserProcessBlob(
const v8::FunctionCallbackInfo<v8::Value>& args) {
- DCHECK_EQ(3, args.Length());
- DCHECK(args[0]->IsString());
- DCHECK(args[1]->IsString());
- DCHECK(args[2]->IsInt32());
+ CHECK_EQ(3, args.Length());
+ CHECK(args[0]->IsString());
+ CHECK(args[1]->IsString());
+ CHECK(args[2]->IsInt32());
std::string uuid(*v8::String::Utf8Value(args[0]));
std::string type(*v8::String::Utf8Value(args[1]));
blink::WebBlob blob =
diff --git a/extensions/renderer/file_system_natives.cc b/extensions/renderer/file_system_natives.cc
index d3b23af..12745096 100644
--- a/extensions/renderer/file_system_natives.cc
+++ b/extensions/renderer/file_system_natives.cc
@@ -31,8 +31,8 @@
void FileSystemNatives::GetIsolatedFileSystem(
const v8::FunctionCallbackInfo<v8::Value>& args) {
- DCHECK(args.Length() == 1 || args.Length() == 2);
- DCHECK(args[0]->IsString());
+ CHECK(args.Length() == 1 || args.Length() == 2);
+ CHECK(args[0]->IsString());
std::string file_system_id(*v8::String::Utf8Value(args[0]));
blink::WebLocalFrame* webframe =
blink::WebLocalFrame::frameForContext(context()->v8_context());
@@ -49,7 +49,7 @@
// system at which to root the DOMFileSystem we're returning to the caller.
std::string optional_root_name;
if (args.Length() == 2) {
- DCHECK(args[1]->IsString());
+ CHECK(args[1]->IsString());
optional_root_name = *v8::String::Utf8Value(args[1]);
}
@@ -66,8 +66,8 @@
void FileSystemNatives::GetFileEntry(
const v8::FunctionCallbackInfo<v8::Value>& args) {
- DCHECK(args.Length() == 5);
- DCHECK(args[0]->IsString());
+ CHECK_EQ(5, args.Length());
+ CHECK(args[0]->IsString());
std::string type_string = *v8::String::Utf8Value(args[0]);
blink::WebFileSystemType type;
bool is_valid_type = storage::GetFileSystemPublicType(type_string, &type);
@@ -76,16 +76,16 @@
return;
}
- DCHECK(args[1]->IsString());
- DCHECK(args[2]->IsString());
- DCHECK(args[3]->IsString());
+ CHECK(args[1]->IsString());
+ CHECK(args[2]->IsString());
+ CHECK(args[3]->IsString());
std::string file_system_name(*v8::String::Utf8Value(args[1]));
GURL file_system_root_url(*v8::String::Utf8Value(args[2]));
std::string file_path_string(*v8::String::Utf8Value(args[3]));
base::FilePath file_path = base::FilePath::FromUTF8Unsafe(file_path_string);
DCHECK(storage::VirtualPath::IsAbsolute(file_path.value()));
- DCHECK(args[4]->IsBoolean());
+ CHECK(args[4]->IsBoolean());
blink::WebDOMFileSystem::EntryType entry_type =
args[4]->BooleanValue() ? blink::WebDOMFileSystem::EntryTypeDirectory
: blink::WebDOMFileSystem::EntryTypeFile;
diff --git a/extensions/renderer/module_system.cc b/extensions/renderer/module_system.cc
index 3e3da6a..e84f0e0 100644
--- a/extensions/renderer/module_system.cc
+++ b/extensions/renderer/module_system.cc
@@ -101,8 +101,8 @@
void SetExportsProperty(
const v8::FunctionCallbackInfo<v8::Value>& args) {
v8::Local<v8::Object> obj = args.This();
- DCHECK_EQ(2, args.Length());
- DCHECK(args[0]->IsString());
+ CHECK_EQ(2, args.Length());
+ CHECK(args[0]->IsString());
v8::Maybe<bool> result =
obj->DefineOwnProperty(args.GetIsolate()->GetCurrentContext(),
args[0]->ToString(), args[1], v8::ReadOnly);
diff --git a/extensions/renderer/v8_helpers.h b/extensions/renderer/v8_helpers.h
index 2bfeee8..0c9a47c 100644
--- a/extensions/renderer/v8_helpers.h
+++ b/extensions/renderer/v8_helpers.h
@@ -7,6 +7,7 @@
#include <string.h>
+#include "base/strings/string_number_conversions.h"
#include "v8/include/v8.h"
namespace extensions {
@@ -56,19 +57,13 @@
return value.IsEmpty() || value->IsUndefined();
}
-// SetProperty() family wraps V8::Object::Set(). Returns true on success.
+// SetProperty() family wraps V8::Object::DefineOwnProperty().
+// Returns true on success.
inline bool SetProperty(v8::Local<v8::Context> context,
v8::Local<v8::Object> object,
- v8::Local<v8::Value> key,
+ v8::Local<v8::String> key,
v8::Local<v8::Value> value) {
- return IsTrue(object->Set(context, key, value));
-}
-
-inline bool SetProperty(v8::Local<v8::Context> context,
- v8::Local<v8::Object> object,
- uint32_t index,
- v8::Local<v8::Value> value) {
- return IsTrue(object->Set(context, index, value));
+ return IsTrue(object->DefineOwnProperty(context, key, value));
}
inline bool SetProperty(v8::Local<v8::Context> context,
@@ -81,6 +76,13 @@
return SetProperty(context, object, v8_key, value);
}
+inline bool SetProperty(v8::Local<v8::Context> context,
+ v8::Local<v8::Object> object,
+ uint32_t index,
+ v8::Local<v8::Value> value) {
+ return SetProperty(context, object, base::UintToString(index).c_str(), value);
+}
+
// GetProperty() family calls V8::Object::Get() and extracts a value from
// returned MaybeLocal. Returns true on success.
template <typename Key>
diff --git a/gin/converter.h b/gin/converter.h
index 8d17d41..12348e2 100644
--- a/gin/converter.h
+++ b/gin/converter.h
@@ -20,7 +20,8 @@
v8::Local<v8::Object> object,
KeyType key,
v8::Local<v8::Value> value) {
- auto maybe = object->Set(isolate->GetCurrentContext(), key, value);
+ auto maybe =
+ object->DefineOwnProperty(isolate->GetCurrentContext(), key, value);
return !maybe.IsNothing() && maybe.FromJust();
}
