CSP Suborigins

Source/platform/weborigin/SecurityOrigin.cpp

 /*
  * Copyright (C) 2007 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 19 matching lines @@
 #include "platform/weborigin/SecurityOrigin.h"
 
 #include "platform/weborigin/KURL.h"
 #include "platform/weborigin/KnownPorts.h"
 #include "platform/weborigin/SchemeRegistry.h"
 #include "platform/weborigin/SecurityOriginCache.h"
 #include "platform/weborigin/SecurityPolicy.h"
 #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"
 
 namespace blink {
 
 const int InvalidPort = 0;
 const int MaxAllowedPort = 65535;
 
 static SecurityOriginCache* s_originCache = 0;
 
 static bool schemeRequiresAuthority(const KURL& url)
 {
     // We expect URLs with these schemes to have authority components. If the
     // URL lacks an authority component, we get concerned and mark the origin
     // as unique.
     return url.protocolIsInHTTPFamily() || url.protocolIs("ftp");
 }
 
 static SecurityOrigin* cachedOrigin(const KURL& url)
 {
     if (s_originCache)
         return s_originCache->cachedOrigin(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.
     if (url.protocolIs("blob"))
         return true;
     if (url.protocolIs("filesystem"))
         return true;
     return false;
 }
 
@@ skipping 34 matching lines @@
     // that assume the scheme has already been canonicalized.
     String protocol = innerURL.protocol().lower();
 
     if (SchemeRegistry::shouldTreatURLSchemeAsNoAccess(protocol))
         return true;
 
     // This is the common case.
     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())
     , m_port(url.port())
     , m_isUnique(false)
     , m_universalAccess(false)
     , m_domainWasSetInDOM(false)
     , 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;
 
     if (isDefaultPortForProtocol(m_port, m_protocol))
         m_port = InvalidPort;
 
     // By default, only local SecurityOrigins can load local resources.
     m_canLoadLocalResources = isLocal();
 
     if (m_canLoadLocalResources)
@@ skipping 12 matching lines @@
     , m_enforceFilePathSeparation(false)
     , m_needsDatabaseIdentifierQuirkForFiles(false)
 {
 }
 
 SecurityOrigin::SecurityOrigin(const SecurityOrigin* other)
     : m_protocol(other->m_protocol.isolatedCopy())
     , 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)
     , m_domainWasSetInDOM(other->m_domainWasSetInDOM)
     , m_canLoadLocalResources(other->m_canLoadLocalResources)
     , m_enforceFilePathSeparation(other->m_enforceFilePathSeparation)
     , m_needsDatabaseIdentifierQuirkForFiles(other->m_needsDatabaseIdentifierQuirkForFiles)
 {
 }
 
@@ skipping 22 matching lines @@
     return adoptRef(new SecurityOrigin(url));
 }
 
 PassRefPtr<SecurityOrigin> SecurityOrigin::createUnique()
 {
     RefPtr<SecurityOrigin> origin = adoptRef(new SecurityOrigin());
     ASSERT(origin->isUnique());
     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));
 }
 
 void SecurityOrigin::setDomainFromDOM(const String& newDomain)
 {
     m_domainWasSetInDOM = true;
     m_domain = newDomain.lower();
 }
@@ skipping 15 matching lines @@
 {
     if (m_universalAccess)
         return true;
 
     if (this == other)
         return true;
 
     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
     //    that the scheme, host, and port of the URLs match.
     //
     // 2) Both documents have set document.domain. In this case, we insist
     //    that the documents have set document.domain to the same value and
     //    that the scheme of the URLs match.
     //
     // This matches the behavior of Firefox 2 and Internet Explorer 6.
@@ skipping 43 matching lines @@
         return true;
 
     if (isUnique())
         return false;
 
     RefPtr<SecurityOrigin> targetOrigin = SecurityOrigin::create(url);
 
     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()))
         return true;
 
     if (SecurityPolicy::isAccessWhiteListed(this, targetOrigin.get()))
         return true;
 
     return false;
 }
@@ skipping 74 matching lines @@
     errorMessage = "Only secure origins are allowed. http://goo.gl/lq4gCo";
     return false;
 }
 
 SecurityOrigin::Policy SecurityOrigin::canShowNotifications() const
 {
     if (m_universalAccess)
         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;
 }
 
 void SecurityOrigin::grantLoadLocalResources()
 {
     // Granting privileges to some, but not all, documents in a SecurityOrigin
     // is a security hazard because the documents without the privilege can
     // obtain the privilege by injecting script into the documents that have
     // been granted the privilege.
     m_canLoadLocalResources = true;
@@ skipping 69 matching lines @@
 AtomicString SecurityOrigin::toRawAtomicString() const
 {
     if (m_protocol == "file")
         return AtomicString("file://", AtomicString::ConstructFromLiteral);
 
     StringBuilder result;
     buildRawString(result);
     return result.toAtomicString();
 }
 
 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);
 
     if (m_port) {
         builder.append(':');
         builder.appendNumber(m_port);
     }
 }
 
 PassRefPtr<SecurityOrigin> SecurityOrigin::createFromString(const String& originString)
 {
     return SecurityOrigin::create(KURL(KURL(), originString));
 }
 
 PassRefPtr<SecurityOrigin> SecurityOrigin::create(const String& protocol, const String& host, int port)
 {
     if (port < 0 || port > MaxAllowedPort)
         return createUnique();
     String decodedHost = decodeURLEscapeSequences(host);
     return create(KURL(KURL(), protocol + "://" + host + ":" + String::number(port) + "/"));
 }
 
 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;
 
     if (m_protocol != other->m_protocol)
         return false;
 
     if (m_port != other->m_port)
         return false;
 
     if (isLocal() && !passesFileCheck(other))
@@ skipping 10 matching lines @@
 }
 
 void SecurityOrigin::transferPrivilegesFrom(const SecurityOrigin& origin)
 {
     m_universalAccess = origin.m_universalAccess;
     m_canLoadLocalResources = origin.m_canLoadLocalResources;
     m_enforceFilePathSeparation = origin.m_enforceFilePathSeparation;
 }
 
 } // namespace blink