Assigning to Devlin, as owner of extensions, for a first stab at this.

Does anyone know where the endpoint for ExternalInterface.addCallback is implemented in Chrome? It seems to be a thing that Flash can call to expose things to JS.

I don't think this is really extensions related (beyond their relationship to isolated worlds).  Over to haraken@ for triage, +cc jbroman@ for fyi.

Confirmed, it looks like content::PluginObject gives the same V8 object to all worlds, which enables cross-world access.

This seems kinda bad, because it will break the security model of many extensions?

#2: a combination of HTMLPlugInElement::pluginWrapper and content::PluginObject, IIUC.

[+bbudge for ppapi/plugins]

Yes, under the right circumstances this could enable a page to gain access to content script's privileged JS objects, which breaks the security model.

I think it would require the content script to be tricked into accessing the plugin object (though you might be able to do that by exploiting the implicit window getter behaviour, by giving the embed a suitable id attribute).

The suggested fix of disallowing access to plugin wrappers from isolated worlds would be quick and certainly do the trick, but it would mean that content scripts wouldn't be able to invoke methods of innocuous Flash objects, either. AFAIK we don't currently have metrics of how common that is.

It seems like it would be possible to make this give separate JS object wrappers to each world (which all have their own separate correspondence to the PPAPI interface), but with Flash's days numbered that seems like a lot of work.

@dcheng from memory... Flash implements PPP_Instance_Private, exposing a scriptable object, and PPB_Var_Deprecated to expose methods/fields on it.
IIRC ExternalInterface.addCallback just modifies the internal state of the plugin so that PPB_Var_Deprecated.HasMethod and PPB_Var_Deprecated.Call on the scriptable object behave as if there was such a method on the scriptable object, which is then implemented inside of Flash to call back into ActionScript

So you can look at PepperPluginInstanceImpl::GetInstanceObject for the entrypoint to get access to the Flash object's exposed methods. At the point where the isolated world reaches there, it has access to all of the object's methods, whether the "fixed" API (e.g. objectName.Play(), see also http://www.permadi.com/tutorial/flashjscommand/) or the callbacks registered by the ActionScript via ExternalInterface. So maybe we need to prevent that from happening?

@#7: I don't think there's any way to distinguish objectName.Play (fixed JS API) vs objectName.someExternalCallback (registered from ExternalInterface) from the Chrome POV, so unless we encode special knowledge of the Flash JS API, there's no way to block callbacks without also blocking the plugin API.

I believe that in Flash, callbacks can only take strings and return strings, but we don't enforce that in Chrome. Not sure if that helps.

#10: The question then becomes, is it really okay to break use of the (entire) Flash JS API from content scripts, or do we need to bite the bullet and have someone actually fix this?

Thankfully the PPAPI vars seem to be such that this is tractable. But it's not a small change.

@#10: Actually you can return arbitrary (presumably serialize?) objects from ExternalInterface.addCallback methods. See the actionscript and HTML code in https://fromwhenceitca.me/ExternalInterfaceComplex/

Hmm. I missed one in the list of PP_Var types: it does seem that it's possible for a plugin to take PP_VARTYPE_OBJECT (not sure about PP_VARTYPE_RESOURCE), which we wouldn't be able to store in a base::Value or similar.

And...there's a comment in V8VarConverter that suggests Flash may rely on this. :/

@#13: yes, PP_Var can expose references to objects, and Flash relies on this
@#12: you're right... it's been a long time. Would need to dig into the Flash code to check what is and what is not possible. I'm 99% sure it can't expose active objects, only data types (arrays/dictionaries, not functions)

@#14 Yeah, when I tested it I couldn't naively get a function to be returned.

Tentatively setting security impact labels. Please follow up on whether this requires action asap.

haraken: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

piman@: Thanks for the digging. This seems like an issue of plugin but would you mind taking ownership of this?

Friendly ping. :) We need to get this high severity issue fixed. piman, are you the right person?

I was OOO when assigned the bug. I'm not the right owner...
I don't think there's anything that the plugin can do to fix this, and I'm not working on Flash anyway.

My suggestion would be to disallow plugin access from content scripts as a (heavy handed) mitigation. I have no idea what's involved in that. ->haraken who knows about bindings (or who could be working on that).

Does anyone know what Mozilla does (whitelisting API? disallow plugin access altogether? Something else?)?

Devlin, any comment on the possibility of disallowing plugin access from content scripts?

There appear to be at least a few extensions with >10,000 users that have calls to ExternalInterface in their scripts.  Unfortunately, without taking a much deeper dive, it's difficult to tell whether or not these are necessary (e.g., some seem to be included as part of a library).  I think to know for sure one way or the other we'd need at least some UMA for this.  If the UMA is compelling enough, I wouldn't entirely against this, since we're largely trying to kill these plugins anyway.  Though a part of me is worried that then websites will start using these to serve ads since extensions couldn't block them...*shudder*.

Do network requests made by plugins get exposed through webRequest API? If so, then blocking them at the network layer should be a good alternative, no?

haraken: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

#27: I don't think this has anything to do with network requests. The basic issue is that both content scripts and ordinary scripts can interact with the plugin, and the way that interaction works allows objects from one to leak to the other. Under the right circumstances, this could allow the ordinary script to escalate to content script privilege.

Unfortunately, the code in question doesn't seem to have an active owner, Flash is on the way out, and there doesn't seem to be a simple fix that doesn't risk breaking extensions which legitimately interact with plugins.

nasko@: Would you find an owner in the security team?

Outside of dcheng@, I don't know who else on the security team might be able to look at this. I don't want to speak for dcheng@, so I'll let him take it if there is possibility.

I thought we'd deserialize the PPVar's into fresh JS values of the requesting context every time, so the content script would get back values from its own context.

Or am I misunderstanding the bug report?

I instrumented the interceptor in blink like this:

diff --git a/third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp b/third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp
index b4a8f8c2f0d7..f08aadcb5afb 100644
--- a/third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp
@@ -61,6 +61,18 @@ void GetScriptableObjectProperty(
            .ToLocal(&value))
     return;
 
+  fprintf(stderr, "property %s\n", name.Utf8().data());
+  fprintf(stderr, "current world id %d\n",
+          ScriptState::Current(info.GetIsolate())->World().GetWorldId());
+  if (value->IsObject()) {
+    fprintf(
+        stderr, "returning world id %d\n",
+        ScriptState::From(v8::Local<v8::Object>::Cast(value)->CreationContext())
+            ->World().GetWorldId());
+  } else {
+    fprintf(stderr, "returning primitive value\n");
+  }
+
   V8SetReturnValue(info, value);
 }
 

and the output from the test is like this:

property someExternalMethod
current world id 0
returning world id 0
property someExternalMethod
current world id 4
returning world id 4

so I lean towards WontFix?

ok, Jeremy shared his repro steps. I guess I can work with that :)

> I thought we'd deserialize the PPVar's into fresh JS values of the requesting context every time, so the content script would get back values from its own context.

I'm not sure that this would be sufficient. An interesting quirk here is that when you call into a Flash method from the content script, if it calls out again to Javascript via ExternalInterface.call, it is in the page's world, not the content scripts. I'm not really sure of the security implications of this - I *think* that there isn't a way exposed to walk the stack and get references to the functions in the content script world from the page world, but it would probably be worth checking out. (arguments.callee.caller presumably doesn't work  across the plugin stack frames...?)

It might be worthwhile to get Tavis to weigh in on the implications of this and the suggested WontFix.

let's first add a use counter and see whether we can just block access from isolated worlds

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

commit 46f7fde1afa674e21a11e7b10b207fa8db09e115
Author: Jochen Eisinger <jochen@chromium.org>
Date: Wed May 24 09:36:33 2017

Add use counters for plugin accesses

BUG= 708237 
R=haraken@chromium.org

Change-Id: Idf89cfe6917e43ba117d65752ff78bc9a89dc86a
Reviewed-on: https://chromium-review.googlesource.com/513902
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#474224}
[modify] https://crrev.com/46f7fde1afa674e21a11e7b10b207fa8db09e115/third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp
[modify] https://crrev.com/46f7fde1afa674e21a11e7b10b207fa8db09e115/third_party/WebKit/Source/core/frame/UseCounter.h
[modify] https://crrev.com/46f7fde1afa674e21a11e7b10b207fa8db09e115/tools/metrics/histograms/enums.xml

On canary, it looks like we see access to the plugin instance from the main work on about 0.03% of pageloads (176,053 unique page loads), and 0% from isolated worlds (667 instances).

Devlin, I think the case can be made that we should just block access. If this really breaks an extension, it can just inject script into the main world (sic). wdyt?

+1; if there's no significant usage of this I'm fine with (and would encourage) removing support.

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

commit 1e196e33687bd22a33c8b2116956754b90b29bde
Author: Jochen Eisinger <jochen@chromium.org>
Date: Wed May 31 11:07:15 2017

Disallow access to plugin instances from non-main world

The entire system is not setup to handle multiple worlds and UMA
suggests that it's also hardly used. So instead of teaching the plugin
system about multiple world, restrict access to the main world

BUG= 708237 
R=haraken@chromium.org

Change-Id: I3fed56c149b2d70593bedb99dff1664a819b3ae4
Reviewed-on: https://chromium-review.googlesource.com/518902
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#475874}
[modify] https://crrev.com/1e196e33687bd22a33c8b2116956754b90b29bde/third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLPlugInElementCustom.cpp

I'd claim that is fixed now

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot