New issue
Advanced search Search tips

Issue 792602 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Extension messaging bindings needlessly and ineffectively avoid using JSON builtins

Project Member Reported by rdevlin....@chromium.org, Dec 6 2017

Issue description

Extension messaging bindings avoid using Object.prototype.toJSON and Array.prototype.toJSON by using [1] the safe-builtins version of JSON.stringify() [2].  This is done in order to prevent script from mucking with the serialization by having custom toJSON methods.  However, this is totally ineffective: though it protects against Object.prototype.toJSON manipulation, script could still easy pass an object with a toJSON property as a message, like the following:

chrome.runtime.sendMessage({foo: 'bar', toJSON: function() { return '5'}});

In this case, the message received will be '5', rather than {foo: bar}.  Because of this, any script that wanted to manipulate the serialization process can do so quite trivially.

In the end, the avoidance of using the [Array|Object].prototype.toJSON methods really only serves to potentially cause confusion - we respect some toJSON implementations, but not others.  What's more, there might be potential reasons to use a script-provided toJSON implementation (if, for instance, script wanted to provide its own serialization for functions or special objects).

Now, onto the code history.  This was originally added in revision 200dcd387d6902556e41e9439ce64f7f219f5809 (back in 2010) in response to extensions clobbering the JSON.stringify implementation that extensions bindings code used for sending *all* arguments to the browser (as well as parsing function schemas).  This would absolutely cause a problem, but is unrelated to JSON message parsing.  The message sender and receiver already have the ability to send arbitrary values and mutate and received values, so we aren't protecting against anything here.

Motivation: this would be painful to implement in native bindings, not to mention being unnecessary and less efficient.

Proposal: in native bindings, respect toJSON behavior.  In the (hopefully unlikely) event that any extensions are relying on this, this will provide a slow roll out.

There's not much action needed here; this bug is mostly just for tracking, historical, and/or discussion purposes.

[1] https://chromium.googlesource.com/chromium/src/+/1c3de3712cc2d6704930681cc29a17ab89490445/extensions/renderer/resources/messaging.js#107
[2] https://chromium.googlesource.com/chromium/src/+/1c3de3712cc2d6704930681cc29a17ab89490445/extensions/renderer/safe_builtins.cc#103
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 31 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b15f7f04de19b4e835f7be615a1d488876f8d086

commit b15f7f04de19b4e835f7be615a1d488876f8d086
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Wed Jan 31 19:37:32 2018

[Extensions Bindings] Finish messaging-related implementations

Add the remaining functionality for messaging-related bindings to the
native bindings implementaiton. This includes odds and ends for the
extension and runtime APIs, such as getBackgroundPage() or
inIncognitoContext().

Also update tests to accept either outcome when there are different
behaviors for native and JS-based bindings. This happens significantly
in the following areas:
- We don't clobber JSON methods with native bindings, because it's
  unhelpful and would be painful to accommodate. See also
  https://crbug.com/792602.
- Native bindings throw an error with unserializable messages, whereas
  JS bindings only log an error (which seems less helpful).
- Native bindings validate that an extension ID is valid when it is
  provided for message passing, rather than just verifying it is a
  string.
- Calling chrome.runtime.sendMessage no longer implicitly calls
  chrome.runtime.connect with native bindings, which adjusts our
  expectations in activity logging (to be more accurate).

Also update WebViewInteractiveTest.TextSelection test to address a race
condition where a test body could finish before a context menu was properly
dismissed, resulting in a timeout.

Additionally, add a nativeCrxBindingsEnabled flag to the test config
exposed to extension tests so they can detect which expectation to use.

Finally, hook up native bindings with messaging so that they are used in
production (if and only if native bindings are enabled on the whole). This
allows us to avoid registering JS bindings for 'messaging',
'messaging_utils', 'runtime', 'extension', and 'tabs'.

Bug:  653596 
Bug: 792602

Change-Id: If09f607fe5c9f30578f84248173848b30c7bbf21
Reviewed-on: https://chromium-review.googlesource.com/860755
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533360}
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/chrome/browser/extensions/extension_apitest.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/chrome/browser/extensions/extension_messages_apitest.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.h
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/chrome/renderer/extensions/extension_hooks_delegate.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/chrome/renderer/extensions/extension_hooks_delegate.h
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/chrome/test/data/extensions/api_test/activity_log_private/test/test.js
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/chrome/test/data/extensions/api_test/messaging/connect/page.js
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/chrome/test/data/extensions/api_test/messaging/connect/test.js
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/chrome/test/data/extensions/api_test/messaging/large_messages/background.js
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/common/api/test.json
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/api_activity_logger.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/api_activity_logger.h
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/bindings/api_event_handler.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/bindings/event_emitter.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/bindings/test_js_runner.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/dispatcher.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/dispatcher_delegate.h
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/extension_frame_helper.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/extension_frame_helper.h
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/gin_port.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/ipc_message_sender.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/messaging_util.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/native_extension_bindings_system.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/native_extension_bindings_system.h
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/native_renderer_messaging_service.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/one_time_message_handler.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/one_time_message_handler_unittest.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/runtime_hooks_delegate.cc
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/runtime_hooks_delegate.h
[modify] https://crrev.com/b15f7f04de19b4e835f7be615a1d488876f8d086/extensions/renderer/runtime_hooks_delegate_unittest.cc

Sign in to add a comment