[wip] Respect the clipboardRead and clipboardWrite permissions in content scripts.

[wip] Respect the clipboardRead and clipboardWrite permissions in content scripts.

BUG=395376

[not at google - send to devlin, 2014-08-14 20:52:52]

Daniel could you have an early look at this? I'm a little iffy about the whole
removing the IPC part, *but* if a webpage manages to circumvent this in some
way, chances are they already can since the clipboard contents is on the
renderer.

If the clipboard wasn't (i.e. the permissions check + grab clipboard were part
of the same IPC), that'd be another story.

The tests are a bit of a disaster (and they don't test all combinations of
clipboardRead/Write, which they should). I'm giving this CL a break for the rest
of the day while the brain dust settles.

[not at google - send to devlin, 2014-08-14 20:55:20]

Adam could you quickly sanity check that I'm doing the Calling vs Current stuff
right?

Charlie it looks like Daniel is OOO but his calendar doesn't say for how long.
Maybe you can triage this?

[dcheng, 2014-08-14 20:57:41]

On 2014/08/14 at 20:55:20, kalman wrote:
> Adam could you quickly sanity check that I'm doing the Calling vs Current
stuff right?
> 
> Charlie it looks like Daniel is OOO but his calendar doesn't say for how long.
Maybe you can triage this?

(I'm looking =)

[adamk, 2014-08-14 21:05:53]

On 2014/08/14 at 20:55:20, kalman wrote:
> Adam could you quickly sanity check that I'm doing the Calling vs Current
stuff right?

Calling seems reasonable to me (though I don't know much about
ScriptContextSet).

[dcheng, 2014-08-14 21:11:57]

My only question is what happens with web extents. I added hacks so that
extension permissions on a web extent effectively work in the origin. This means
that an about:blank iframe in a web extent would 'inherit' its parent's
permissions. Does that still work?

https://codereview.chromium.org/475883003/diff/1/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/475883003/diff/1/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:417: // permissions.
I think this is fine. Ultimately, this permission is enforced in the renderer
anyway, so sending it to the browser is pointless.

[not at google - send to devlin, 2014-08-14 21:53:52]

On 2014/08/14 21:11:57, dcheng (OOO) wrote:
> My only question is what happens with web extents. I added hacks so that
> extension permissions on a web extent effectively work in the origin. This
means
> that an about:blank iframe in a web extent would 'inherit' its parent's
> permissions. Does that still work?

No idea, though I'd guess probably not unless it's tested. We don't propagate
the opener's permissions to about:blank frames etc, though there was that code
you reviewed a little while ago to make it work for content scripts.

I'll need to look into that.

> 
>
https://codereview.chromium.org/475883003/diff/1/chrome/renderer/content_sett...
> File chrome/renderer/content_settings_observer.cc (right):
> 
>
https://codereview.chromium.org/475883003/diff/1/chrome/renderer/content_sett...
> chrome/renderer/content_settings_observer.cc:417: // permissions.
> I think this is fine. Ultimately, this permission is enforced in the renderer
> anyway, so sending it to the browser is pointless.

[not at google - send to devlin, 2014-08-14 21:54:22]

> content scripts.

... host permissions, rather.

[dcheng, 2014-08-15 04:03:34]

On 2014/08/14 at 21:53:52, kalman wrote:
> On 2014/08/14 21:11:57, dcheng (OOO) wrote:
> > My only question is what happens with web extents. I added hacks so that
> > extension permissions on a web extent effectively work in the origin. This
means
> > that an about:blank iframe in a web extent would 'inherit' its parent's
> > permissions. Does that still work?
> 
> No idea, though I'd guess probably not unless it's tested. We don't propagate
the opener's permissions to about:blank frames etc, though there was that code
you reviewed a little while ago to make it work for content scripts.

We don't explicitly propagate them, but that was the net effect of original
permission check. We had to do this to fix https://crbug.com/94399, which is
something Drive ran into a long time ago. I've not thrilled with the 'fix' I
ended up implementing, since that meant extension permission handling became
this horrendous mishmash mixing origins freely with URLs, depending on whichever
one happened to be convenient. http://codereview.chromium.org/7972020/ has some
more background about this.

[not at google - send to devlin, 2014-08-15 15:53:14]

On 2014/08/15 04:03:34, dcheng (OOO) wrote:
> On 2014/08/14 at 21:53:52, kalman wrote:
> > On 2014/08/14 21:11:57, dcheng (OOO) wrote:
> > > My only question is what happens with web extents. I added hacks so that
> > > extension permissions on a web extent effectively work in the origin. This
> means
> > > that an about:blank iframe in a web extent would 'inherit' its parent's
> > > permissions. Does that still work?
> > 
> > No idea, though I'd guess probably not unless it's tested. We don't
propagate
> the opener's permissions to about:blank frames etc, though there was that code
> you reviewed a little while ago to make it work for content scripts.
> 
> We don't explicitly propagate them, but that was the net effect of original
> permission check. We had to do this to fix https://crbug.com/94399, which is
> something Drive ran into a long time ago. I've not thrilled with the 'fix' I
> ended up implementing, since that meant extension permission handling became
> this horrendous mishmash mixing origins freely with URLs, depending on
whichever
> one happened to be convenient. http://codereview.chromium.org/7972020/ has
some
> more background about this.

Ok. I've wanted (well... considered) for a while to make about:blank iframes
inherit the privilege level of their parents/openers/whatever (i.e. be able to
call the same extension APIs and so on). At the moment they get nothing, and
that's... a pain. If I did that it'd automatically solve this problem. Does that
sound reasonable?

[dcheng, 2014-08-15 18:11:54]

On 2014/08/15 at 15:53:14, kalman wrote:
> On 2014/08/15 04:03:34, dcheng (OOO) wrote:
> > On 2014/08/14 at 21:53:52, kalman wrote:
> > > On 2014/08/14 21:11:57, dcheng (OOO) wrote:
> > > > My only question is what happens with web extents. I added hacks so that
> > > > extension permissions on a web extent effectively work in the origin.
This
> > means
> > > > that an about:blank iframe in a web extent would 'inherit' its parent's
> > > > permissions. Does that still work?
> > > 
> > > No idea, though I'd guess probably not unless it's tested. We don't
propagate
> > the opener's permissions to about:blank frames etc, though there was that
code
> > you reviewed a little while ago to make it work for content scripts.
> > 
> > We don't explicitly propagate them, but that was the net effect of original
> > permission check. We had to do this to fix https://crbug.com/94399, which is
> > something Drive ran into a long time ago. I've not thrilled with the 'fix' I
> > ended up implementing, since that meant extension permission handling became
> > this horrendous mishmash mixing origins freely with URLs, depending on
whichever
> > one happened to be convenient. http://codereview.chromium.org/7972020/ has
some
> > more background about this.
> 
> Ok. I've wanted (well... considered) for a while to make about:blank iframes
inherit the privilege level of their parents/openers/whatever (i.e. be able to
call the same extension APIs and so on). At the moment they get nothing, and
that's... a pain. If I did that it'd automatically solve this problem. Does that
sound reasonable?

That would work. It's too bad we have to make a duplicate of the web security
model though =(

[not at google - send to devlin, 2014-08-15 18:24:47]

> That would work. It's too bad we have to make a duplicate of the web security
> model though =(

Yep :( is it... something we can ask blink, in theory?

[dcheng, 2014-08-18 19:16:51]

On 2014/08/15 at 18:24:47, kalman wrote:
> > That would work. It's too bad we have to make a duplicate of the web
security
> > model though =(
> 
> Yep :( is it... something we can ask blink, in theory?

We could probably make use of SecurityOrigin::canAccess() to try to make sure
permissions are propagated only when child windows/frames are same origin with
their parents, but I'm not familiar with how we handle extension permissions in
the renderer side to say if this would be just a little work or tons of work.

[jleichtling, 2014-08-26 02:21:34]

On 2014/08/18 19:16:51, dcheng (OOO) wrote:
> On 2014/08/15 at 18:24:47, kalman wrote:
> > > That would work. It's too bad we have to make a duplicate of the web
> security
> > > model though =(
> > 
> > Yep :( is it... something we can ask blink, in theory?
> 
> We could probably make use of SecurityOrigin::canAccess() to try to make sure
> permissions are propagated only when child windows/frames are same origin with
> their parents, but I'm not familiar with how we handle extension permissions
in
> the renderer side to say if this would be just a little work or tons of work.

What is the status of this change/bug fix at the moment?

[jleichtling, 2014-08-26 02:21:36]

On 2014/08/18 19:16:51, dcheng (OOO) wrote:
> On 2014/08/15 at 18:24:47, kalman wrote:
> > > That would work. It's too bad we have to make a duplicate of the web
> security
> > > model though =(
> > 
> > Yep :( is it... something we can ask blink, in theory?
> 
> We could probably make use of SecurityOrigin::canAccess() to try to make sure
> permissions are propagated only when child windows/frames are same origin with
> their parents, but I'm not familiar with how we handle extension permissions
in
> the renderer side to say if this would be just a little work or tons of work.

What is the status of this change/bug fix at the moment?

[not at google - send to devlin, 2014-08-26 02:30:14]

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

[not at google - send to devlin, 2014-08-26 02:30:14]

Marijn wrote a test to prove this patch has a bug.

[Marijn Kruisselbrink, 2014-09-03 19:55:57]

On 2014/08/26 02:30:14, kalman (OOO until 6th Oct) wrote:
> Marijn wrote a test to prove this patch has a bug.
I've put up a fixed version of this CL without the bug at
https://codereview.chromium.org/498513002/