summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorrdevlin.cronin <rdevlin.cronin@chromium.org>2016-03-01 16:13:47 -0800
committerCommit bot <commit-bot@chromium.org>2016-03-02 00:15:10 +0000
commit75b803b1c81ed9fa5513cbff550232b4fb915e7b (patch)
tree0521ba16bc6f3655bb51c81892a79fffc2765dc3
parente69130f5b1a31d11badc7e034252038dc03b8ec6 (diff)
downloadchromium_src-75b803b1c81ed9fa5513cbff550232b4fb915e7b.zip
chromium_src-75b803b1c81ed9fa5513cbff550232b4fb915e7b.tar.gz
chromium_src-75b803b1c81ed9fa5513cbff550232b4fb915e7b.tar.bz2
[Extensions] Harden against bindings interception
There's more we can do but this is a start. BUG=590275 BUG=590118 Review URL: https://codereview.chromium.org/1748943002 Cr-Commit-Position: refs/heads/master@{#378621}
Diffstat
-rw-r--r--chrome/browser/extensions/extension_bindings_apitest.cc25
-rw-r--r--chrome/test/data/extensions/api_test/bindings/function_interceptions.html56
-rw-r--r--extensions/renderer/module_system.cc4
-rw-r--r--extensions/renderer/v8_helpers.h43
4 files changed, 126 insertions, 2 deletions
diff --git a/chrome/browser/extensions/extension_bindings_apitest.cc b/chrome/browser/extensions/extension_bindings_apitest.cc
index 07f6ac3..387576f 100644
--- a/chrome/browser/extensions/extension_bindings_apitest.cc
+++ b/chrome/browser/extensions/extension_bindings_apitest.cc
@@ -183,5 +183,30 @@ IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest, HandlerFunctionTypeChecking) {
EXPECT_EQ("success", result);
}
+IN_PROC_BROWSER_TEST_F(ExtensionBindingsApiTest,
+ MoreNativeFunctionInterceptionTests) {
+ ASSERT_TRUE(embedded_test_server()->Start());
+
+ // 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/function_interceptions.html"));
+ content::WebContents* web_contents =
+ browser()->tab_strip_model()->GetActiveWebContents();
+ EXPECT_FALSE(web_contents->IsCrashed());
+ // See function_interceptions.html.
+ std::string result;
+ EXPECT_TRUE(content::ExecuteScriptAndExtractString(
+ web_contents, "window.domAutomationController.send(window.testStatus);",
+ &result));
+ EXPECT_EQ("success", result);
+}
+
} // namespace
} // namespace extensions
diff --git a/chrome/test/data/extensions/api_test/bindings/function_interceptions.html b/chrome/test/data/extensions/api_test/bindings/function_interceptions.html
new file mode 100644
index 0000000..07342ca5
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/bindings/function_interceptions.html
@@ -0,0 +1,56 @@
+<body>
+<script>
+
+window.testStatus = '';
+var objects = ['runtime', 'require', 'test', 'binding'];
+var leaked = [];
+
+function intercept(objectKey) {
+ Object.defineProperty(Object.prototype, objectKey, {
+ get: function () {
+ leaked.push({name: objectKey, obj: this});
+ },
+ set: function (v) {
+ Object.defineProperty(this, objectKey, {
+ value: v,
+ configurable: true,
+ enumerable: true,
+ writable: true
+ });
+ },
+ configurable: true,
+ });
+}
+
+// Set up interceptors.
+for (let objectKey of objects)
+ intercept(objectKey);
+
+// Poke chrome.runtime and chrome.app.
+try {
+ chrome.runtime;
+} catch (e) {}
+try {
+ chrome.app;
+} catch (e) {}
+
+// Cleanup - we don't want to be triggering our own interceptors.
+for (let objKey of objects)
+ delete Object.prototype[objKey];
+
+// Check what we intercepted.
+var keysToCheck = ['utils', 'binding'];
+for (let nameAndObj of leaked) {
+ for (let key of keysToCheck) {
+ if (!!nameAndObj.obj[key]) {
+ window.testStatus +=
+ 'Failed: Found ' + key + ' on ' + nameAndObj.name + '\n';
+ }
+ }
+}
+
+if (window.testStatus === '')
+ window.testStatus = 'success';
+
+</script>
+</body>
diff --git a/extensions/renderer/module_system.cc b/extensions/renderer/module_system.cc
index ef96d73..f027413 100644
--- a/extensions/renderer/module_system.cc
+++ b/extensions/renderer/module_system.cc
@@ -251,12 +251,12 @@ v8::Local<v8::Value> ModuleSystem::RequireForJsInner(
v8::Local<v8::Object> modules(v8::Local<v8::Object>::Cast(modules_value));
v8::Local<v8::Value> exports;
- if (!GetProperty(v8_context, modules, module_name, &exports) ||
+ if (!GetPrivateProperty(v8_context, modules, module_name, &exports) ||
!exports->IsUndefined())
return handle_scope.Escape(exports);
exports = LoadModule(*v8::String::Utf8Value(module_name));
- SetProperty(v8_context, modules, module_name, exports);
+ SetPrivateProperty(v8_context, modules, module_name, exports);
return handle_scope.Escape(exports);
}
diff --git a/extensions/renderer/v8_helpers.h b/extensions/renderer/v8_helpers.h
index 2a6fa9c..3017772 100644
--- a/extensions/renderer/v8_helpers.h
+++ b/extensions/renderer/v8_helpers.h
@@ -60,6 +60,9 @@ inline bool IsEmptyOrUndefied(v8::Local<v8::Value> value) {
// SetProperty() family wraps V8::Object::DefineOwnProperty().
// Returns true on success.
+// NOTE: Think about whether you want this or SetPrivateProperty() below.
+// TODO(devlin): Sort through more of the callers of this and see if we can
+// convert more to be private.
inline bool SetProperty(v8::Local<v8::Context> context,
v8::Local<v8::Object> object,
v8::Local<v8::String> key,
@@ -84,8 +87,29 @@ inline bool SetProperty(v8::Local<v8::Context> context,
return SetProperty(context, object, base::UintToString(index).c_str(), value);
}
+// Wraps v8::Object::SetPrivate(). When possible, prefer this to SetProperty().
+inline bool SetPrivateProperty(v8::Local<v8::Context> context,
+ v8::Local<v8::Object> object,
+ v8::Local<v8::String> key,
+ v8::Local<v8::Value> value) {
+ return IsTrue(object->SetPrivate(
+ context, v8::Private::ForApi(context->GetIsolate(), key), value));
+}
+
+inline bool SetPrivateProperty(v8::Local<v8::Context> context,
+ v8::Local<v8::Object> object,
+ const char* key,
+ v8::Local<v8::Value> value) {
+ v8::Local<v8::String> v8_key;
+ return ToV8String(context->GetIsolate(), key, &v8_key) &&
+ IsTrue(object->SetPrivate(
+ context, v8::Private::ForApi(context->GetIsolate(), v8_key),
+ value));
+}
+
// GetProperty() family calls V8::Object::Get() and extracts a value from
// returned MaybeLocal. Returns true on success.
+// NOTE: Think about whether you want this or GetPrivateProperty() below.
template <typename Key>
inline bool GetProperty(v8::Local<v8::Context> context,
v8::Local<v8::Object> object,
@@ -104,6 +128,25 @@ inline bool GetProperty(v8::Local<v8::Context> context,
return GetProperty(context, object, v8_key, out);
}
+// Wraps v8::Object::GetPrivate(). When possible, prefer this to GetProperty().
+inline bool GetPrivateProperty(v8::Local<v8::Context> context,
+ v8::Local<v8::Object> object,
+ v8::Local<v8::String> key,
+ v8::Local<v8::Value>* out) {
+ return object
+ ->GetPrivate(context, v8::Private::ForApi(context->GetIsolate(), key))
+ .ToLocal(out);
+}
+
+inline bool GetPrivateProperty(v8::Local<v8::Context> context,
+ v8::Local<v8::Object> object,
+ const char* key,
+ v8::Local<v8::Value>* out) {
+ v8::Local<v8::String> v8_key;
+ return ToV8String(context->GetIsolate(), key, &v8_key) &&
+ GetPrivateProperty(context, object, v8_key, out);
+}
+
// GetPropertyUnsafe() family wraps v8::Object::Get(). They crash when an
// exception is thrown.
inline v8::Local<v8::Value> GetPropertyUnsafe(v8::Local<v8::Context> context,
generated by cgit v1.1 at 2025-01-08 03:39:16 +0000