Respect the clipboardRead and clipboardWrite permissions in content scripts.

Respect the clipboardRead and clipboardWrite permissions in content scripts.

Added an extra "effective extension" property to ScriptContext for this to still work correctly in about:blank iframes inside extension pages.

BUG=395376
Committed: https://crrev.com/7e1d745456f66b1d32df236e285a849c8d69db00
Cr-Commit-Position: refs/heads/master@{#293818}

[Marijn Kruisselbrink, 2014-09-03 19:32:29]

mek@chromium.org changed reviewers:
+ dcheng@chromium.org, rdevlin.cronin@chromium.org

[Marijn Kruisselbrink, 2014-09-03 19:32:29]

This is a fixed version of what kalman@ started in
https://codereview.chromium.org/475883003/. The original CL had the problem that
it broke clipboardRead and clipboardWrite in about:blank iframes opened by
extension pages, this fixes that by adding an additional effective_extension to
the extensions::ScriptContext and using that extension to check for copy/paste
permissions.

Unfortunately all the reviewers of the original CL are OOO...
Devlin: can you take a look at the extensions side of things
Daniel: you're shown as OOO, but adding you anyway just in case

[Devlin, 2014-09-03 22:09:17]

https://codereview.chromium.org/498513002/diff/80001/chrome/browser/renderer_...
File chrome/browser/renderer_host/chrome_extension_message_filter.cc (right):

https://codereview.chromium.org/498513002/diff/80001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_extension_message_filter.cc:26: #include
"extensions/common/constants.h"
It also _looks_ like you don't need this (hard to say for sure, since constants
has so much random stuff).

https://codereview.chromium.org/498513002/diff/80001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_extension_message_filter.cc:32: using
extensions::APIPermission;
nit: Remove this now.

https://codereview.chromium.org/498513002/diff/80001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/498513002/diff/80001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:416: // *safer* but made it
impossible to take into account content script
I'm not familiar enough with this security to make a good judgment, but it seems
like if it was safer to check browser-side, we'd be able to just pass an
extension id and then compare the permissions there, too, right?

https://codereview.chromium.org/498513002/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/clipboard/extension/test.js (right):

https://codereview.chromium.org/498513002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/clipboard/extension/test.js:10: var pass =
chrome.test.callbackPass;
for the one time we use this, I'd just inline it.  Or at least put it closer.

https://codereview.chromium.org/498513002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/clipboard/extension/test.js:58:
chrome.tabs.executeScript(tabId, {file: 'content_script.js'});
hmm...

Saying this injects from a content script, and then injecting it via
executeScript is a little misleading. They have different permissions and
(slightly) different injection methods/times.  I'd say we should rename this to
be more clear that it's not actually a content script.

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/disp...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/disp...
extensions/renderer/dispatcher.cc:252: const Extension* extension =
extensions_.GetByID(extension_id);
This is unnecessarily complicated.  We should just make it GetExtensionFromWorld
(or some other name) and have it return the extension
(ExtensionSet::GetExtensionOrAppIDByURL already looks up the full extension
anyway).

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/disp...
extensions/renderer/dispatcher.cc:268: const Extension* effective_extension =
extensions_.GetByID(extension_id);
If I recall correctly, GetEffectiveDocumentURL() only changes from frame_url if
it's a non-sandboxed about:blank. If this is the case, then |extension| on line
252 is almost certainly NULL, right?  (No extension will have an associated url
of about:blank.)  So if that's the case, do we really need both
|effective_extension| and |extension| in the ScriptContext, or could we just
make |extension| the effective extension?  Would that break things elsewhere?

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/scri...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/scri...
extensions/renderer/script_context.cc:149: std::string
ScriptContext::GetContextTypeDescription(
Unless this is used elsewhere, I'd probably move it to an anonymous namespace to
reduce bloat in the header.

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/scri...
File extensions/renderer/script_context.h (right):

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/scri...
extensions/renderer/script_context.h:84: // empty string if there is no such
extension.
Are all  of these accessors really needed?  If not, I'd be in favor of taking
out the ones like this (where it's perfectly easy to instead use
effective_extension()->id()).

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/scri...
extensions/renderer/script_context.h:111: // Returns a string description of the
type of context this is.
nit: "...of the |context_type|."

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/scri...
extensions/renderer/script_context.h:117: // Returns a string description of the
type of context this is.
... of the effective type of context...

[Marijn Kruisselbrink, 2014-09-03 23:54:06]

mek@chromium.org changed reviewers:
+ rockot@chromium.org

[Marijn Kruisselbrink, 2014-09-03 23:54:07]

PTAL
and adding rockot@ just in case he has an opinion on this as well, since I just
noticed kalman@ suggested he would make for a good reviewer for this CL...

https://codereview.chromium.org/498513002/diff/80001/chrome/browser/renderer_...
File chrome/browser/renderer_host/chrome_extension_message_filter.cc (right):

https://codereview.chromium.org/498513002/diff/80001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_extension_message_filter.cc:26: #include
"extensions/common/constants.h"
On 2014/09/03 22:09:16, Devlin wrote:
> It also _looks_ like you don't need this (hard to say for sure, since
constants
> has so much random stuff).

Done.

https://codereview.chromium.org/498513002/diff/80001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_extension_message_filter.cc:32: using
extensions::APIPermission;
On 2014/09/03 22:09:16, Devlin wrote:
> nit: Remove this now.

Done.

https://codereview.chromium.org/498513002/diff/80001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/498513002/diff/80001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:416: // *safer* but made it
impossible to take into account content script
On 2014/09/03 22:09:16, Devlin wrote:
> I'm not familiar enough with this security to make a good judgment, but it
seems
> like if it was safer to check browser-side, we'd be able to just pass an
> extension id and then compare the permissions there, too, right?
Quoting from a comment about this in kalman's CL:
"I think this is fine. Ultimately, this permission is enforced in the renderer
anyway, so sending it to the browser is pointless."

I've removed this comment.

https://codereview.chromium.org/498513002/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/clipboard/extension/test.js (right):

https://codereview.chromium.org/498513002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/clipboard/extension/test.js:10: var pass =
chrome.test.callbackPass;
On 2014/09/03 22:09:16, Devlin wrote:
> for the one time we use this, I'd just inline it.  Or at least put it closer.

Done.

https://codereview.chromium.org/498513002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/clipboard/extension/test.js:58:
chrome.tabs.executeScript(tabId, {file: 'content_script.js'});
On 2014/09/03 22:09:16, Devlin wrote:
> hmm...
> 
> Saying this injects from a content script, and then injecting it via
> executeScript is a little misleading. They have different permissions and
> (slightly) different injection methods/times.  I'd say we should rename this
to
> be more clear that it's not actually a content script.
I renamed the test/methods. But that makes it sounds like ideally we would also
have a test that actually involves a content script?

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/disp...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/disp...
extensions/renderer/dispatcher.cc:252: const Extension* extension =
extensions_.GetByID(extension_id);
On 2014/09/03 22:09:16, Devlin wrote:
> This is unnecessarily complicated.  We should just make it
GetExtensionFromWorld
> (or some other name) and have it return the extension
> (ExtensionSet::GetExtensionOrAppIDByURL already looks up the full extension
> anyway).
Done, although I'm not entirely sure if it really was unnecessarily? I'm not
sure under which conditions "despite a context being associated with an
extension, no extension actually gets found", but by using
GetExtensionOrAppByURL instead of GetExtensionOrAppIDByURL followed by looking
up the extension by ID, I've eliminated at least one code paths that could
before have resulted in this LOG(ERROR). (I could of course change
GetExtensionFromFrameAndWorld back to first get the ID, but that doesn't really
seem much of an improvement compared to just leaving the code as it is).
So I'm tempted to undo the renaming/changing to GetExtensionFromFrameAndWorld
and instead go back to GetExtensionID

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/disp...
extensions/renderer/dispatcher.cc:268: const Extension* effective_extension =
extensions_.GetByID(extension_id);
On 2014/09/03 22:09:16, Devlin wrote:
> If I recall correctly, GetEffectiveDocumentURL() only changes from frame_url
if
> it's a non-sandboxed about:blank. If this is the case, then |extension| on
line
> 252 is almost certainly NULL, right?  (No extension will have an associated
url
> of about:blank.)  So if that's the case, do we really need both
> |effective_extension| and |extension| in the ScriptContext, or could we just
> make |extension| the effective extension?  Would that break things elsewhere?
Yes, the ideal end state is to get rid of extension and only have
effective_extension. But, quoting kalman:
"Just changing that will change everything (the "ideal" case as I put it in the
bug). But I'm scared, and we'd need to do an audit of everywhere we use [it].

The hackier/conservative fix would be to store 2 Extensions in ScriptContext,
one derived from GetDataSourceURLForFrame and the other from
GetEffectiveDocumentURL, then use the latter in a CanClipboardRead/Write
method:"

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/scri...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/scri...
extensions/renderer/script_context.cc:149: std::string
ScriptContext::GetContextTypeDescription(
On 2014/09/03 22:09:16, Devlin wrote:
> Unless this is used elsewhere, I'd probably move it to an anonymous namespace
to
> reduce bloat in the header.

Done.

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/scri...
File extensions/renderer/script_context.h (right):

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/scri...
extensions/renderer/script_context.h:84: // empty string if there is no such
extension.
On 2014/09/03 22:09:16, Devlin wrote:
> Are all  of these accessors really needed?  If not, I'd be in favor of taking
> out the ones like this (where it's perfectly easy to instead use
> effective_extension()->id()).
Well, it's the "or empty string" part that makes it not quite that easy. It's
replacement would be |effective_extension() ? effective_extension()->id() : ""|
or something similar. But either way this is only used in debug logs, so other
than consistency there really isn't a reason to add this accessor. I removed it
(and added a helper method to get the ID for a possibly null extension to an
anonymous namespace).

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/scri...
extensions/renderer/script_context.h:111: // Returns a string description of the
type of context this is.
On 2014/09/03 22:09:16, Devlin wrote:
> nit: "...of the |context_type|."

Acknowledged.

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/scri...
extensions/renderer/script_context.h:117: // Returns a string description of the
type of context this is.
On 2014/09/03 22:09:16, Devlin wrote:
> ... of the effective type of context...

Done.

[Devlin, 2014-09-04 19:15:11]

lgtm with nits

https://codereview.chromium.org/498513002/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/clipboard/extension/test.js (right):

https://codereview.chromium.org/498513002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/clipboard/extension/test.js:58:
chrome.tabs.executeScript(tabId, {file: 'content_script.js'});
On 2014/09/03 23:54:07, Marijn Kruisselbrink wrote:
> On 2014/09/03 22:09:16, Devlin wrote:
> > hmm...
> > 
> > Saying this injects from a content script, and then injecting it via
> > executeScript is a little misleading. They have different permissions and
> > (slightly) different injection methods/times.  I'd say we should rename this
> to
> > be more clear that it's not actually a content script.
> I renamed the test/methods. But that makes it sounds like ideally we would
also
> have a test that actually involves a content script?

Ideally, yes, I'd say having a content script test would be good (since those
were most specifically mentioned in the bug).

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/disp...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/disp...
extensions/renderer/dispatcher.cc:252: const Extension* extension =
extensions_.GetByID(extension_id);
On 2014/09/03 23:54:07, Marijn Kruisselbrink wrote:
> On 2014/09/03 22:09:16, Devlin wrote:
> > This is unnecessarily complicated.  We should just make it
> GetExtensionFromWorld
> > (or some other name) and have it return the extension
> > (ExtensionSet::GetExtensionOrAppIDByURL already looks up the full extension
> > anyway).
> Done, although I'm not entirely sure if it really was unnecessarily? I'm not
> sure under which conditions "despite a context being associated with an
> extension, no extension actually gets found", but by using
> GetExtensionOrAppByURL instead of GetExtensionOrAppIDByURL followed by looking
> up the extension by ID, I've eliminated at least one code paths that could
> before have resulted in this LOG(ERROR). (I could of course change
> GetExtensionFromFrameAndWorld back to first get the ID, but that doesn't
really
> seem much of an improvement compared to just leaving the code as it is).
> So I'm tempted to undo the renaming/changing to GetExtensionFromFrameAndWorld
> and instead go back to GetExtensionID

Good point, so I don't really feel too strongly either way.

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/disp...
extensions/renderer/dispatcher.cc:268: const Extension* effective_extension =
extensions_.GetByID(extension_id);
On 2014/09/03 23:54:07, Marijn Kruisselbrink wrote:
> On 2014/09/03 22:09:16, Devlin wrote:
> > If I recall correctly, GetEffectiveDocumentURL() only changes from frame_url
> if
> > it's a non-sandboxed about:blank. If this is the case, then |extension| on
> line
> > 252 is almost certainly NULL, right?  (No extension will have an associated
> url
> > of about:blank.)  So if that's the case, do we really need both
> > |effective_extension| and |extension| in the ScriptContext, or could we just
> > make |extension| the effective extension?  Would that break things
elsewhere?
> Yes, the ideal end state is to get rid of extension and only have
> effective_extension. But, quoting kalman:
> "Just changing that will change everything (the "ideal" case as I put it in
the
> bug). But I'm scared, and we'd need to do an audit of everywhere we use [it].
> 
> The hackier/conservative fix would be to store 2 Extensions in ScriptContext,
> one derived from GetDataSourceURLForFrame and the other from
> GetEffectiveDocumentURL, then use the latter in a CanClipboardRead/Write
> method:"

Ah.  That's too bad.  Okay.

https://codereview.chromium.org/498513002/diff/100001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/498513002/diff/100001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:431: // See comment in
allowReadFromClipboard.
There's no comment there anymore. :)

https://codereview.chromium.org/498513002/diff/100001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:436: if (calling_context) {
nit: I'd maybe consolidate this and the above a bit to improve
readability/changability.  Something like

bool CallingExtensionHasPermission(ScriptContext* calling_context,
                                   APIPermission permission) {
  const Extension* extension =
      calling_context ? calling_context->effective_extension() : NULL;
  return extension &&
         extension->permissions_data()->HasAPIPermission(permission)
}

But I don't feel strongly about it, so up to you.

https://codereview.chromium.org/498513002/diff/100001/extensions/renderer/scr...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/498513002/diff/100001/extensions/renderer/scr...
extensions/renderer/script_context.cc:59: return base::EmptyString();
base::EmptyString is expensive.  It'd probably be better to just return a
std::string(), or inline this into the logs (I don't think extension_ ?
extension_->id() : "" is that long/unreadable - but that's just me).

[Marijn Kruisselbrink, 2014-09-04 23:41:11]

https://codereview.chromium.org/498513002/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/clipboard/extension/test.js (right):

https://codereview.chromium.org/498513002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/clipboard/extension/test.js:58:
chrome.tabs.executeScript(tabId, {file: 'content_script.js'});
On 2014/09/04 19:15:10, Devlin wrote:
> On 2014/09/03 23:54:07, Marijn Kruisselbrink wrote:
> > On 2014/09/03 22:09:16, Devlin wrote:
> > > hmm...
> > > 
> > > Saying this injects from a content script, and then injecting it via
> > > executeScript is a little misleading. They have different permissions and
> > > (slightly) different injection methods/times.  I'd say we should rename
this
> > to
> > > be more clear that it's not actually a content script.
> > I renamed the test/methods. But that makes it sounds like ideally we would
> also
> > have a test that actually involves a content script?
> 
> Ideally, yes, I'd say having a content script test would be good (since those
> were most specifically mentioned in the bug).
Okay, I added tests for content scripts as well.

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/disp...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/498513002/diff/80001/extensions/renderer/disp...
extensions/renderer/dispatcher.cc:252: const Extension* extension =
extensions_.GetByID(extension_id);
On 2014/09/04 19:15:10, Devlin wrote:
> On 2014/09/03 23:54:07, Marijn Kruisselbrink wrote:
> > On 2014/09/03 22:09:16, Devlin wrote:
> > > This is unnecessarily complicated.  We should just make it
> > GetExtensionFromWorld
> > > (or some other name) and have it return the extension
> > > (ExtensionSet::GetExtensionOrAppIDByURL already looks up the full
extension
> > > anyway).
> > Done, although I'm not entirely sure if it really was unnecessarily? I'm not
> > sure under which conditions "despite a context being associated with an
> > extension, no extension actually gets found", but by using
> > GetExtensionOrAppByURL instead of GetExtensionOrAppIDByURL followed by
looking
> > up the extension by ID, I've eliminated at least one code paths that could
> > before have resulted in this LOG(ERROR). (I could of course change
> > GetExtensionFromFrameAndWorld back to first get the ID, but that doesn't
> really
> > seem much of an improvement compared to just leaving the code as it is).
> > So I'm tempted to undo the renaming/changing to
GetExtensionFromFrameAndWorld
> > and instead go back to GetExtensionID
> 
> Good point, so I don't really feel too strongly either way.
Actually I kind of like having this GetExtensionFromFrameAndWorld method; I now
slightly rewrote it to keep the old behavior by first looking up the extension
id.

https://codereview.chromium.org/498513002/diff/100001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/498513002/diff/100001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:431: // See comment in
allowReadFromClipboard.
On 2014/09/04 19:15:11, Devlin wrote:
> There's no comment there anymore. :)

Done.

https://codereview.chromium.org/498513002/diff/100001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:436: if (calling_context) {
On 2014/09/04 19:15:11, Devlin wrote:
> nit: I'd maybe consolidate this and the above a bit to improve
> readability/changability.  Something like
> 
> bool CallingExtensionHasPermission(ScriptContext* calling_context,
>                                    APIPermission permission) {
>   const Extension* extension =
>       calling_context ? calling_context->effective_extension() : NULL;
>   return extension &&
>          extension->permissions_data()->HasAPIPermission(permission)
> }
> 
> But I don't feel strongly about it, so up to you.
It would maybe be nice to have this as a ScriptContext::HasAPIPermission method,
but that might get a bit ugly with the separate extension() and
effective_extension() bits. It would either need to hardcode which one to use
for which permission, or have some kind of flag/separate method for either one,
and neither of those options seem particularly appealing. So I think for now
I'll just leave it as is.

https://codereview.chromium.org/498513002/diff/100001/extensions/renderer/scr...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/498513002/diff/100001/extensions/renderer/scr...
extensions/renderer/script_context.cc:59: return base::EmptyString();
On 2014/09/04 19:15:11, Devlin wrote:
> base::EmptyString is expensive.  It'd probably be better to just return a
> std::string(), or inline this into the logs (I don't think extension_ ?
> extension_->id() : "" is that long/unreadable - but that's just me).
Good point, done.

[Marijn Kruisselbrink, 2014-09-04 23:50:12]

mek@chromium.org changed reviewers:
+ jln@chromium.org, thestig@chromium.org

[Marijn Kruisselbrink, 2014-09-04 23:50:13]

adding OWNERS reviewers:
+thestig for chrome/browser/renderer_host/chrome_extension_message_filter.* and
chrome/renderer/content_settings_observer.*
+jln for chrome/common/extensions/chrome_extension_messages.h

[Lei Zhang, 2014-09-05 00:03:10]

lgtm

[Ken Rockot(use gerrit already), 2014-09-05 00:11:12]

lgtm as well

[jln (very slow on Chromium), 2014-09-08 19:09:09]

chrome/common/extensions/chrome_extension_messages.h lgtm

[Marijn Kruisselbrink, 2014-09-08 19:15:10]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-09-08 19:23:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mek@chromium.org/498513002/120001

[commit-bot: I haz the power, 2014-09-09 00:27:25]

Committed patchset #6 (id:120001) as 8c040ea2a6b36cd0d9719370c30c1daacd4b4879

[commit-bot: I haz the power, 2014-09-10 03:49:09]

Patchset 6 (id:??) landed as
https://crrev.com/7e1d745456f66b1d32df236e285a849c8d69db00
Cr-Commit-Position: refs/heads/master@{#293818}