[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}

5 files changed, 32 insertions, 30 deletions

diff --git a/extensions/renderer/api_activity_logger.cc b/extensions/renderer/api_activity_logger.cc
index 826abb0..f92ce53 100644
--- a/extensions/renderer/api_activity_logger.cc
+++ b/extensions/renderer/api_activity_logger.cc
@@ -38,10 +38,10 @@ void APIActivityLogger::LogEvent(
 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 d6e757a..12068c3 100644
--- a/extensions/renderer/blob_native_handler.cc
+++ b/extensions/renderer/blob_native_handler.cc
@@ -14,7 +14,7 @@ namespace {
 
 // 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 @@ BlobNativeHandler::BlobNativeHandler(ScriptContext* context)
 // 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..1274509 100644
--- a/extensions/renderer/file_system_natives.cc
+++ b/extensions/renderer/file_system_natives.cc
@@ -31,8 +31,8 @@ FileSystemNatives::FileSystemNatives(ScriptContext* context)
 
 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 @@ void FileSystemNatives::GetIsolatedFileSystem(
   // 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::GetIsolatedFileSystem(
 
 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 @@ void FileSystemNatives::GetFileEntry(
     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 @@ class DefaultExceptionHandler : public ModuleSystem::ExceptionHandler {
 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 @@ inline bool IsEmptyOrUndefied(v8::Local<v8::Value> value) {
   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 @@ inline bool SetProperty(v8::Local<v8::Context> context,
   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>