CSP Suborigins

Source/platform/weborigin/SecurityOrigin.cpp

Index: Source/platform/weborigin/SecurityOrigin.cpp
diff --git a/Source/platform/weborigin/SecurityOrigin.cpp b/Source/platform/weborigin/SecurityOrigin.cpp
index b33955e0ec34ebf4481f60a5f19fd344f373f523..0c307ea5a27f0fb1de0d50b38adc297bb4379d49 100644
--- a/Source/platform/weborigin/SecurityOrigin.cpp
+++ b/Source/platform/weborigin/SecurityOrigin.cpp
@@ -37,6 +37,7 @@
 #include "url/url_canon_ip.h"
 #include "wtf/HexNumber.h"
 #include "wtf/MainThread.h"
+#include "wtf/NotFound.h"
 #include "wtf/StdLibExtras.h"
 #include "wtf/text/StringBuilder.h"
 
@@ -62,6 +63,12 @@ static SecurityOrigin* cachedOrigin(const KURL& url)
     return 0;
 }
 
+static bool sameSuborigin(const SecurityOrigin* origin1, const SecurityOrigin* origin2)
+{
+    // If either origin has a suborigin set, both must have the same suborigin.
+    return origin1->suboriginName() == origin2->suboriginName();
+}
+
 bool SecurityOrigin::shouldUseInnerURL(const KURL& url)
 {
     // FIXME: Blob URLs don't have inner URLs. Their form is "blob:<inner-origin>/<UUID>", so treating the part after "blob:" as a URL is incorrect.
@@ -116,6 +123,24 @@ static bool shouldTreatAsUniqueOrigin(const KURL& url)
     return false;
 }
 
+bool deserializeSuboriginAndProtocol(const String& oldProtocol, String& suboriginName, String& newProtocol)
+{
+    const char suboriginPrefix[] = "suborigin+";
+    if (!oldProtocol.startsWith(suboriginPrefix))
+        return false;
+
+    size_t suboriginStart = strlen(suboriginPrefix);
+    size_t protocolStart = oldProtocol.find('+', suboriginStart + 1);
+
+    if (protocolStart == kNotFound)
+        return false;
+
+    suboriginName = oldProtocol.substring(suboriginStart, protocolStart - suboriginStart);
+    newProtocol = oldProtocol.substring(protocolStart + 1);
+

[Mike West, 2015/03/23 07:32:56]

ASSERT that the runtime-enabled feature is enabled?

[jww, 2015/04/11 02:52:36]

I instead put a check at the top to make sure we only try to deserialize a
suborigin if the feature is enabled.

+    return true;
+}
+
 SecurityOrigin::SecurityOrigin(const KURL& url)
     : m_protocol(url.protocol().isNull() ? "" : url.protocol().lower())
     , m_host(url.host().isNull() ? "" : url.host().lower())
@@ -126,6 +151,11 @@ SecurityOrigin::SecurityOrigin(const KURL& url)
     , m_enforceFilePathSeparation(false)
     , m_needsDatabaseIdentifierQuirkForFiles(false)
 {
+    // Suborigins are serialized into the protocol, so extract it if necessary.
+    String suboriginName;
+    if (deserializeSuboriginAndProtocol(m_protocol, suboriginName, m_protocol))

[Mike West, 2015/03/23 07:32:55]

A side-effect of lumping the suborigin into the protocol is that two URLs
produced from the same "real" origin might compare as distinct given their
suborigins. Have you looked into the impact of suborigins on the URL parsing
behavior of things like `Document::completeURL`? What is the expected impact?

[jww, 2015/04/25 00:41:22]

Agreed. For this original implementation, my idea was to "deny" by default and
work out how to "allow" on a case by case basis. Based on my converastion with
aaj@, I'm not sure that's the right approach so I'm reevaluating.

+        addSuborigin(suboriginName);
+
     // document.domain starts as m_host, but can be set by the DOM.
     m_domain = m_host;
 
@@ -158,6 +188,7 @@ SecurityOrigin::SecurityOrigin(const SecurityOrigin* other)
     , m_host(other->m_host.isolatedCopy())
     , m_domain(other->m_domain.isolatedCopy())
     , m_filePath(other->m_filePath.isolatedCopy())
+    , m_suboriginName(other->m_suboriginName)
     , m_port(other->m_port)
     , m_isUnique(other->m_isUnique)
     , m_universalAccess(other->m_universalAccess)
@@ -200,6 +231,15 @@ PassRefPtr<SecurityOrigin> SecurityOrigin::createUnique()
     return origin.release();
 }
 
+void SecurityOrigin::addSuborigin(const String& suborigin)
+{
+    // Changing suborigins midstream is bad. Very bad. It should not happen.
+    // This is, in fact, one of the very basic invariants that makes suborigins
+    // an effective security tool.

[Mike West, 2015/03/23 07:32:56]

ASSERT that the runtime-enabled feature is enabled?

[jww, 2015/04/11 02:52:36]

Done.

+    RELEASE_ASSERT(m_suboriginName.isNull() || m_suboriginName == suborigin);

[Mike West, 2015/03/23 07:32:55]

When would the suborigin be set to itself? Could we simply assert that it is
null instead?

[jww, 2015/04/11 02:52:36]

There are some cases, I believe during script execution, where
applyPolicySideEffectsToExecutionContext() can be called that then call
'addSuborigin()' again. We're cool with that, as long as something funky doesn't
happen and the suborigin name remains the same.

+    m_suboriginName = suborigin;

[Mike West, 2015/03/23 07:32:55]

Please add unit tests for this change. You can verify that the test crashes via
EXPECT_DEATH or similar from `gtest_death_test.h`.

[jww, 2015/04/11 02:52:36]

Done.

+}
+
 PassRefPtr<SecurityOrigin> SecurityOrigin::isolatedCopy() const
 {
     return adoptRef(new SecurityOrigin(this));
@@ -235,6 +275,9 @@ bool SecurityOrigin::canAccess(const SecurityOrigin* other) const
     if (isUnique() || other->isUnique())
         return false;
 
+    if (!sameSuborigin(this, other))
+        return false;

[Mike West, 2015/03/23 07:32:56]

Please add unit tests for this change.

[jww, 2015/04/11 02:52:36]

Done.

+
     // Here are two cases where we should permit access:
     //
     // 1) Neither document has set document.domain. In this case, we insist
@@ -298,6 +341,9 @@ bool SecurityOrigin::canRequest(const KURL& url) const
     if (targetOrigin->isUnique())
         return false;
 
+    if (!sameSuborigin(this, targetOrigin.get()))

[Mike West, 2015/03/23 07:32:55]

Please add unit tests for this change.

It seems like it's going to do more than you want, however: there are a number
of places that use `canRequest` as a proxy for "is same-origin", but don't do
CORS checks (I'm thinking of CSP violation report `referer`-stripping, but I'm
sure there are a number of other cases in the ~40 callsites). If we make this
change, we'll need to walk through each of them and add tests to verify the
behavior we expect.

(Same for `canAccess`, really, but I suspect that's more likely to be used only
in the context of another "real" SecurityOrigin, not one just generated from a
URL).

[jww, 2015/04/11 02:52:36]

Added.
So your concern is that this is *too* aggressive? That is, it will cause
'canAcess()' to return false in too many cases? I can see that being a problem,
and yes, I'll probably need to review them. It seems like they should only be a
problem if suborigins is enabled, though, so as long as we're not changing
security in the suborigin-disabled case, I have fewer concerns.

+        return false;
+
     // We call isSameSchemeHostPort here instead of canAccess because we want
     // to ignore document.domain effects.
     if (isSameSchemeHostPort(targetOrigin.get()))
@@ -392,6 +438,8 @@ SecurityOrigin::Policy SecurityOrigin::canShowNotifications() const
         return AlwaysAllow;
     if (isUnique())
         return AlwaysDeny;
+    if (hasSuborigin())
+        return AlwaysDeny;

[Mike West, 2015/03/23 07:32:55]

There's no test for this behavioral change. Please add unit tests and layout
tests.

[jww, 2015/04/11 02:52:36]

Done.

     return Ask;
 }
 
@@ -483,7 +531,14 @@ AtomicString SecurityOrigin::toRawAtomicString() const
 
 inline void SecurityOrigin::buildRawString(StringBuilder& builder) const

[Mike West, 2015/03/23 07:32:55]

Please add unit tests for the new behavior here.

 {
-    builder.reserveCapacity(m_protocol.length() + m_host.length() + 10);
+    if (hasSuborigin()) {
+        builder.reserveCapacity(11 + m_suboriginName.length() + m_protocol.length() + m_host.length() + 10);
+        builder.appendLiteral("suborigin+");
+        builder.append(m_suboriginName);
+        builder.append('+');

[Mike West, 2015/03/23 07:32:55]

This is very strange syntax.

[jww, 2015/04/11 02:52:36]

Initially I went with ':', but this causes all sorts of problems with things
then looking at the URL and thinking 'suborigin:' was the protocol. I went with
'+' instead, here, but it now has the double '+'s. This is the syntax I proposed
on WebAppSec, and I am by no means wedded to it, but I'd like to stick with it
or something similar until we get external objection or discussion (I'm happy to
change the particular tokens, though, but the protocol RFC limits us to
alphanumeric plus '+', '-', and '.').

+    } else {
+        builder.reserveCapacity(m_protocol.length() + m_host.length() + 10);
+    }
     builder.append(m_protocol);
     builder.appendLiteral("://");
     builder.append(m_host);
@@ -509,6 +564,9 @@ PassRefPtr<SecurityOrigin> SecurityOrigin::create(const String& protocol, const
 
 bool SecurityOrigin::isSameSchemeHostPort(const SecurityOrigin* other) const
 {
+    if (!sameSuborigin(this, other))

[Mike West, 2015/03/23 07:32:55]

Do we always want the suborigin check when doing a scheme/host/port check? If
so, we should rename the method.

It would be reasonable to audit the ~11 calls to verify that the behavioral
changes this would introduce are indeed reasonable. For instance, this would
appear cause us to clear the referrer for previously same-origin PingLoader
requests. That doesn't seem like the correct behavior.

[jww, 2015/04/11 02:52:36]

Even while it's experimental? It won't be a suborigin check for most people,
since they won't have the flag turned on. I guess we could make the name more
generic? Not sure what though.
It depends on how aggressive we want to be. In the initial proposal, I actually
do think it's reasonable. It should be treated as a separate origin, as if it's
untrusted, so the referrer shouldn't be sent. I think a likely outcome is that
this might turn out to be unwarranted or unnecessary, so we might want to change
it, based on feedback from consumers.

+        return false;
+
     if (m_host != other->m_host)
         return false;