Replace v8::Object::Set with CreateDataProperty for security reasons.

chrome/renderer/extensions/automation_internal_custom_bindings.cc

Index: chrome/renderer/extensions/automation_internal_custom_bindings.cc
diff --git a/chrome/renderer/extensions/automation_internal_custom_bindings.cc b/chrome/renderer/extensions/automation_internal_custom_bindings.cc
index 5c7fe9c093e45a5fcda17c48621198cdaaad611c..531c66e761fcda15657f30a66ec7e9ace37e0ed4 100644
--- a/chrome/renderer/extensions/automation_internal_custom_bindings.cc
+++ b/chrome/renderer/extensions/automation_internal_custom_bindings.cc
@@ -47,28 +47,39 @@ void ThrowInvalidArgumentsException(
              << automation_bindings->context()->GetStackTraceAsString();
 }
 
-v8::Local<v8::Value> CreateV8String(v8::Isolate* isolate, const char* str) {
-  return v8::String::NewFromUtf8(isolate, str, v8::String::kNormalString,
-                                 strlen(str));
-}
-
-v8::Local<v8::Value> CreateV8String(v8::Isolate* isolate,
-                                    const std::string& str) {
+v8::Local<v8::String> CreateV8String(v8::Isolate* isolate,
+                                     const std::string& str) {
   return v8::String::NewFromUtf8(isolate, str.c_str(),

[Devlin, 2017/04/24 20:01:42]

nit: Let's use gin::StringToSymbol()

[dmazzoni, 2017/04/26 00:09:35]

Done.

                                  v8::String::kNormalString, str.length());
 }
 
+// Note: when building up an object to return from one of the
+// automation API bindings like a rect ({left: 0, top: 0, ...}) or
+// something like that, we should use this function instead of
+// v8::Object::Set, because a malicious extension author could use
+// Object.defineProperty to override a setter and trigger all sorts of
+// things to happen in the middle of one of our functions below.

[Devlin, 2017/04/24 20:01:42]

I'd expand this to say that this is only safe if the |object| is passed into
this function before it can be exposed to any untrusted script.  This isn't a
panacea for getters/setters, and only works because we know that the object
can't have any getters/setters installed yet.

[dmazzoni, 2017/04/26 00:09:35]

Done.

+void SafeSetV8Property(v8::Isolate* isolate,
+                       v8::Local<v8::Object> object,
+                       const std::string& key,

[Devlin, 2017/04/24 20:01:42]

s/const std::string&/base::StringPiece.

Since these are all passed in as raw strings, no need to allocate a full
std::string.

[dmazzoni, 2017/04/26 00:09:35]

Done.

+                       v8::Local<v8::Value> value) {
+  v8::Maybe<bool> maybe = object->CreateDataProperty(
+      isolate->GetCurrentContext(), CreateV8String(isolate, key), value);
+  if (!maybe.IsJust() || !maybe.FromJust())
+    LOG(ERROR) << "Failed to set property with key " << key;

[Devlin, 2017/04/24 20:01:42]

We should at least NOTREACHED() here.  Maybe even CHECK(maybe.ToChecked()),
since we don't think this is possible.

[dmazzoni, 2017/04/26 00:09:35]

Done. Note that maybe.ToChecked() is the same as maybe.FromJust().

+}
+
 v8::Local<v8::Object> RectToV8Object(v8::Isolate* isolate,
                                      const gfx::Rect& rect) {
   v8::Local<v8::Object> result(v8::Object::New(isolate));
-  result->Set(CreateV8String(isolate, "left"),
-              v8::Integer::New(isolate, rect.x()));
-  result->Set(CreateV8String(isolate, "top"),
-              v8::Integer::New(isolate, rect.y()));
-  result->Set(CreateV8String(isolate, "width"),
-              v8::Integer::New(isolate, rect.width()));
-  result->Set(CreateV8String(isolate, "height"),
-              v8::Integer::New(isolate, rect.height()));
+  SafeSetV8Property(isolate, result, "left",
+                    v8::Integer::New(isolate, rect.x()));
+  SafeSetV8Property(isolate, result, "top",
+                    v8::Integer::New(isolate, rect.y()));
+  SafeSetV8Property(isolate, result, "width",
+                    v8::Integer::New(isolate, rect.width()));
+  SafeSetV8Property(isolate, result, "height",
+                    v8::Integer::New(isolate, rect.height()));
   return result;
 }
 
@@ -945,10 +956,10 @@ void AutomationInternalCustomBindings::GetFocus(
 
   v8::Isolate* isolate = GetIsolate();
   v8::Local<v8::Object> result(v8::Object::New(isolate));
-  result->Set(CreateV8String(isolate, "treeId"),
-              v8::Integer::New(isolate, focused_tree_cache->tree_id));
-  result->Set(CreateV8String(isolate, "nodeId"),
-              v8::Integer::New(isolate, focused_node->id()));
+  SafeSetV8Property(isolate, result, "treeId",
+                    v8::Integer::New(isolate, focused_tree_cache->tree_id));
+  SafeSetV8Property(isolate, result, "nodeId",
+                    v8::Integer::New(isolate, focused_node->id()));
   args.GetReturnValue().Set(result);
 }
 
@@ -974,7 +985,7 @@ void AutomationInternalCustomBindings::GetHtmlAttributes(
   for (size_t i = 0; i < src.size(); i++) {
     std::string& key = src[i].first;
     std::string& value = src[i].second;
-    dst->Set(CreateV8String(isolate, key), CreateV8String(isolate, value));
+    SafeSetV8Property(isolate, dst, key, CreateV8String(isolate, value));
   }
   args.GetReturnValue().Set(dst);
 }
@@ -1001,7 +1012,7 @@ void AutomationInternalCustomBindings::GetState(
   while (state_shifter) {
     if (state_shifter & 1) {
       std::string key = ToString(static_cast<ui::AXState>(state_pos));
-      state->Set(CreateV8String(isolate, key), v8::Boolean::New(isolate, true));
+      SafeSetV8Property(isolate, state, key, v8::Boolean::New(isolate, true));
     }
     state_shifter = state_shifter >> 1;
     state_pos++;
@@ -1014,13 +1025,13 @@ void AutomationInternalCustomBindings::GetState(
   ui::AXNode* focused_node = nullptr;
   if (GetFocusInternal(top_cache, &focused_cache, &focused_node)) {
     if (focused_cache == cache && focused_node == node) {
-      state->Set(CreateV8String(isolate, "focused"),
-                 v8::Boolean::New(isolate, true));
+      SafeSetV8Property(isolate, state, "focused",
+                        v8::Boolean::New(isolate, true));
     }
   }
   if (cache->tree.data().focus_id == node->id()) {
-    state->Set(CreateV8String(isolate, "focused"),
-               v8::Boolean::New(isolate, true));
+    SafeSetV8Property(isolate, state, "focused",
+                      v8::Boolean::New(isolate, true));
   }
 
   args.GetReturnValue().Set(state);