Harden the implementation of '--disable-web-security'

Harden the implementation of '--disable-web-security'

This patch adds a browser-side check that ensures universal access can
only be granted if the browser process has the '--disable-web-security'
command-line flag. It also weakens the grant given for 'file:' URLs
by things like the 'setAllowUniversalAccessFromFileURLs()' WebView API
by ensuring that it may only be applied to "local" origins.

BUG=327804

[Mike West, 2015-12-09 12:39:41]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org, torne@chromium.org

[Mike West, 2015-12-09 12:39:42]

WDYT, Jochen?

torne@: FYI. I don't think this will affect Android, as the existing WebView API
should be safely locked to `file:` URLs anyway. It's possible that this
hardening will break someone somewhere, though, so, you know. WDYT?

[Torne, 2015-12-09 12:55:57]

LGTM from WebView's POV. We definitely don't intend to ever allow the equivalent
of --disable-web-security (thankfully).

https://codereview.chromium.org/1507023004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h (right):

https://codereview.chromium.org/1507023004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h:161: // the API
which requires this method, we should remove it.
You can have this comment if you want, but realistically WebView is not going to
be able to actually remove this for a very long time - we're hoping to deprecate
it, strongly discourage it, and maybe even disable it in apps that target at
least some reasonably modern OS version, but it's hard to get away from backward
compatibility for old apps. :/

[Mike West, 2015-12-09 14:17:49]

Thanks for the confirmation, torne@!

https://codereview.chromium.org/1507023004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h (right):

https://codereview.chromium.org/1507023004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h:161: // the API
which requires this method, we should remove it.
On 2015/12/09 at 12:55:56, Torne wrote:
> You can have this comment if you want, but realistically WebView is not going
to be able to actually remove this for a very long time - we're hoping to
deprecate it, strongly discourage it, and maybe even disable it in apps that
target at least some reasonably modern OS version, but it's hard to get away
from backward compatibility for old apps. :/

*sigh*

[Marshall, 2015-12-09 14:20:21]

marshall@chromium.org changed reviewers:
+ marshall@chromium.org

[Marshall, 2015-12-09 14:20:22]

https://codereview.chromium.org/1507023004/diff/40001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1507023004/diff/40001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:434: thread->Send(new
RenderProcessHostMsg_GetUniversalAccessDisposition(
Since the result of this sync IPC call is effectively static it might be better
to either (a) cache the result from a single call or (b) pass the
kDisableWebSecurity flag to the renderer process and check it there instead of
using an IPC.

[Mike West, 2015-12-09 14:53:50]

On 2015/12/09 at 14:20:22, marshall wrote:
>
https://codereview.chromium.org/1507023004/diff/40001/content/renderer/render...
> content/renderer/renderer_blink_platform_impl.cc:434: thread->Send(new
RenderProcessHostMsg_GetUniversalAccessDisposition(
> Since the result of this sync IPC call is effectively static it might be
better to either (a) cache the result from a single call or (b) pass the
kDisableWebSecurity flag to the renderer process and check it there instead of
using an IPC.

The current code more or less caches the value of this flag via WebPreferences
and Settings. The sync IPC is meant to mitigate the risk of a compromised
renderer by allowing the browser to mediate each privilege grant. Only caching
the result in the renderer means trusting the renderer more than we need to.

*shrug* I think it would be helpful, but I'm happy to believe that this would
only provide imaginary threat mitigation. :)

[Marshall, 2015-12-09 15:09:37]

On 2015/12/09 14:53:50, Mike West wrote:
> On 2015/12/09 at 14:20:22, marshall wrote:
> >
>
https://codereview.chromium.org/1507023004/diff/40001/content/renderer/render...
> > content/renderer/renderer_blink_platform_impl.cc:434: thread->Send(new
> RenderProcessHostMsg_GetUniversalAccessDisposition(
> > Since the result of this sync IPC call is effectively static it might be
> better to either (a) cache the result from a single call or (b) pass the
> kDisableWebSecurity flag to the renderer process and check it there instead of
> using an IPC.
> 
> The current code more or less caches the value of this flag via WebPreferences
> and Settings. The sync IPC is meant to mitigate the risk of a compromised
> renderer by allowing the browser to mediate each privilege grant. Only caching
> the result in the renderer means trusting the renderer more than we need to.
> 
> *shrug* I think it would be helpful, but I'm happy to believe that this would
> only provide imaginary threat mitigation. :)

That makes sense. However, I wonder if frequently making this sync IPC as a
result of SecurityOrigin calls will have performance consequences.

[Mike West, 2015-12-09 17:05:40]

On 2015/12/09 at 15:09:37, marshall wrote:
> > *shrug* I think it would be helpful, but I'm happy to believe that this
would
> > only provide imaginary threat mitigation. :)
> 
> That makes sense. However, I wonder if frequently making this sync IPC as a
result of SecurityOrigin calls will have performance consequences.

It will. I'm only doing a RELEASE_ASSERT_WITH_SECURITY_IMPLICATION once, when
the privilege is granted (which happens exactly once per `SecurityOrigin`, when
the document is created). The ASSERTs in `canRequest()`, `canAccess()`, and
`canDisplay()` will only take effect in debug, but will be compiled out for
release. I'd love to do the same for `canAccess()`, but we call it all the time,
and it would tank our DOM perf, which I don't think folks will accept.

[Mike West, 2015-12-09 17:06:40]

On 2015/12/09 at 17:05:40, Mike West wrote:
> On 2015/12/09 at 15:09:37, marshall wrote:
> > > *shrug* I think it would be helpful, but I'm happy to believe that this
would
> > > only provide imaginary threat mitigation. :)
> > 
> > That makes sense. However, I wonder if frequently making this sync IPC as a
result of SecurityOrigin calls will have performance consequences.
> 
> It will. I'm only doing a RELEASE_ASSERT_WITH_SECURITY_IMPLICATION once, when
the privilege is granted (which happens exactly once per `SecurityOrigin`, when
the document is created). The ASSERTs in `canRequest()`, `canAccess()`, and
`canDisplay()` will only take effect in debug, but will be compiled out for
release. I'd love to do the same for `canAccess()`, but we call it all the time,
and it would tank our DOM perf, which I don't think folks will accept.

Hit "send" too quickly: I wanted to add that if the Debug bots have 0 perf
impact from this CL, I'd totally flip to RELEASE_ASSERT. I think the chances of
that happening are around -3%. :)

[esprehn, 2015-12-10 08:06:43]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-12-10 08:06:44]

https://codereview.chromium.org/1507023004/diff/40001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1507023004/diff/40001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:433: if (RenderThread* thread =
RenderThread::Get()) {
how is it possible that this is null? I don;t think we can be here and not have
a RenderThread.

https://codereview.chromium.org/1507023004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/1507023004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:244:
ASSERT(blink::Platform::current()->canGrantUniversalAccess());
you don't need the blink:: prefix, also this should probably null check platform
for tests?

https://codereview.chromium.org/1507023004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:305:
ASSERT(blink::Platform::current()->canGrantUniversalAccess());
ditto

https://codereview.chromium.org/1507023004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:361:
ASSERT(blink::Platform::current()->canGrantUniversalAccess());
ditto

[Mike West, 2015-12-10 13:44:15]

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

[commit-bot: I haz the power, 2015-12-10 13:44:29]

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

[Mike West, 2015-12-10 13:45:17]

On 2015/12/10 at 08:06:44, esprehn wrote:
>
https://codereview.chromium.org/1507023004/diff/40001/content/renderer/render...
> File content/renderer/renderer_blink_platform_impl.cc (right):
> 
>
https://codereview.chromium.org/1507023004/diff/40001/content/renderer/render...
> content/renderer/renderer_blink_platform_impl.cc:433: if (RenderThread* thread
= RenderThread::Get()) {
> how is it possible that this is null? I don;t think we can be here and not
have a RenderThread.
> 
>
https://codereview.chromium.org/1507023004/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):
> 
>
https://codereview.chromium.org/1507023004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:244:
ASSERT(blink::Platform::current()->canGrantUniversalAccess());
> you don't need the blink:: prefix, also this should probably null check
platform for tests?
> 
>
https://codereview.chromium.org/1507023004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:305:
ASSERT(blink::Platform::current()->canGrantUniversalAccess());
> ditto
> 
>
https://codereview.chromium.org/1507023004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:361:
ASSERT(blink::Platform::current()->canGrantUniversalAccess());
> ditto

I've addressed these comments, thanks! Would you mind taking another look at the
Blink bits?

Jochen, would you mind taking a look at the //content bits?

[commit-bot: I haz the power, 2015-12-10 14:53:38]

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

[commit-bot: I haz the power, 2015-12-10 14:53:39]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jochen (gone - plz use gerrit), 2015-12-10 15:39:33]

https://codereview.chromium.org/1507023004/diff/40001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1507023004/diff/40001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:434: thread->Send(new
RenderProcessHostMsg_GetUniversalAccessDisposition(
On 2015/12/09 at 14:20:22, Marshall wrote:
> Since the result of this sync IPC call is effectively static it might be
better to either (a) cache the result from a single call or (b) pass the
kDisableWebSecurity flag to the renderer process and check it there instead of
using an IPC.

a compromised renderer can just not ask the browser

https://codereview.chromium.org/1507023004/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/Platform.h (left):

https://codereview.chromium.org/1507023004/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/Platform.h:415: 
unrelated?

https://codereview.chromium.org/1507023004/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameClient.h (left):

https://codereview.chromium.org/1507023004/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:192: 
unrelated?

[esprehn, 2015-12-11 00:09:54]

When are these sync IPCs? Every time you create a new iframe? Please don't add
more of those, they're a real burden on page load, ads mean there's lots of
iframes.

[Mike West, 2015-12-11 14:06:17]

On 2015/12/10 at 15:39:33, jochen wrote:
>
https://codereview.chromium.org/1507023004/diff/40001/content/renderer/render...
> content/renderer/renderer_blink_platform_impl.cc:434: thread->Send(new
RenderProcessHostMsg_GetUniversalAccessDisposition(
> On 2015/12/09 at 14:20:22, Marshall wrote:
> > Since the result of this sync IPC call is effectively static it might be
better to either (a) cache the result from a single call or (b) pass the
kDisableWebSecurity flag to the renderer process and check it there instead of
using an IPC.
> 
> a compromised renderer can just not ask the browser

There's compromise and compromise. This patch would prevent malicious folks from
just calling `grantUniversalAccess()`, which seems like a win. It wouldn't
prevent them from just overwriting the boolean, of course. Defense in depth,
right?

[Mike West, 2015-12-11 14:11:42]

On 2015/12/11 at 00:09:54, esprehn wrote:
> When are these sync IPCs? Every time you create a new iframe? Please don't add
more of those, they're a real burden on page load, ads mean there's lots of
iframes.

This is called during `Document` initialization.

I can make it an async call, I suppose, but that turns into a good deal of
boilerplate for something that boils down to asking the browser whether a bit is
set, and it could end up making the feature flaky (that is,
`--disable-web-security` might take effect if your JavaScript is called after
the IPC callback, or it might fail if things happen in the opposite order). I'm
not sure that's a great tradeoff. *shrug*

[esprehn, 2015-12-11 16:23:13]

Doing another sync up for every document is not acceptable. This needs to
be passed down at process startup or async. The browser process is busy
with an kinds of things, and doing this ipc for every iframe just gets you
descheduled or blocked. This does not seem like a good security vs perf
trade off, especially since a compromised renderer can just skip it anyway.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[esprehn, 2015-12-11 16:29:07]

Doing another sync up for every document is not acceptable. This needs to
be passed down at process startup or async. The browser process is busy
with an kinds of things, and doing this ipc for every iframe just gets you
descheduled or blocked. This does not seem like a good security vs perf
trade off, especially since a compromised renderer can just skip it anyway.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[jochen (gone - plz use gerrit), 2015-12-14 12:09:25]

On 2015/12/11 at 14:06:17, mkwst wrote:
> On 2015/12/10 at 15:39:33, jochen wrote:
> >
https://codereview.chromium.org/1507023004/diff/40001/content/renderer/render...
> > content/renderer/renderer_blink_platform_impl.cc:434: thread->Send(new
RenderProcessHostMsg_GetUniversalAccessDisposition(
> > On 2015/12/09 at 14:20:22, Marshall wrote:
> > > Since the result of this sync IPC call is effectively static it might be
better to either (a) cache the result from a single call or (b) pass the
kDisableWebSecurity flag to the renderer process and check it there instead of
using an IPC.
> > 
> > a compromised renderer can just not ask the browser
> 
> There's compromise and compromise. This patch would prevent malicious folks
from just calling `grantUniversalAccess()`, which seems like a win. It wouldn't
prevent them from just overwriting the boolean, of course. Defense in depth,
right?

hum, i don't think this adds security. if you have code execution, you have code
execution

[Torne, 2016-09-13 15:03:58]

Are we going ahead with this, or should the CL be closed?