[webcrypto] Only allow crypto.subtle.* to be used from "secure origins".

[webcrypto] Only allow crypto.subtle.* to be used from "secure origins".

The meaning of a secure origin is defined by:
http://www.chromium.org/Home/chromium-security/security-faq#TOC-Which-origins-are-secure-

In essence, "secure origins" are those that load resources either from the local machine or over the network from a cryptographically-authenticated server.

For example these are considered secure origins:
  * chrome-extension://xxx
  * https://xxx
  * wss://xxx
  * file://xxx
  * http://localhost/
  * http://127.0.0.1/

Whereas these are considered insecure:
  * http://foobar
  * ws://foobar

crypto.subtle itself is visible from insecure origins. However all of its methods will fail by returning a rejected Promise for NotSupportedError.

BUG=373032, 245025, 362214
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175916

[eroman, 2014-05-23 22:52:51]

Chris, could you give this a look over before I get Blink folks to review?
Thanks!

[eroman, 2014-05-23 23:17:23]

s/cpalmer@/palmer@

[palmer, 2014-05-24 01:44:29]

LGTM with nits.

https://codereview.chromium.org/299253003/diff/1/Source/modules/crypto/Subtle...
File Source/modules/crypto/SubtleCrypto.cpp (right):

https://codereview.chromium.org/299253003/diff/1/Source/modules/crypto/Subtle...
Source/modules/crypto/SubtleCrypto.cpp:70: SecurityOrigin* origin =
scriptState->executionContext()->securityOrigin();
Can |origin| be declared const here? Does it matter? I don't know Blink
conventions well, but it occurred to me.

https://codereview.chromium.org/299253003/diff/1/Source/platform/weborigin/Se...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/299253003/diff/1/Source/platform/weborigin/Se...
Source/platform/weborigin/SecurityOrigin.cpp:386: // FIXME: According to
http://www.chromium.org/Home/chromium-security/security-faq#TOC-Which-origins...
should match all of 127/8 and ::1/8
Should be ::1/128. Also provide a reference to
https://code.google.com/p/chromium/issues/detail?id=362214 — I'll resolve the
FIXME as part of that.

https://codereview.chromium.org/299253003/diff/1/Source/platform/weborigin/Se...
File Source/platform/weborigin/SecurityOriginTest.cpp (right):

https://codereview.chromium.org/299253003/diff/1/Source/platform/weborigin/Se...
Source/platform/weborigin/SecurityOriginTest.cpp:77: { true,
"http://127.0.0.1:80" },
Maybe throw in a few more non-standard ports just to be safe?

[eroman, 2014-05-24 02:08:57]

https://codereview.chromium.org/299253003/diff/1/Source/modules/crypto/Subtle...
File Source/modules/crypto/SubtleCrypto.cpp (right):

https://codereview.chromium.org/299253003/diff/1/Source/modules/crypto/Subtle...
Source/modules/crypto/SubtleCrypto.cpp:70: SecurityOrigin* origin =
scriptState->executionContext()->securityOrigin();
On 2014/05/24 01:44:30, Chromium Palmer wrote:
> Can |origin| be declared const here? Does it matter? I don't know Blink
> conventions well, but it occurred to me.

Done.

https://codereview.chromium.org/299253003/diff/1/Source/platform/weborigin/Se...
File Source/platform/weborigin/SecurityOriginTest.cpp (right):

https://codereview.chromium.org/299253003/diff/1/Source/platform/weborigin/Se...
Source/platform/weborigin/SecurityOriginTest.cpp:77: { true,
"http://127.0.0.1:80" },
On 2014/05/24 01:44:30, Chromium Palmer wrote:
> Maybe throw in a few more non-standard ports just to be safe?

Done. Expanded on the tests:
  * Added ftp://localhost
  * Added http://foobar.com:443

[eroman, 2014-05-24 02:11:42]

@abarth: Would you be able to review this?

Also if you have any advice on testing: I have tested at the unit-test level and
manually, however I don't have any LayoutTests tests for webcrypto rejecting the
Promises. Not sure how to accomplish that since it requires controlling the
hostname that the page is served from.

[eroman, 2014-05-28 20:44:25]

Changing reviewer to Darin.

@darin: Please see question in comment #5 (guidance on how to add layout tests).

[darin (slow to review), 2014-05-28 21:38:03]

On 2014/05/28 20:44:25, eroman wrote:
> Changing reviewer to Darin.
> 
> @darin: Please see question in comment #5 (guidance on how to add layout
tests).

Do you need complete control over the hostnames or can you get by using
different ports? IIRC http-based layout tests can be loaded over different
ports (or perhaps just a fixed set of different ports).

Alternatively, it seems like it should be fairly straightforward to modify
the content_shell to use a fake DNS provider when running layout tests. The
fake DNS provider would just map hostnames to 172.0.0.1 (i.e., to the test
server).

[eroman, 2014-05-28 23:59:07]

Yes, would need control over hostnames not just the ports.

Chrome already has such a command line flag, "--host-rules", so I could probably
wire that up for the test runner if there isn't already something like it.

I will continue exploring that in parallel. In meantime I believe this CL is
ready for review (has some basic unit tests), please take a look, thanks!

[darin (slow to review), 2014-05-29 05:08:14]

https://codereview.chromium.org/299253003/diff/40001/Source/platform/weborigi...
File Source/platform/weborigin/SecurityOriginTest.cpp (right):

https://codereview.chromium.org/299253003/diff/40001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOriginTest.cpp:82: // Access is granted to all
secure transports.
What about blob URLs generated from https:// pages? Suppose I coin a Blob from
my https:// page, and then navigate an iframe to the URL of the Blob. Is the
resulting document granted access? I think it should be.

Ditto for filesystem: URLs.

[eroman, 2014-05-29 22:23:53]

Thanks for pointing out the missing tests for filesystem and blob URLs!

Added tests for these.
@palmer: please verify those test expectations (I am treating filesystem URLs
according to their inner origin, and not as "local")

I also added a comment block explaining more on how data:URLs work and the
scenarios that I considered.

PTAL.

[palmer, 2014-05-30 19:15:58]

LGTM.

[eroman, 2014-06-03 15:33:28]

@darin: ping

[eroman, 2014-06-06 20:24:04]

Ping (need  a Blink OWNER to look)
;)

[abarth-chromium, 2014-06-06 20:36:35]

https://codereview.chromium.org/299253003/diff/70001/Source/modules/crypto/Su...
File Source/modules/crypto/SubtleCrypto.cpp (right):

https://codereview.chromium.org/299253003/diff/70001/Source/modules/crypto/Su...
Source/modules/crypto/SubtleCrypto.cpp:84: if
(!ensureCanAccessWebCrypto(scriptState, result.get()))
ensureCanAccessWebCrypto -> canAccessWebCrypto

https://codereview.chromium.org/299253003/diff/70001/Source/platform/weborigi...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/299253003/diff/70001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:441: //     data:URL.
Please remove this comment.  If you want to have this much exposition, you can
put it on the wiki page referenced on the first line of the comment.

https://codereview.chromium.org/299253003/diff/70001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:451: return false;
Generally, we don't include dead code in the product.  If you like, you can
ASSERT(m_protocol != "data");  If you're really worried, you can use
RELEASE_ASSERT instead.

https://codereview.chromium.org/299253003/diff/70001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:460: return true;
Using m_domain here is incorrect.  You want to use m_host.

It can't be right to tread 127.0.0.1 differently than 127.0.0.2.  Also, this
should be extracted into a helper function.

https://codereview.chromium.org/299253003/diff/70001/Source/platform/weborigi...
File Source/platform/weborigin/SecurityOriginTest.cpp (right):

https://codereview.chromium.org/299253003/diff/70001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOriginTest.cpp:96: { false,
"javascript:alert('hi')" },
Please remove this case.  It's an error to construct a SecurityOrigin object
with a m_protocol of "javascript".  If you like, you can add an ASSERT to the
SecurityOrigin constructor instead.

[eroman, 2014-06-10 01:00:11]

PTAL.

I addressed the FIXME for testing of 127.0.0.1/8 and added corresponding tests.

https://codereview.chromium.org/299253003/diff/70001/Source/platform/weborigi...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/299253003/diff/70001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:441: //     data:URL.
On 2014/06/06 20:36:34, abarth wrote:
> Please remove this comment.  If you want to have this much exposition, you can
> put it on the wiki page referenced on the first line of the comment.

Done.

https://codereview.chromium.org/299253003/diff/70001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:451: return false;
On 2014/06/06 20:36:34, abarth wrote:
> Generally, we don't include dead code in the product.  If you like, you can
> ASSERT(m_protocol != "data");  If you're really worried, you can use
> RELEASE_ASSERT instead.

Done.

https://codereview.chromium.org/299253003/diff/70001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:460: return true;
On 2014/06/06 20:36:34, abarth wrote:
> Using m_domain here is incorrect.  You want to use m_host.
> 
> It can't be right to tread 127.0.0.1 differently than 127.0.0.2.  Also, this
> should be extracted into a helper function.

Done.

https://codereview.chromium.org/299253003/diff/70001/Source/platform/weborigi...
File Source/platform/weborigin/SecurityOriginTest.cpp (right):

https://codereview.chromium.org/299253003/diff/70001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOriginTest.cpp:96: { false,
"javascript:alert('hi')" },
On 2014/06/06 20:36:34, abarth wrote:
> Please remove this case.  It's an error to construct a SecurityOrigin object
> with a m_protocol of "javascript".  If you like, you can add an ASSERT to the
> SecurityOrigin constructor instead.

Done.

[abarth-chromium, 2014-06-10 17:09:12]

lgtm

https://codereview.chromium.org/299253003/diff/130001/Source/platform/weborig...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/299253003/diff/130001/Source/platform/weborig...
Source/platform/weborigin/SecurityOrigin.cpp:41: #include <url/url_canon_ip.h>
#include "url/url_canon_ip.h"

You should probably also change Source/platform/DEPS to +url so that we're being
honest about our dependencies.

https://codereview.chromium.org/299253003/diff/130001/Source/platform/weborig...
Source/platform/weborigin/SecurityOrigin.cpp:382: return isLocal() ||
isLocalhost() || SchemeRegistry::shouldTreatURLSchemeAsSecure(m_protocol);
We should move isLocalhost() to the end because it's the slowest.

[eroman, 2014-06-10 18:47:27]

Thanks for reviews!

https://codereview.chromium.org/299253003/diff/130001/Source/platform/weborig...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/299253003/diff/130001/Source/platform/weborig...
Source/platform/weborigin/SecurityOrigin.cpp:41: #include <url/url_canon_ip.h>
On 2014/06/10 17:09:11, abarth wrote:
> #include "url/url_canon_ip.h"
> 
> You should probably also change Source/platform/DEPS to +url so that we're
being
> honest about our dependencies.

Done.

https://codereview.chromium.org/299253003/diff/130001/Source/platform/weborig...
Source/platform/weborigin/SecurityOrigin.cpp:382: return isLocal() ||
isLocalhost() || SchemeRegistry::shouldTreatURLSchemeAsSecure(m_protocol);
On 2014/06/10 17:09:11, abarth wrote:
> We should move isLocalhost() to the end because it's the slowest.

Good point! Done.

Similarly, I moved the secure scheme check first, since that being true will be
the normal codepath.

[eroman, 2014-06-10 18:47:32]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-06-10 18:48:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eroman@chromium.org/299253003/150001

[commit-bot: I haz the power, 2014-06-10 19:56:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10995)

[commit-bot: I haz the power, 2014-06-10 20:30:22]

Change committed as 175916