Implement cross-origin attributes using access check interceptors.

third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl

Index: third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
diff --git a/third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl b/third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
index 01285ef6476d60ab7440e4352ac38c3876343484..8ef493842bcb8831af01963cb29e2a81966529e1 100644
--- a/third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
+++ b/third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
@@ -121,27 +121,6 @@ static void (*{{method.name}}MethodForPartialInterface)(const v8::FunctionCallba
 {% endfor %}
 {% endfor %}
 {##############################################################################}
-{% block security_check_functions %}
-{% if has_access_check_callbacks and not is_partial %}
-bool securityCheck(v8::Local<v8::Context> accessingContext, v8::Local<v8::Object> accessedObject, v8::Local<v8::Value> data) {
-  {% if interface_name == 'Window' %}
-  v8::Isolate* isolate = v8::Isolate::GetCurrent();
-  v8::Local<v8::Object> window = V8Window::findInstanceInPrototypeChain(accessedObject, isolate);
-  if (window.IsEmpty())
-    return false; // the frame is gone.
-
-  const DOMWindow* targetWindow = V8Window::toImpl(window);
-  return BindingSecurity::shouldAllowAccessTo(toLocalDOMWindow(toDOMWindow(accessingContext)), targetWindow, BindingSecurity::ErrorReportOption::DoNotReport);
-  {% else %}{# if interface_name == 'Window' #}
-  {# Not 'Window' means it\'s Location. #}
-  {{cpp_class}}* impl = {{v8_class}}::toImpl(accessedObject);
-  return BindingSecurity::shouldAllowAccessTo(toLocalDOMWindow(toDOMWindow(accessingContext)), impl, BindingSecurity::ErrorReportOption::DoNotReport);
-  {% endif %}{# if interface_name == 'Window' #}
-}
-
-{% endif %}
-{% endblock %}
-{##############################################################################}
 {# Methods #}
 {% from 'methods.cpp.tmpl' import generate_method, overload_resolution_method,
        method_callback, origin_safe_method_getter, generate_constructor,
@@ -176,7 +155,7 @@ bool securityCheck(v8::Local<v8::Context> accessingContext, v8::Local<v8::Object
 {{method_callback(method, world_suffix)}}
 {% endif %}
 {% endif %}
-{% if method.is_do_not_check_security and method.visible %}
+{% if method.is_cross_origin and method.visible %}
 {{origin_safe_method_getter(method, world_suffix)}}
 {% endif %}
 {% endfor %}
@@ -209,6 +188,121 @@ bool securityCheck(v8::Local<v8::Context> accessingContext, v8::Local<v8::Object
 {% block indexed_property_setter_callback %}{% endblock %}
 {% block indexed_property_deleter %}{% endblock %}
 {% block indexed_property_deleter_callback %}{% endblock %}
+{##############################################################################}
+{% block security_check_functions %}
+{% if has_access_check_callbacks and not is_partial %}
+bool securityCheck(v8::Local<v8::Context> accessingContext, v8::Local<v8::Object> accessedObject, v8::Local<v8::Value> data) {
+  {% if interface_name == 'Window' %}
+  v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  v8::Local<v8::Object> window = V8Window::findInstanceInPrototypeChain(accessedObject, isolate);
+  if (window.IsEmpty())
+    return false; // the frame is gone.

[haraken, 2016/12/08 08:21:02]

Not really. window.IsEmpty happens when the prototype chain is tweaked. Whether
the frame is gone is checked by the following
BindingSecurity::shouldAllowAccessTo.

[dcheng, 2016/12/08 09:04:11]

I'm just copying and pasting code here =)

What would you suggest as a more accurate comment?

[haraken, 2016/12/09 02:26:55]

"It's not a window wrapper" :)

[dcheng, 2016/12/09 05:06:14]

Strictly speaking, is that true though? We only set access check callbacks for
Window and Location, and this code is specific for handling Window. So it must
be a window wrapper, but somehow got disassociated by changing the prototype
chain.

+
+  const DOMWindow* targetWindow = V8Window::toImpl(window);
+  return BindingSecurity::shouldAllowAccessTo(toLocalDOMWindow(toDOMWindow(accessingContext)), targetWindow, BindingSecurity::ErrorReportOption::DoNotReport);
+  {% else %}{# if interface_name == 'Window' #}
+  {# Not 'Window' means it\'s Location. #}
+  {{cpp_class}}* impl = {{v8_class}}::toImpl(accessedObject);
+  return BindingSecurity::shouldAllowAccessTo(toLocalDOMWindow(toDOMWindow(accessingContext)), impl, BindingSecurity::ErrorReportOption::DoNotReport);
+  {% endif %}{# if interface_name == 'Window' #}
+}
+
+{% if has_cross_origin_named_enumerator %}
+// TODO(dcheng): Can we / should we use AtomicString here? That means using DEFINE_STATIC_LOCAL here.

[haraken, 2016/12/08 08:21:02]

I'm fine with const char*.

[dcheng, 2016/12/08 09:04:11]

Done. I removed the TODO... I think it might still be an interesting
optimization, but maybe it's not a big deal.

+static const struct {
+  using GetterCallback = void(*)(const v8::PropertyCallbackInfo<v8::Value>&);
+  using SetterCallback = void(*)(v8::Local<v8::Value>, const V8CrossOriginSetterInfo&);
+
+  const char* const name;
+  const GetterCallback getter;
+  const SetterCallback setter;
+} kCrossOriginAttributeTable[] = {
+  {##### Cross-origin attributes #####}
+  {% for attribute in attributes if attribute.has_cross_origin_getter or attribute.has_cross_origin_setter %}
+  {
+    "{{attribute.name}}",
+    {%+ if attribute.has_cross_origin_getter %}&{{cpp_class}}V8Internal::{{attribute.name}}AttributeGetter{% else %}nullptr{% endif %},
+    {%+ if attribute.has_cross_origin_setter %}&{{cpp_class}}V8Internal::{{attribute.name}}AttributeSetter{% else %}nullptr{% endif %},
+  },
+  {% endfor %}
+  {##### Cross-origin methods #####}
+  {% for method in methods if method.is_cross_origin %}
+  {"{{method.name}}", &{{cpp_class}}V8Internal::{{method.name}}OriginSafeMethodGetter, nullptr},

[haraken, 2016/12/08 08:21:02]

After this CL, OriginSafeMethodSetter is no longer used, right? If so, can we
remove code for OriginSafeMethodSetter?

[dcheng, 2016/12/08 09:04:11]

Unfortunately, it is still needed. Originally, I tried removing this, and this
broke things like postMessage: event.source would be set incorrectly.

postMessage and the location setters need the calling context (incumbent realm
in spec terminology): they use this to determine if the calling context has
access to the target context. Since v8 is trying to get rid of
GetCallingContext(), we need to support this another way: instead, we create
function objects for each realm, so that when we call GetCurrentContext(), it
ends up pointing to the context which we created the function object for.

With this hack, if document A post messages document B, GetCurrentContext() ==
document A, and we enforce security checks / set event.source correctly, etc.

However, if we do not create a per-realm function object, then
GetCurrentContext() == the context we are calling *into*: that means that when
document A post messages document B, GetCurrentContext() == document B. This
breaks security checks for Location, as well as event.source (since event.source
doesn't point back at the caller).

[haraken, 2016/12/09 02:26:56]

Yes, I understand why we still need to keep OriginSafeMethodGetter. My question
is if we still need OriginSafeMethodSetter.

[dcheng, 2016/12/09 05:06:13]

I thought that this would be needed so that if JS overwrites the property, we
can return the user-supplied value rather than the function object. I might be
missing something, but I don't see how I'd be able to make this work without a
setter.

+  {% endfor %}
+};
+{% endif %}
+
+{% if has_cross_origin_named_getter %}
+void crossOriginNamedGetter(v8::Local<v8::Name> name, const v8::PropertyCallbackInfo<v8::Value>& info) {

[haraken, 2016/12/08 08:21:02]

Just to confirm: V8 calls back crossOriginNamedGetter only when securityCheck
returns false, right? I just want to check if the overhead of
crossOriginNamedGetter doesn't affect normal window.foo accesses.

[dcheng, 2016/12/08 09:04:11]

That's correct: this interceptor is only invoked when the access check fails.

+  if (!name->IsString())
+    return;
+  const AtomicString& propertyName = toCoreAtomicString(name.As<v8::String>());
+
+  for (const auto& attribute: kCrossOriginAttributeTable) {

[Yuki, 2016/12/07 12:03:40]

nit: attribute : kCrossOrigin...
(one space before the colon)

[dcheng, 2016/12/08 09:04:11]

Done.

+    if (propertyName == attribute.name && attribute.getter) {
+      attribute.getter(info);
+      return;
+    }
+  }
+
+  {% if named_property_getter and named_property_getter.is_cross_origin %}
+  {% if named_property_getter.is_custom %}
+  {{v8_class}}::namedPropertyGetterCustom(propertyName, info);
+  {% else %}
+  {{cpp_class}}V8Internal::namedPropertyGetter(propertyName, info);
+  {% endif %}
+  {% else %}
+  BindingSecurity::failedAccessCheckFor(
+      info.GetIsolate(),
+      {{v8_class}}::toImpl(info.Holder())->frame());
+  {% endif %}
+}
+{% endif %}
+
+{% if has_cross_origin_named_setter %}
+void crossOriginNamedSetter(v8::Local<v8::Name> name, v8::Local<v8::Value> value, const v8::PropertyCallbackInfo<v8::Value>& info) {
+  if (!name->IsString())
+    return;
+  const AtomicString& propertyName = toCoreAtomicString(name.As<v8::String>());
+
+  for (const auto& attribute: kCrossOriginAttributeTable) {

[Yuki, 2016/12/07 12:03:40]

ditto

[dcheng, 2016/12/08 09:04:11]

Done.

+    if (propertyName == attribute.name && attribute.setter) {
+      attribute.setter(value, V8CrossOriginSetterInfo(info.GetIsolate(), info.Holder()));
+      return;
+    }
+  }
+

[haraken, 2016/12/08 08:21:02]

Why don't you need to call V8Internal::namedPropertySetter when
named_property_setter.is_cross_origin is set?

[dcheng, 2016/12/08 09:04:11]

I could add this for consistency... but there is no IDL that would use it, and
it would not be tested at all. So I just omitted it.

[haraken, 2016/12/09 02:26:56]

Okay, let's add a comment.

[dcheng, 2016/12/09 05:06:13]

Done.

+  BindingSecurity::failedAccessCheckFor(
+      info.GetIsolate(),
+      {{v8_class}}::toImpl(info.Holder())->frame());
+}
+{% endif %}
+
+{% if has_cross_origin_named_enumerator %}
+void crossOriginNamedEnumerator(const v8::PropertyCallbackInfo<v8::Array>& info) {
+  Vector<String> names;
+  for (const auto& attribute : kCrossOriginAttributeTable)
+    names.append(attribute.name);
+
+  v8SetReturnValue(
+      info,
+      toV8(names, info.Holder(), info.GetIsolate()).As<v8::Array>());
+}
+{% endif %}
+
+{% if has_cross_origin_indexed_getter %}
+void crossOriginIndexedGetter(uint32_t index, const v8::PropertyCallbackInfo<v8::Value>& info) {
+  {% if indexed_property_getter.is_custom %}
+  {{v8_class}}::indexedPropertyGetterCustom(index, info);

[haraken, 2016/12/08 08:21:02]

Is there any real use case of indexedPropertyGetterCustom?

[dcheng, 2016/12/08 09:04:10]

See my previous comment about window[0], window[1], etc.

+  {% else %}
+  {{cpp_class}}V8Internal::indexedPropertyGetter(index, info);
+  {% endif %}
+}
+{% endif %}
+
+{% endif %}
+{% endblock %}
+{##############################################################################}
 } // namespace {{cpp_class_or_partial}}V8Internal
 
 {% block visit_dom_wrapper %}{% endblock %}
@@ -350,7 +444,11 @@ static void install{{v8_class}}Template(v8::Isolate* isolate, const DOMWrapperWo
   {% endfilter %}
   {%- if has_access_check_callbacks and not is_partial %}{{newline}}
   // Cross-origin access check
-  instanceTemplate->SetAccessCheckCallback({{cpp_class}}V8Internal::securityCheck, v8::External::New(isolate, const_cast<WrapperTypeInfo*>(&{{v8_class}}::wrapperTypeInfo)));
+  {% set cross_origin_named_getter = '%sV8Internal::crossOriginNamedGetter' % cpp_class if has_cross_origin_named_getter else 'nullptr' %}
+  {% set cross_origin_named_setter = '%sV8Internal::crossOriginNamedSetter' % cpp_class if has_cross_origin_named_setter else 'nullptr' %}
+  {% set cross_origin_named_enumerator = '%sV8Internal::crossOriginNamedEnumerator' % cpp_class if has_cross_origin_named_enumerator else 'nullptr' %}
+  {% set cross_origin_indexed_getter = '%sV8Internal::crossOriginIndexedGetter' % cpp_class if has_cross_origin_indexed_getter else 'nullptr' %}
+  instanceTemplate->SetAccessCheckCallbackAndHandler({{cpp_class}}V8Internal::securityCheck, v8::NamedPropertyHandlerConfiguration({{cross_origin_named_getter}}, {{cross_origin_named_setter}}, nullptr, nullptr, {{cross_origin_named_enumerator}}), v8::IndexedPropertyHandlerConfiguration({{cross_origin_indexed_getter}}), v8::External::New(isolate, const_cast<WrapperTypeInfo*>(&{{v8_class}}::wrapperTypeInfo)));
   {% endif %}
 
   {%- for group in attributes | purely_runtime_enabled_attributes | groupby('runtime_feature_name') %}{{newline}}
@@ -409,8 +507,11 @@ static void install{{v8_class}}Template(v8::Isolate* isolate, const DOMWrapperWo
                     if method.overloads else method.exposed_test) %}
   {% filter runtime_enabled(method.overloads.runtime_enabled_function_all
                             if method.overloads else method.runtime_enabled_function) %}
-  {% if method.is_do_not_check_security %}
-  {{install_do_not_check_security_method(method, '', 'instanceTemplate', 'prototypeTemplate') | indent(2)}}
+  {% if method.is_cross_origin %}
+  {# TODO(dcheng): This shouldn't be necessary with cross-origin interceptors,
+     but v8 doesn't support querying the incumbent context. For now, always
+     incorrectly create per-realm representations. #}
+  {{install_origin_safe_method(method, '', 'instanceTemplate', 'prototypeTemplate') | indent(2)}}
   {% else %}
   {{install_custom_signature(method, 'instanceTemplate', 'prototypeTemplate', 'interfaceTemplate', 'signature') | indent(2)}}
   {% endif %}