Adding <keygen> Content Setting (Final)

Adding <keygen> Content Setting (Final)

Disabling KEYGEN based on Content Setting.

BUG=514767
Committed: https://crrev.com/be63f077985bdacc6e17cf33b151855a8412f29b
Cr-Commit-Position: refs/heads/master@{#369580}

[svaldez, 2015-12-11 16:24:35]

svaldez@chromium.org changed reviewers:
+ bauerb@chromium.org, rsleevi@chromium.org

[svaldez, 2015-12-11 16:24:36]

This is the Keygen piece that actually blocks keygen usage.

[Bernhard Bauer, 2015-12-11 16:41:46]

https://codereview.chromium.org/1476873002/diff/20001/content/common/render_p...
File content/common/render_process_messages.h (right):

https://codereview.chromium.org/1476873002/diff/20001/content/common/render_p...
content/common/render_process_messages.h:44: GURL /* URL of parent document */,
I would call this "URL of top-level document", because it might not be the
direct parent, right?

[svaldez, 2015-12-11 17:13:56]

https://codereview.chromium.org/1476873002/diff/20001/content/common/render_p...
File content/common/render_process_messages.h (right):

https://codereview.chromium.org/1476873002/diff/20001/content/common/render_p...
content/common/render_process_messages.h:44: GURL /* URL of parent document */,
On 2015/12/11 16:41:46, Bernhard Bauer wrote:
> I would call this "URL of top-level document", because it might not be the
> direct parent, right?

Done.

[Bernhard Bauer, 2015-12-11 17:17:04]

lgtm

[svaldez, 2016-01-05 22:28:56]

The CQ bit was checked by svaldez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-05 22:30:08]

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

[commit-bot: I haz the power, 2016-01-05 22:35:49]

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

[commit-bot: I haz the power, 2016-01-05 22:35:49]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[svaldez, 2016-01-05 23:33:33]

The CQ bit was checked by svaldez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-05 23:36:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1476873002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1476873002/60001

[commit-bot: I haz the power, 2016-01-06 00:49:56]

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

[commit-bot: I haz the power, 2016-01-06 00:49:56]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2016-01-06 01:40:27]

rsleevi@chromium.org changed reviewers:
+ nasko@chromium.org

[Ryan Sleevi, 2016-01-06 01:40:28]

LGTM - but you'll also need an IPC reviewer.

It's worth noting that from a permissions/security perspective, this isn't quite
right, because a compromised renderer can IPC-lie about it's top URL to cause
trouble. Ideally we'd know the document state on the browser process side to
effectively handle that. I don't know if that's something that we can get from
the browser NavigationController view - that is, what page it believes is
loaded, since that should always be the topmost document URL.

Adding nasko@ since I'm sure if there is a suggestion to be made, he can
probably make it.

That said, I don't think that should *block* this - this is a best-effort
stepping stone on the path to removal, but if we can get a little bit more
robust defense in depth, cheaply, all the better.

[nasko, 2016-01-06 17:39:29]

This CL is Not LGTM.

In the presence of out-of-process iframes, there is no guarantee that a
top-level document exists in the renderer process. If we move the code to use
the origin of the top-level frame, which is always available, it will be fine.

[nasko, 2016-01-06 17:43:46]

On 2016/01/06 01:40:28, Ryan Sleevi wrote:
> LGTM - but you'll also need an IPC reviewer.
> 
> It's worth noting that from a permissions/security perspective, this isn't
quite
> right, because a compromised renderer can IPC-lie about it's top URL to cause
> trouble. Ideally we'd know the document state on the browser process side to
> effectively handle that. I don't know if that's something that we can get from
> the browser NavigationController view - that is, what page it believes is
> loaded, since that should always be the topmost document URL.

Since the checks run on the IO thread as part of a message filter, this
information is not available. NavigationController and all navigation logic runs
only on the UI thread.
 
> Adding nasko@ since I'm sure if there is a suggestion to be made, he can
> probably make it.
> 
> That said, I don't think that should *block* this - this is a best-effort
> stepping stone on the path to removal, but if we can get a little bit more
> robust defense in depth, cheaply, all the better.

I don't think there is a cheap way of getting it at this point in time
unfortunately.

[svaldez, 2016-01-11 16:34:08]

On 2016/01/06 17:43:46, nasko wrote:
> On 2016/01/06 01:40:28, Ryan Sleevi wrote:
> > LGTM - but you'll also need an IPC reviewer.
> > 
> > It's worth noting that from a permissions/security perspective, this isn't
> quite
> > right, because a compromised renderer can IPC-lie about it's top URL to
cause
> > trouble. Ideally we'd know the document state on the browser process side to
> > effectively handle that. I don't know if that's something that we can get
from
> > the browser NavigationController view - that is, what page it believes is
> > loaded, since that should always be the topmost document URL.
> 
> Since the checks run on the IO thread as part of a message filter, this
> information is not available. NavigationController and all navigation logic
runs
> only on the UI thread.
>  
> > Adding nasko@ since I'm sure if there is a suggestion to be made, he can
> > probably make it.
> > 
> > That said, I don't think that should *block* this - this is a best-effort
> > stepping stone on the path to removal, but if we can get a little bit more
> > robust defense in depth, cheaply, all the better.
> 
> I don't think there is a cheap way of getting it at this point in time
> unfortunately.

So is the correct thing to do this:

document().frame()->localFrameRoot()->baseURL()?

[nasko, 2016-01-11 18:11:29]

On 2016/01/11 16:34:08, svaldez wrote:
> On 2016/01/06 17:43:46, nasko wrote:
> > On 2016/01/06 01:40:28, Ryan Sleevi wrote:
> > > LGTM - but you'll also need an IPC reviewer.
> > > 
> > > It's worth noting that from a permissions/security perspective, this isn't
> > quite
> > > right, because a compromised renderer can IPC-lie about it's top URL to
> cause
> > > trouble. Ideally we'd know the document state on the browser process side
to
> > > effectively handle that. I don't know if that's something that we can get
> from
> > > the browser NavigationController view - that is, what page it believes is
> > > loaded, since that should always be the topmost document URL.
> > 
> > Since the checks run on the IO thread as part of a message filter, this
> > information is not available. NavigationController and all navigation logic
> runs
> > only on the UI thread.
> >  
> > > Adding nasko@ since I'm sure if there is a suggestion to be made, he can
> > > probably make it.
> > > 
> > > That said, I don't think that should *block* this - this is a best-effort
> > > stepping stone on the path to removal, but if we can get a little bit more
> > > robust defense in depth, cheaply, all the better.
> > 
> > I don't think there is a cheap way of getting it at this point in time
> > unfortunately.
> 
> So is the correct thing to do this:
> 
> document().frame()->localFrameRoot()->baseURL()?

I'm sorry for missing to include a guidance in my previous replies. We still
need to use the top-level frame, but since it doesn't have a document, we can
only use the security origin of it (which we take care to replicate across
processes).
document().frame()->tree().top()->securityContext()->securityOrigin() will give
you the SecurityOrigin of the top frame.

[svaldez, 2016-01-11 18:36:29]

On 2016/01/11 18:11:29, nasko wrote:
> On 2016/01/11 16:34:08, svaldez wrote:
> > On 2016/01/06 17:43:46, nasko wrote:
> > > On 2016/01/06 01:40:28, Ryan Sleevi wrote:
> > > > LGTM - but you'll also need an IPC reviewer.
> > > > 
> > > > It's worth noting that from a permissions/security perspective, this
isn't
> > > quite
> > > > right, because a compromised renderer can IPC-lie about it's top URL to
> > cause
> > > > trouble. Ideally we'd know the document state on the browser process
side
> to
> > > > effectively handle that. I don't know if that's something that we can
get
> > from
> > > > the browser NavigationController view - that is, what page it believes
is
> > > > loaded, since that should always be the topmost document URL.
> > > 
> > > Since the checks run on the IO thread as part of a message filter, this
> > > information is not available. NavigationController and all navigation
logic
> > runs
> > > only on the UI thread.
> > >  
> > > > Adding nasko@ since I'm sure if there is a suggestion to be made, he can
> > > > probably make it.
> > > > 
> > > > That said, I don't think that should *block* this - this is a
best-effort
> > > > stepping stone on the path to removal, but if we can get a little bit
more
> > > > robust defense in depth, cheaply, all the better.
> > > 
> > > I don't think there is a cheap way of getting it at this point in time
> > > unfortunately.
> > 
> > So is the correct thing to do this:
> > 
> > document().frame()->localFrameRoot()->baseURL()?
> 
> I'm sorry for missing to include a guidance in my previous replies. We still
> need to use the top-level frame, but since it doesn't have a document, we can
> only use the security origin of it (which we take care to replicate across
> processes).
> document().frame()->tree().top()->securityContext()->securityOrigin() will
give
> you the SecurityOrigin of the top frame.

Should be correct now.

[nasko, 2016-01-11 22:25:47]

The CL needs a bit more updating, as it has labels of "top url" nature all over
the place, which are now "top origin".

https://codereview.chromium.org/1476873002/diff/100001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1476873002/diff/100001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:826: const blink::WebURL&
top_url) {
This is no longer an URL, but an origin. Can you please update this and the IPC
message?

[svaldez, 2016-01-12 13:53:38]

https://codereview.chromium.org/1476873002/diff/100001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1476873002/diff/100001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:826: const blink::WebURL&
top_url) {
On 2016/01/11 22:25:47, nasko wrote:
> This is no longer an URL, but an origin. Can you please update this and the
IPC
> message?

Done.

[svaldez, 2016-01-12 17:12:22]

svaldez@chromium.org changed reviewers:
+ jochen@chromium.org

[svaldez, 2016-01-12 17:12:23]

jochen: Any chance you could take a look at the Blink changes. This is the other
part of CL 1417033010 to actually block keygen based on the Content Setting.

[jochen (gone - plz use gerrit), 2016-01-12 17:27:33]

Nasko is the better reviewer here

[svaldez, 2016-01-12 17:29:11]

On 2016/01/12 17:27:33, jochen wrote:
> Nasko is the better reviewer here

I think I also need OWNERS review for third_party/WebKit/?

[jochen (gone - plz use gerrit), 2016-01-12 17:30:21]

happy to rubberstamp once he's ready

[nasko, 2016-01-12 17:34:35]

LGTM

[jochen (gone - plz use gerrit), 2016-01-12 17:35:20]

lgtm

[svaldez, 2016-01-14 21:26:59]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2016-01-14 21:26:59]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1476873002/#ps120001
(title: "Fixing naming.")

[commit-bot: I haz the power, 2016-01-14 21:28:00]

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

[commit-bot: I haz the power, 2016-01-14 22:31:41]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-14 22:32:25]

Description was changed from

==========
Adding <keygen> Content Setting (Final)

Disabling KEYGEN based on Content Setting.

BUG=514767
==========

to

==========
Adding <keygen> Content Setting (Final)

Disabling KEYGEN based on Content Setting.

BUG=514767

Committed: https://crrev.com/be63f077985bdacc6e17cf33b151855a8412f29b
Cr-Commit-Position: refs/heads/master@{#369580}
==========

[commit-bot: I haz the power, 2016-01-14 22:32:26]

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