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

chrome/renderer/extensions/automation_internal_custom_bindings.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/renderer/extensions/automation_internal_custom_bindings.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <memory>
@@ skipping 29 matching lines @@
       v8::String::NewFromUtf8(
           isolate,
           "Invalid arguments to AutomationInternalCustomBindings function",
           v8::NewStringType::kNormal)
           .ToLocalChecked());
 
   LOG(FATAL) << "Invalid arguments to AutomationInternalCustomBindings function"
              << 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::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());
 }
 
-v8::Local<v8::Value> CreateV8String(v8::Isolate* isolate,
-                                    const std::string& str) {
-  return v8::String::NewFromUtf8(isolate, str.c_str(),
-                                 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;
 }
 
 // Compute the bounding box of a node, fixing nodes with empty bounds by
 // unioning the bounds of their children.
 static gfx::RectF ComputeLocalNodeBounds(TreeCache* cache, ui::AXNode* node) {
   gfx::RectF bounds = node->data().location;
   if (bounds.width() > 0 && bounds.height() > 0)
     return bounds;
 
@@ skipping 856 matching lines @@
   if (!cache)
     return;
 
   TreeCache* focused_tree_cache = nullptr;
   ui::AXNode* focused_node = nullptr;
   if (!GetFocusInternal(cache, &focused_tree_cache, &focused_node))
     return;
 
   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);
 }
 
 void AutomationInternalCustomBindings::GetHtmlAttributes(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::Isolate* isolate = GetIsolate();
   if (args.Length() < 2 || !args[0]->IsNumber() || !args[1]->IsNumber())
     ThrowInvalidArgumentsException(this);
 
   int tree_id = args[0]->Int32Value();
   int node_id = args[1]->Int32Value();
 
   TreeCache* cache = GetTreeCacheFromTreeID(tree_id);
   if (!cache)
     return;
 
   ui::AXNode* node = cache->tree.GetFromId(node_id);
   if (!node)
     return;
 
   v8::Local<v8::Object> dst(v8::Object::New(isolate));
   base::StringPairs src = node->data().html_attributes;
   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);
 }
 
 void AutomationInternalCustomBindings::GetState(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::Isolate* isolate = GetIsolate();
   if (args.Length() < 2 || !args[0]->IsNumber() || !args[1]->IsNumber())
     ThrowInvalidArgumentsException(this);
 
   int tree_id = args[0]->Int32Value();
   int node_id = args[1]->Int32Value();
 
   TreeCache* cache = GetTreeCacheFromTreeID(tree_id);
   if (!cache)
     return;
 
   ui::AXNode* node = cache->tree.GetFromId(node_id);
   if (!node)
     return;
 
   v8::Local<v8::Object> state(v8::Object::New(isolate));
   uint32_t state_pos = 0, state_shifter = node->data().state;
   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++;
   }
 
   TreeCache* top_cache = GetTreeCacheFromTreeID(0);
   if (!top_cache)
     top_cache = cache;
   TreeCache* focused_cache = nullptr;
   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);
 }
 
 void AutomationInternalCustomBindings::UpdateOverallTreeChangeObserverFilter() {
   tree_change_observer_overall_filter_ = 0;
   for (const auto& observer : tree_change_observers_)
     tree_change_observer_overall_filter_ |= 1 << observer.filter;
 }
@@ skipping 379 matching lines @@
     for (auto id : ids)
       nodes->AppendInteger(id);
     args.Append(std::move(nodes));
   }
 
   bindings_system_->DispatchEventInContext("automationInternal.onNodesRemoved",
                                            &args, nullptr, context());
 }
 
 }  // namespace extensions