CSP: Adding the 'upgrade-insecure-requests' directive.

CSP: Adding the 'upgrade-insecure-requests' directive.

This is an initial implementation of the upgrade mechanism specified
in https://w3c.github.io/webappsec/specs/upgrade/. We don't have
layout tests, as the upgrade intentionally doesn't touch the port,
and we use excitingly interesting ports like 8080 and 8443, which
mean that the resources won't load even after upgrade.

Test coverage is provided by unit tests which verify that CSP sets
the InsecureContentPolicy is correctly set for a document based on
a given policy, and that RequestFetcher and DOMWebSocket use that
policy information to upgrade URLs.

The new directive is behind the "experimental csp features" flag,
and is nowhere near shipping.

Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/rjeFL53OV4I/_NvMh0_qsWEJ

BUG=455674
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189646

[Mike West, 2015-02-05 12:35:00]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org, yoav@yoav.ws

[Mike West, 2015-02-05 12:35:01]

Jochen, Yoav, WDYT?

https://codereview.chromium.org/901903003/diff/1/Source/core/fetch/ResourceFe...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/901903003/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.cpp:725: url.setPort(8443);
:(

I think we need some sort of platform API that allows the embedder to give us
the default port (which we can override in layouttests), rather than relying on
KnownPorts.h in Blink.

WDYT of that approach?

[Yoav Weiss, 2015-02-05 13:25:36]

All in all the code looks good, but I'm slightly afraid of being too
"trigger-happy" with upgrading, in cases/contexts where the safer option (in
terms of breakage) is to stay on http, even if it means mixed-content warnings.

Also, should this be behind a feature flag?

https://codereview.chromium.org/901903003/diff/1/Source/core/dom/SecurityCont...
File Source/core/dom/SecurityContext.h (right):

https://codereview.chromium.org/901903003/diff/1/Source/core/dom/SecurityCont...
Source/core/dom/SecurityContext.h:45: InsecureContentIgnore = 0,
Excuse the bikeshedding but is "ignore" the best name here? If we're not
upgrading, we're still blocking, right?

https://codereview.chromium.org/901903003/diff/1/Source/core/fetch/ResourceFe...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/901903003/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.cpp:727: url.setPort(443);
What happens if the port is neither one of those? Do we send https traffic on
the same original port and hope for the best?

https://codereview.chromium.org/901903003/diff/1/Source/core/frame/csp/Conten...
File Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/901903003/diff/1/Source/core/frame/csp/Conten...
Source/core/frame/csp/ContentSecurityPolicy.cpp:101: return
(equalIgnoringCase(name, ConnectSrc)
Unrelated to current patch, but we're lowercasing `name` here many, many times.
Is there some caching inside String that handles that? If not, we may want to
lower case it once and compare using "==" (Or build such caching into String)

https://codereview.chromium.org/901903003/diff/1/Source/core/loader/FrameLoad...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/901903003/diff/1/Source/core/loader/FrameLoad...
Source/core/loader/FrameLoader.cpp:1414: return
SecurityContext::InsecureContentUpgrade;
Why always upgrade? Isn't ignoring the safer option if we don't know what to do?

[Mike West, 2015-02-05 13:39:34]

https://codereview.chromium.org/901903003/diff/1/Source/core/dom/SecurityCont...
File Source/core/dom/SecurityContext.h (right):

https://codereview.chromium.org/901903003/diff/1/Source/core/dom/SecurityCont...
Source/core/dom/SecurityContext.h:45: InsecureContentIgnore = 0,
On 2015/02/05 at 13:25:36, Yoav Weiss wrote:
> Excuse the bikeshedding but is "ignore" the best name here? If we're not
upgrading, we're still blocking, right?

No, it certainly isn't. InsecureContentDefault, maybe? Idunno.

https://codereview.chromium.org/901903003/diff/1/Source/core/fetch/ResourceFe...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/901903003/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.cpp:727: url.setPort(443);
On 2015/02/05 at 13:25:36, Yoav Weiss wrote:
> What happens if the port is neither one of those? Do we send https traffic on
the same original port and hope for the best?

Yup. Same as HSTS. See the comment I just added to the spec in step 8 of
https://w3c.github.io/webappsec/specs/upgrade/#upgrade-request.

https://codereview.chromium.org/901903003/diff/1/Source/core/frame/csp/CSPDir...
File Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/901903003/diff/1/Source/core/frame/csp/CSPDir...
Source/core/frame/csp/CSPDirectiveList.cpp:744: else if (equalIgnoringCase(name,
ContentSecurityPolicy::UpgradeInsecureContent))
This probably wasn't clear from the diff, but we only hit this piece if the
"Experimental CSP" flag is set.

https://codereview.chromium.org/901903003/diff/1/Source/core/frame/csp/Conten...
File Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/901903003/diff/1/Source/core/frame/csp/Conten...
Source/core/frame/csp/ContentSecurityPolicy.cpp:101: return
(equalIgnoringCase(name, ConnectSrc)
On 2015/02/05 at 13:25:36, Yoav Weiss wrote:
> Unrelated to current patch, but we're lowercasing `name` here many, many
times. Is there some caching inside String that handles that? If not, we may
want to lower case it once and compare using "==" (Or build such caching into
String)

Sounds reasonable. I'll follow up on that.

https://codereview.chromium.org/901903003/diff/1/Source/core/loader/FrameLoad...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/901903003/diff/1/Source/core/loader/FrameLoad...
Source/core/loader/FrameLoader.cpp:1414: return
SecurityContext::InsecureContentUpgrade;
On 2015/02/05 at 13:25:36, Yoav Weiss wrote:
> Why always upgrade? Isn't ignoring the safer option if we don't know what to
do?

I suppose so. *shrug* I'll change it. :)

[Yoav Weiss, 2015-02-05 13:49:13]

https://codereview.chromium.org/901903003/diff/1/Source/core/dom/SecurityCont...
File Source/core/dom/SecurityContext.h (right):

https://codereview.chromium.org/901903003/diff/1/Source/core/dom/SecurityCont...
Source/core/dom/SecurityContext.h:45: InsecureContentIgnore = 0,
On 2015/02/05 13:39:34, Mike West wrote:
> On 2015/02/05 at 13:25:36, Yoav Weiss wrote:
> > Excuse the bikeshedding but is "ignore" the best name here? If we're not
> upgrading, we're still blocking, right?
> 
> No, it certainly isn't. InsecureContentDefault, maybe? Idunno.

Yeah, "InsecureContentDefault" works for me

https://codereview.chromium.org/901903003/diff/1/Source/core/frame/csp/CSPDir...
File Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/901903003/diff/1/Source/core/frame/csp/CSPDir...
Source/core/frame/csp/CSPDirectiveList.cpp:744: else if (equalIgnoringCase(name,
ContentSecurityPolicy::UpgradeInsecureContent))
On 2015/02/05 13:39:34, Mike West wrote:
> This probably wasn't clear from the diff, but we only hit this piece if the
> "Experimental CSP" flag is set.

Yeah, missed that :)

[Yoav Weiss, 2015-02-05 13:55:29]

LGTM

https://codereview.chromium.org/901903003/diff/20001/Source/core/dom/Security...
File Source/core/dom/SecurityContext.h (right):

https://codereview.chromium.org/901903003/diff/20001/Source/core/dom/Security...
Source/core/dom/SecurityContext.h:45: InsecureContentDoNotUpgrade = 0,
Even better :)

https://codereview.chromium.org/901903003/diff/20001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/901903003/diff/20001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:726: else if (url.port() == 80)
Most probably premature optimization, but since port 80 is the majority case,
having its "if" as the first comparison can save us a couple of comparisons most
of the time.

[Yoav Weiss, 2015-02-05 14:01:31]

The comment in https://w3c.github.io/webappsec/specs/upgrade/#upgrade-request
doesn't refer to ports 8000 and 8080.
Is the move from 8000/8080 to 8433 defined somewhere?

[Mike West, 2015-02-05 14:01:42]

On 2015/02/05 at 13:55:29, yoav wrote:
> LGTM
> 
>
https://codereview.chromium.org/901903003/diff/20001/Source/core/dom/Security...
> File Source/core/dom/SecurityContext.h (right):
> 
>
https://codereview.chromium.org/901903003/diff/20001/Source/core/dom/Security...
> Source/core/dom/SecurityContext.h:45: InsecureContentDoNotUpgrade = 0,
> Even better :)
> 
>
https://codereview.chromium.org/901903003/diff/20001/Source/core/fetch/Resour...
> File Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/901903003/diff/20001/Source/core/fetch/Resour...
> Source/core/fetch/ResourceFetcher.cpp:726: else if (url.port() == 80)
> Most probably premature optimization, but since port 80 is the majority case,
having its "if" as the first comparison can save us a couple of comparisons most
of the time.

The majority case is actually 0: `http://example.com/` has a port of 0 in KURL,
while `http://example.com:80/` has a port of 80.

[Mike West, 2015-02-05 14:02:52]

https://codereview.chromium.org/901903003/diff/20001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/901903003/diff/20001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:724: if (url.port() == 8000 || url.port()
== 8080)
And, as much as I'd like to, I really can't land it with this comparison. I need
a better option to distinguish between the way it should work in the real world,
and the way it should work for crazy layout tests. :/

[Mike West, 2015-02-05 14:07:41]

Also also, you should tell me to send an intent to implement to blink-dev. :)

[Yoav Weiss, 2015-02-05 14:09:30]

On 2015/02/05 14:07:41, Mike West wrote:
> Also also, you should tell me to send an intent to implement to blink-dev. :)

true :)

[Mike West, 2015-02-05 14:51:45]

New patchsets have been uploaded after l-g-t-m from yoav@yoav.ws

[Ryan Sleevi, 2015-02-05 20:03:03]

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

[Ryan Sleevi, 2015-02-05 20:03:04]

This isn't hidden behind a features flag?

https://codereview.chromium.org/901903003/diff/40001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/901903003/diff/40001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:907: if
(m_document->insecureContentPolicy() == SecurityContext::InsecureContentUpgrade
&& url.protocol() == "http") {
*cough* ws:// -> wss://

https://codereview.chromium.org/901903003/diff/40001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:911: url.setPort(8443);
wat?

No no no

https://codereview.chromium.org/901903003/diff/40001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:913: url.setPort(443);
yes yes yes

[Mike West, 2015-02-05 20:19:08]

On 2015/02/05 at 20:03:04, rsleevi wrote:
> This isn't hidden behind a features flag?

It is. It isn't obvious from the diff, but the CSP directive is only parsed if
"experimental csp bits and pieces" is on.

https://codereview.chromium.org/901903003/diff/40001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/901903003/diff/40001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:907: if
(m_document->insecureContentPolicy() == SecurityContext::InsecureContentUpgrade
&& url.protocol() == "http") {
On 2015/02/05 at 20:03:04, Ryan Sleevi wrote:
> *cough* ws:// -> wss://

That's going to happen in a followup (somewhere around
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...),
as WebSockets doesn't go through ResourceFetcher.

https://codereview.chromium.org/901903003/diff/40001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:911: url.setPort(8443);
On 2015/02/05 at 20:03:03, Ryan Sleevi wrote:
> wat?
> 
> No no no

Totally. I did this to make the test work as a PoC, I don't intend to land it
this way. As I noted in patchset #2, "And, as much as I'd like to, I really
can't land it with this comparison. I need a better option to distinguish
between the way it should work in the real world, and the way it should work for
crazy layout tests. :/"

[Mike West, 2015-02-06 09:24:54]

Yoav, WDYT about the unittests here?

[Yoav Weiss, 2015-02-06 09:58:36]

These layout tests look great, but I don't think they cover everything.
Specifically, they do not cover the setting of that directive through HTML or
HTTP headers. Can you add 1-2 layout tests just to make sure that this part
actually works?

https://codereview.chromium.org/901903003/diff/60001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-script.html
(right):

https://codereview.chromium.org/901903003/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-script.html:6:
if (window.parent != window)
Is this a leftover from the layout tests change?

https://codereview.chromium.org/901903003/diff/60001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/901903003/diff/60001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:901: void
ResourceFetcher::maybeUpgradeInsecureRequestURL(FetchRequest& request)
"monitor" does nothing for now, right?

Maybe add a FIXME to that effect?

https://codereview.chromium.org/901903003/diff/60001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcherTest.cpp (right):

https://codereview.chromium.org/901903003/diff/60001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcherTest.cpp:88: RefPtr<SecurityOrigin>
insecureOrigin;
Is insecureOrigin used anywhere?

https://codereview.chromium.org/901903003/diff/60001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcherTest.cpp:89: RefPtr<DocumentLoader>
documentLoader;
documentLoader is a member only because we want to maintain it alive while
fetcher and document are used, right? Maybe add a comment to that effect, so it
won't get accidentally deleted in the future?

https://codereview.chromium.org/901903003/diff/60001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcherTest.cpp:92: RefPtrWillBePersistent<Document>
document;
Not sure if it's a requirement or just commonly used pattern, but shouldn't
these members start with "m_"?

[Mike West, 2015-02-06 11:06:57]

Take another look? :) I added WebSocket support to make Ryan happy.

[Yoav Weiss, 2015-02-06 11:35:10]

On 2015/02/06 11:06:57, Mike West wrote:
> Take another look? :) I added WebSocket support to make Ryan happy.

Looks good! :)
re-LGTM for core/
I'll let Ryan report his happiness status with the WebSocket stuff.

[Mike West, 2015-02-06 14:16:19]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2015-02-06 14:16:20]

I'll land it to make sure something is in next week's Canary to play around
with; if Ryan doesn't like something about WebSockets, I'll change it in a
subsequent CL.

Thanks!

[commit-bot: I haz the power, 2015-02-06 14:16:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/901903003/80001

[commit-bot: I haz the power, 2015-02-06 14:19:39]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189646

[Yoav Weiss, 2015-02-06 14:19:50]

On 2015/02/06 14:16:20, Mike West wrote:
> I'll land it to make sure something is in next week's Canary to play around
> with; if Ryan doesn't like something about WebSockets, I'll change it in a
> subsequent CL.
> 
> Thanks!

WFM. FWIW, I reviewed the WebSockets part as well, just don't feel I know it
well enough.

[Ryan Sleevi, 2015-02-07 00:50:10]

The websockets bits LGTM too, as it all does. Assuming it's flag protected :)

[Mike West, 2015-02-07 04:46:00]

On 2015/02/07 at 00:50:10, rsleevi wrote:
> The websockets bits LGTM too, as it all does. Assuming it's flag protected :)

It's behind a flag, and the
`RuntimeEnabledFeatures::setExperimentalContentSecurityPolicyFeaturesEnabled(true);`
bits in `ContentSecurityPolicyTest.cpp` verify that behavior.