Basic experimental suborigin CSP directive and SecurityOrigin mods

Basic experimental suborigin CSP directive and SecurityOrigin mods

This adds a basic CSP directive 'suborigin' behind an experimental flag.
The implementation also adds plumbing in SecurityOrigin to support
suborigins, although nothing is explicitly enforced. However, if a
suborigin is used, it will be serialized into the SecurityOrigin's
string on a toString call, so this will have lots of side effects that
will be addressed in a subsequent CL.

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

[jww, 2015-04-25 00:44:45]

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

[jww, 2015-04-25 00:44:46]

Hi jochen and mkwst. This is another sub-CL based on
https://codereview.chromium.org/27073003/. This implements the basic CSP
directive for enforcement plus it adds parsing to SecurityOrigin. Note that I've
removed all of the access control checks, including in canAccess and canRequest.
However, as mkwst@ pointed out in the other CL, this will have side effects in
code that makes decisions based on calls to SecurityOrigin::toString since this
does serialize the Suborigin into those cases.

My original idea was to deny-by-default in those cases and work out fixes on a
per-call basis (such as calling a new SecurityOrigin::toRealOriginString()
method or something instead of SecurityOrigin::toString()), but after talking
with aaj, I'm thinking that may be the backwards approach. 

Given that this is behind an experimental flag, I'm not sure if there's a big
difference initially, though, so I thought you could take a look at this CL
as-is.

[jww, 2015-04-27 06:46:49]

On 2015/04/25 00:44:46, jww wrote:
> Hi jochen and mkwst. This is another sub-CL based on
> https://codereview.chromium.org/27073003/. This implements the basic CSP
> directive for enforcement plus it adds parsing to SecurityOrigin. Note that
I've
> removed all of the access control checks, including in canAccess and
canRequest.
> However, as mkwst@ pointed out in the other CL, this will have side effects in
> code that makes decisions based on calls to SecurityOrigin::toString since
this
> does serialize the Suborigin into those cases.
> 
> My original idea was to deny-by-default in those cases and work out fixes on a
> per-call basis (such as calling a new SecurityOrigin::toRealOriginString()
> method or something instead of SecurityOrigin::toString()), but after talking
> with aaj, I'm thinking that may be the backwards approach. 
> 
> Given that this is behind an experimental flag, I'm not sure if there's a big
> difference initially, though, so I thought you could take a look at this CL
> as-is.

I realized I should probably clarify a few points. First, I changed the
serialization to be in the host name. I've been debating over whether it should
be in the protocol or the hostname for a while, and from an implementation
perspective, KURL doesn't handle weird protocols well, so it seem to make more
sense to me to put it in the host, as well as it just seeming more of a
host-like property than a protocol related one. I chose to go with '_' because
it's a symbol that KURL understands in the host, but it is not a valid IETF
hostname character.

Additionally, I'm still debating over where to go with serialization. This
implementation serializes the suborigin with '_' as described above in
toString() and toRawString(). This will break a lot of stuff if suborigins are
used. My thought is to go call site by call site and fix the ones that should be
getting a URL without a suborigin, but if you think that the better way to go is
the other way for now (i.e. go call site by call site and figure out what
*should* be using suborigins), I'm open to that as well.

[jochen (gone - plz use gerrit), 2015-04-27 19:35:10]

overall looks ok

https://codereview.chromium.org/1109633002/diff/1/Source/core/dom/ExecutionCo...
File Source/core/dom/ExecutionContext.cpp (right):

https://codereview.chromium.org/1109633002/diff/1/Source/core/dom/ExecutionCo...
Source/core/dom/ExecutionContext.cpp:263:
ASSERT(RuntimeEnabledFeatures::suboriginsEnabled());
maybe just return if it's not enabled?

https://codereview.chromium.org/1109633002/diff/1/Source/core/dom/ExecutionCo...
File Source/core/dom/ExecutionContext.h (right):

https://codereview.chromium.org/1109633002/diff/1/Source/core/dom/ExecutionCo...
Source/core/dom/ExecutionContext.h:148: bool hasSuborigin() { return
securityContext().securityOrigin()->hasSuborigin(); }
if you moved the bodies to the .cc you wouldn't need the #include

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

https://codereview.chromium.org/1109633002/diff/1/Source/platform/weborigin/S...
Source/platform/weborigin/SecurityOrigin.cpp:508: size_t suboriginEnd =
oldHost.find('_');
underscore looks odd but is ok i guess

[Mike West, 2015-05-17 09:34:53]

These patches are languishing, and I've lost track of the conversation
elsewhere. Are you waiting on Jochen or I for anything?

[jww, 2015-05-29 04:36:36]

No, I need to address some of Jochens comments, and I'm still working on
some design details.

On Sun, May 17, 2015, 2:34 AM  <mkwst@chromium.org> wrote:

> These patches are languishing, and I've lost track of the conversation
> elsewhere. Are you waiting on Jochen or I for anything?
>
> https://codereview.chromium.org/1109633002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[jww, 2015-05-30 01:11:07]

jochen@, how does this look at this point as a commit? Ready to go? I'd like to
get this in before working on actual enforcement in a separate commit.

https://codereview.chromium.org/1109633002/diff/1/Source/core/dom/ExecutionCo...
File Source/core/dom/ExecutionContext.cpp (right):

https://codereview.chromium.org/1109633002/diff/1/Source/core/dom/ExecutionCo...
Source/core/dom/ExecutionContext.cpp:263:
ASSERT(RuntimeEnabledFeatures::suboriginsEnabled());
On 2015/04/27 19:35:10, jochen wrote:
> maybe just return if it's not enabled?

If Suborigins aren't enabled, then the CSP directive will just fail silently,
but it should never reach here, so I'd rather leave this as an ASSERT if that's
OK.

https://codereview.chromium.org/1109633002/diff/1/Source/core/dom/ExecutionCo...
File Source/core/dom/ExecutionContext.h (right):

https://codereview.chromium.org/1109633002/diff/1/Source/core/dom/ExecutionCo...
Source/core/dom/ExecutionContext.h:148: bool hasSuborigin() { return
securityContext().securityOrigin()->hasSuborigin(); }
On 2015/04/27 19:35:10, jochen wrote:
> if you moved the bodies to the .cc you wouldn't need the #include

Done.

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

https://codereview.chromium.org/1109633002/diff/1/Source/platform/weborigin/S...
Source/platform/weborigin/SecurityOrigin.cpp:508: size_t suboriginEnd =
oldHost.find('_');
On 2015/04/27 19:35:10, jochen wrote:
> underscore looks odd but is ok i guess

Yeah, I'm happy to discuss what we should use as a token, but I was struggling
to find a token that was allowed by the spec, but unused, while also something
that KURL doesn't fail on.

[jochen (gone - plz use gerrit), 2015-06-01 13:04:23]

lgtm

[jww, 2015-06-01 13:54:53]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2015-06-01 13:55:26]

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

[commit-bot: I haz the power, 2015-06-01 15:07:25]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196231