[Extensions] More bindings improvements

extensions/renderer/resources/binding.js

 // 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.
 
 var Event = require('event_bindings').Event;
 var forEach = require('utils').forEach;
+// Note: Beware sneaky getters/setters when using GetAvailbility(). Use safe/raw
+// variables as arguments.
 var GetAvailability = requireNative('v8_context').GetAvailability;
 var exceptionHandler = require('uncaught_exception_handler');
 var lastError = require('lastError');
 var logActivity = requireNative('activityLogger');
 var logging = requireNative('logging');
 var process = requireNative('process');
 var schemaRegistry = requireNative('schema_registry');
 var schemaUtils = require('schemaUtils');
 var utils = require('utils');
 var sendRequestHandler = require('sendRequest');
@@ skipping 212 matching lines @@
 
   // Generates the bindings from the schema for |this.apiName_| and integrates
   // any custom bindings that might be present.
   generate: function() {
     // NB: It's important to load the schema during generation rather than
     // setting it beforehand so that we're more confident the schema we're
     // loading is real, and not one that was injected by a page intercepting
     // Binding.generate.
     // Additionally, since the schema is an object returned from a native
     // handler, its properties don't have the custom getters/setters that a page
-    // may have put on Object.prototype.
+    // may have put on Object.prototype, and the object is frozen by v8.
     var schema = schemaRegistry.GetSchema(this.apiName_);
 
     function shouldCheckUnprivileged() {
       var shouldCheck = 'unprivileged' in schema;
       if (shouldCheck)
         return shouldCheck;
 
       $Array.forEach(['functions', 'events'], function(type) {
         if ($Object.hasOwnProperty(schema, type)) {
           $Array.forEach(schema[type], function(node) {
@@ skipping 109 matching lines @@
                           ' already defined in ' + schema.namespace);
         }
 
         if (!isSchemaNodeSupported(functionDef, platform, manifestVersion)) {
           this.apiFunctions_.registerUnavailable(functionDef.name);
           return;
         }
 
         var apiFunction = {};
         apiFunction.definition = functionDef;
-        apiFunction.name = schema.namespace + '.' + functionDef.name;
+        var apiFunctionName = schema.namespace + '.' + functionDef.name;
+        apiFunction.name = apiFunctionName;

[robwu, 2016/04/20 10:49:17]

If you're concerned about being affected by prototype poisoning, use
var apiFunction = $Object.create(null);

Then you'll get an object that doesn't inherit from any object.

[Devlin, 2016/04/20 13:44:38]

This is what https://codereview.chromium.org/1902003003/ does for the Binding
object, which helps.

We need to be careful where we do this because some of these objects are
actually exposed to extension developers, and someone may be relying on object
behavior for the API objects (e.g., perhaps someone checks
chrome.runtime.hasOwnProperty rather than for undefined).

Long-term we should do an audit of pretty much all of these files and make sure
that none are exposing anything they shouldn't, but, well, there's about 129 of
them, so I've been starting with the low-hanging fruit.

[robwu, 2016/04/20 15:06:35]

|apiFunction| is an internal object, so it shouldn't matter. For exposed
objects, it's possible to start with a null prototype and then update the
prototype to $Object.self ($Object.setPrototypeOf(obj, $Object.self); - todo
export setPrototypeOf in safe_builtins.cc).

 
-        if (!GetAvailability(apiFunction.name).is_available ||

[asargent_no_longer_on_chrome, 2016/04/19 23:06:48]

ugh, javascript

I fear we will this mistake again someday soon - it's so easy to forget about
getters/setters

[Devlin, 2016/04/19 23:18:22]

Quite :/  I've been talking with Adam Klein and gotten a few fun tips for how to
help out in some areas.  The problem, of course, is that there are roughly 129
js files that could potentially be attack surfaces.  Obviously, binding.js and
others are juicier, but I'm nervous we'll have issues with others as well.

+        if (!GetAvailability(apiFunctionName).is_available ||
             (checkUnprivileged && !isSchemaAccessAllowed(functionDef))) {
           this.apiFunctions_.registerUnavailable(functionDef.name);
           return;
         }
 
         // TODO(aa): It would be best to run this in a unit test, but in order
         // to do that we would need to better factor this code so that it
         // doesn't depend on so much v8::Extension machinery.
         if (logging.DCHECK_IS_ON() &&
             schemaUtils.isFunctionSignatureAmbiguous(apiFunction.definition)) {
@@ skipping 157 matching lines @@
                     availability.message);
       return;
     }
 
     this.runHooks_(mod, schema);
     return mod;
   }
 };
 
 exports.$set('Binding', Binding);