[Extensions] Don't allow extensions to inject scripts into extension pages

[Extensions] Don't allow extensions to inject scripts into extension pages

Don't allow extensions to inject scripts into other extension pages, since this
is a security risk. This was meant to be addressed, but there was an incorrect
early-return. Also add a regression test.

BUG=529682
Committed: https://crrev.com/c318b93de2ee7b8cc78e506dd2dd161af7d6819d
Cr-Commit-Position: refs/heads/master@{#348707}

[Devlin, 2015-09-11 22:00:53]

rdevlin.cronin@chromium.org changed reviewers:
+ kalman@chromium.org

[Devlin, 2015-09-11 22:00:53]

fix + test

[not at google - send to devlin, 2015-09-11 22:46:39]

https://codereview.chromium.org/1335083004/diff/1/extensions/renderer/extensi...
File extensions/renderer/extension_injection_host.cc (right):

https://codereview.chromium.org/1335083004/diff/1/extensions/renderer/extensi...
extensions/renderer/extension_injection_host.cc:83: // outside of tabs because
there is nowhere to surface a request.
If you made the RenderFrameHost change I mentioned, you wouldn't need this logic
on the renderer anymore.

https://codereview.chromium.org/1335083004/diff/1/extensions/renderer/script_...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/1335083004/diff/1/extensions/renderer/script_...
extensions/renderer/script_injection.cc:143: SendInjectionMessage(true /*
request permission */);
This change means that you're always going to be sending this IPC, which is
wasteful given you're not even using its result.

On the other hand, you *could* actually take this opportunity to move these
checks into the browser (ActiveScriptController) to make it use RenderFrameHosts
rather than the WebContents.

Unfortunately that would commit to having these IPCs flying around and we might
want to wait until we can optimise only sending 1 IPC per origin rather than per
frame. You will have a better sense than me for whether taking the simpler
always-send-IPC approach is acceptable.

[Devlin, 2015-09-11 22:55:02]

https://codereview.chromium.org/1335083004/diff/1/extensions/renderer/script_...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/1335083004/diff/1/extensions/renderer/script_...
extensions/renderer/script_injection.cc:143: SendInjectionMessage(true /*
request permission */);
On 2015/09/11 22:46:39, kalman wrote:
> This change means that you're always going to be sending this IPC, which is
> wasteful given you're not even using its result.

Nope, because the logic to choose allowed or not allowed for a case of having no
UI surface is handled in the injection host now.  It will just return
ACCESS_DENIED here, so won't send any IPC.

> On the other hand, you *could* actually take this opportunity to move these
> checks into the browser (ActiveScriptController) to make it use
RenderFrameHosts
> rather than the WebContents.
> 
> Unfortunately that would commit to having these IPCs flying around and we
might
> want to wait until we can optimise only sending 1 IPC per origin rather than
per
> frame. You will have a better sense than me for whether taking the simpler
> always-send-IPC approach is acceptable.

Doesn't that also break things like doc-start completely?

[not at google - send to devlin, 2015-09-11 23:09:49]

lgtm

https://codereview.chromium.org/1335083004/diff/1/extensions/renderer/script_...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/1335083004/diff/1/extensions/renderer/script_...
extensions/renderer/script_injection.cc:143: SendInjectionMessage(true /*
request permission */);
On 2015/09/11 22:55:02, Devlin wrote:
> On 2015/09/11 22:46:39, kalman wrote:
> > This change means that you're always going to be sending this IPC, which is
> > wasteful given you're not even using its result.
> 
> Nope, because the logic to choose allowed or not allowed for a case of having
no
> UI surface is handled in the injection host now.  It will just return
> ACCESS_DENIED here, so won't send any IPC.

I see. I read these methods in reverse.

> 
> > On the other hand, you *could* actually take this opportunity to move these
> > checks into the browser (ActiveScriptController) to make it use
> RenderFrameHosts
> > rather than the WebContents.
> > 
> > Unfortunately that would commit to having these IPCs flying around and we
> might
> > want to wait until we can optimise only sending 1 IPC per origin rather than
> per
> > frame. You will have a better sense than me for whether taking the simpler
> > always-send-IPC approach is acceptable.
> 
> Doesn't that also break things like doc-start completely?

Only change for withheld, not for everything, otherwise yes it would break
document_start all of the time.

Basically you'd remove the "web_frame->parent()" checks here - which is only hit
in the WITHHELD case - and instead do the check on the browser inside the
SendInjectionMessage handler (which would mean dealing with RenderFrameHost
etc).

as I said you may want to think about avoiding a flood of IPCs on every page
load with iframes, though.

[Devlin, 2015-09-11 23:11:02]

On 2015/09/11 23:09:49, kalman wrote:
> lgtm
> 
>
https://codereview.chromium.org/1335083004/diff/1/extensions/renderer/script_...
> File extensions/renderer/script_injection.cc (right):
> 
>
https://codereview.chromium.org/1335083004/diff/1/extensions/renderer/script_...
> extensions/renderer/script_injection.cc:143: SendInjectionMessage(true /*
> request permission */);
> On 2015/09/11 22:55:02, Devlin wrote:
> > On 2015/09/11 22:46:39, kalman wrote:
> > > This change means that you're always going to be sending this IPC, which
is
> > > wasteful given you're not even using its result.
> > 
> > Nope, because the logic to choose allowed or not allowed for a case of
having
> no
> > UI surface is handled in the injection host now.  It will just return
> > ACCESS_DENIED here, so won't send any IPC.
> 
> I see. I read these methods in reverse.
> 
> > 
> > > On the other hand, you *could* actually take this opportunity to move
these
> > > checks into the browser (ActiveScriptController) to make it use
> > RenderFrameHosts
> > > rather than the WebContents.
> > > 
> > > Unfortunately that would commit to having these IPCs flying around and we
> > might
> > > want to wait until we can optimise only sending 1 IPC per origin rather
than
> > per
> > > frame. You will have a better sense than me for whether taking the simpler
> > > always-send-IPC approach is acceptable.
> > 
> > Doesn't that also break things like doc-start completely?
> 
> Only change for withheld, not for everything, otherwise yes it would break
> document_start all of the time.
> 
> Basically you'd remove the "web_frame->parent()" checks here - which is only
hit
> in the WITHHELD case - and instead do the check on the browser inside the
> SendInjectionMessage handler (which would mean dealing with RenderFrameHost
> etc).
> 
> as I said you may want to think about avoiding a flood of IPCs on every page
> load with iframes, though.

Since this is fixing a security bug, let's go ahead and go with the simpler
approach (this one).  We'll have to adjust later for click-to-script anyway.

[Devlin, 2015-09-11 23:11:06]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2015-09-11 23:11:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1335083004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1335083004/1

[commit-bot: I haz the power, 2015-09-11 23:58:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-11 23:58:33]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2015-09-14 19:47:58]

Patchset #2 (id:20001) has been deleted

[Devlin, 2015-09-14 19:48:06]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-09-14 19:48:06]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1335083004/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-09-14 19:49:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1335083004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1335083004/40001

[commit-bot: I haz the power, 2015-09-14 20:22:37]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-09-14 20:23:15]

Patchset 2 (id:??) landed as
https://crrev.com/c318b93de2ee7b8cc78e506dd2dd161af7d6819d
Cr-Commit-Position: refs/heads/master@{#348707}

[commit-bot: I haz the power, 2015-09-23 12:36:46]

Patchset 2 (id:??) landed as
https://crrev.com/c318b93de2ee7b8cc78e506dd2dd161af7d6819d
Cr-Commit-Position: refs/heads/master@{#348707}