Skip to content
Permalink
Browse files
Make SecurityOrigin safe to create and use from any thread
https://bugs.webkit.org/show_bug.cgi?id=184216

Reviewed by Youenn Fablet.

We found that we have a decent amount of code constructing and using SecurityOrigin
objects from non-main threads. Unfortunately, this was not safe, mostly due to
SecurityOrigin's reliance on the SchemeRegistry.

This patch makes it safe to construct a SecurityOrigin on any thread A and use
it later on the same thread A. However, developers still need to call isolatedCopy()
if they want to pass such object to another thread B.

* page/SecurityOrigin.cpp:
(WebCore::SecurityOrigin::canDisplay const):
* page/SecurityOrigin.h:
* page/SecurityPolicy.cpp:
(WebCore::originAccessMapLock):
(WebCore::originAccessMap):
(WebCore::SecurityPolicy::isAccessWhiteListed):
(WebCore::SecurityPolicy::addOriginAccessWhitelistEntry):
(WebCore::SecurityPolicy::removeOriginAccessWhitelistEntry):
(WebCore::SecurityPolicy::resetOriginAccessWhitelists):
* platform/SchemeRegistry.cpp:
(WebCore::schemeRegistryLock):
(WebCore::allBuiltinSchemes):
(WebCore::builtinLocalURLSchemes):
(WebCore::localURLSchemes):
(WebCore::displayIsolatedURLSchemes):
(WebCore::builtinSecureSchemes):
(WebCore::secureSchemes):
(WebCore::builtinSchemesWithUniqueOrigins):
(WebCore::schemesWithUniqueOrigins):
(WebCore::builtinEmptyDocumentSchemes):
(WebCore::emptyDocumentSchemes):
(WebCore::schemesForbiddenFromDomainRelaxation):
(WebCore::builtinCanDisplayOnlyIfCanRequestSchemes):
(WebCore::canDisplayOnlyIfCanRequestSchemes):
(WebCore::notAllowingJavascriptURLsSchemes):
(WebCore::SchemeRegistry::registerURLSchemeAsLocal):
(WebCore::SchemeRegistry::removeURLSchemeRegisteredAsLocal):
(WebCore::schemesAllowingLocalStorageAccessInPrivateBrowsing):
(WebCore::schemesAllowingDatabaseAccessInPrivateBrowsing):
(WebCore::builtinCORSEnabledSchemes):
(WebCore::CORSEnabledSchemes):
(WebCore::ContentSecurityPolicyBypassingSchemes):
(WebCore::cachePartitioningSchemes):
(WebCore::serviceWorkerSchemes):
(WebCore::alwaysRevalidatedSchemes):
(WebCore::SchemeRegistry::shouldTreatURLSchemeAsLocal):
(WebCore::SchemeRegistry::registerURLSchemeAsNoAccess):
(WebCore::SchemeRegistry::shouldTreatURLSchemeAsNoAccess):
(WebCore::SchemeRegistry::registerURLSchemeAsDisplayIsolated):
(WebCore::SchemeRegistry::shouldTreatURLSchemeAsDisplayIsolated):
(WebCore::SchemeRegistry::registerURLSchemeAsSecure):
(WebCore::SchemeRegistry::shouldTreatURLSchemeAsSecure):
(WebCore::SchemeRegistry::canDisplayOnlyIfCanRequest):
(WebCore::SchemeRegistry::registerAsCanDisplayOnlyIfCanRequest):
(WebCore::SchemeRegistry::registerURLSchemeAsBypassingContentSecurityPolicy):
(WebCore::SchemeRegistry::removeURLSchemeRegisteredAsBypassingContentSecurityPolicy):
(WebCore::SchemeRegistry::schemeShouldBypassContentSecurityPolicy):
(WebCore::SchemeRegistry::registerURLSchemeAsCachePartitioned):
(WebCore::SchemeRegistry::shouldPartitionCacheForURLScheme):
(WebCore::SchemeRegistry::registerURLSchemeServiceWorkersCanHandle):
(WebCore::SchemeRegistry::canServiceWorkersHandleURLScheme):
(WebCore::SchemeRegistry::isServiceWorkerContainerCustomScheme):
* platform/SchemeRegistry.h:


Canonical link: https://commits.webkit.org/199815@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@230205 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
cdumez committed Apr 3, 2018
1 parent 276d268 commit 517ce500d7e95713e5506693596ae04c6ace5f90
@@ -1,3 +1,73 @@
2018-04-03 Chris Dumez <cdumez@apple.com>

Make SecurityOrigin safe to create and use from any thread
https://bugs.webkit.org/show_bug.cgi?id=184216

Reviewed by Youenn Fablet.

We found that we have a decent amount of code constructing and using SecurityOrigin
objects from non-main threads. Unfortunately, this was not safe, mostly due to
SecurityOrigin's reliance on the SchemeRegistry.

This patch makes it safe to construct a SecurityOrigin on any thread A and use
it later on the same thread A. However, developers still need to call isolatedCopy()
if they want to pass such object to another thread B.

* page/SecurityOrigin.cpp:
(WebCore::SecurityOrigin::canDisplay const):
* page/SecurityOrigin.h:
* page/SecurityPolicy.cpp:
(WebCore::originAccessMapLock):
(WebCore::originAccessMap):
(WebCore::SecurityPolicy::isAccessWhiteListed):
(WebCore::SecurityPolicy::addOriginAccessWhitelistEntry):
(WebCore::SecurityPolicy::removeOriginAccessWhitelistEntry):
(WebCore::SecurityPolicy::resetOriginAccessWhitelists):
* platform/SchemeRegistry.cpp:
(WebCore::schemeRegistryLock):
(WebCore::allBuiltinSchemes):
(WebCore::builtinLocalURLSchemes):
(WebCore::localURLSchemes):
(WebCore::displayIsolatedURLSchemes):
(WebCore::builtinSecureSchemes):
(WebCore::secureSchemes):
(WebCore::builtinSchemesWithUniqueOrigins):
(WebCore::schemesWithUniqueOrigins):
(WebCore::builtinEmptyDocumentSchemes):
(WebCore::emptyDocumentSchemes):
(WebCore::schemesForbiddenFromDomainRelaxation):
(WebCore::builtinCanDisplayOnlyIfCanRequestSchemes):
(WebCore::canDisplayOnlyIfCanRequestSchemes):
(WebCore::notAllowingJavascriptURLsSchemes):
(WebCore::SchemeRegistry::registerURLSchemeAsLocal):
(WebCore::SchemeRegistry::removeURLSchemeRegisteredAsLocal):
(WebCore::schemesAllowingLocalStorageAccessInPrivateBrowsing):
(WebCore::schemesAllowingDatabaseAccessInPrivateBrowsing):
(WebCore::builtinCORSEnabledSchemes):
(WebCore::CORSEnabledSchemes):
(WebCore::ContentSecurityPolicyBypassingSchemes):
(WebCore::cachePartitioningSchemes):
(WebCore::serviceWorkerSchemes):
(WebCore::alwaysRevalidatedSchemes):
(WebCore::SchemeRegistry::shouldTreatURLSchemeAsLocal):
(WebCore::SchemeRegistry::registerURLSchemeAsNoAccess):
(WebCore::SchemeRegistry::shouldTreatURLSchemeAsNoAccess):
(WebCore::SchemeRegistry::registerURLSchemeAsDisplayIsolated):
(WebCore::SchemeRegistry::shouldTreatURLSchemeAsDisplayIsolated):
(WebCore::SchemeRegistry::registerURLSchemeAsSecure):
(WebCore::SchemeRegistry::shouldTreatURLSchemeAsSecure):
(WebCore::SchemeRegistry::canDisplayOnlyIfCanRequest):
(WebCore::SchemeRegistry::registerAsCanDisplayOnlyIfCanRequest):
(WebCore::SchemeRegistry::registerURLSchemeAsBypassingContentSecurityPolicy):
(WebCore::SchemeRegistry::removeURLSchemeRegisteredAsBypassingContentSecurityPolicy):
(WebCore::SchemeRegistry::schemeShouldBypassContentSecurityPolicy):
(WebCore::SchemeRegistry::registerURLSchemeAsCachePartitioned):
(WebCore::SchemeRegistry::shouldPartitionCacheForURLScheme):
(WebCore::SchemeRegistry::registerURLSchemeServiceWorkersCanHandle):
(WebCore::SchemeRegistry::canServiceWorkersHandleURLScheme):
(WebCore::SchemeRegistry::isServiceWorkerContainerCustomScheme):
* platform/SchemeRegistry.h:

2018-04-03 Carlos Garcia Campos <cgarcia@igalia.com>

[SOUP] Stop using ResourceHandle to load GResources
@@ -123,10 +123,6 @@ static bool isLoopbackIPAddress(const String& host)
// https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy (Editor's Draft, 17 November 2016)
static bool shouldTreatAsPotentiallyTrustworthy(const String& protocol, const String& host)
{
// FIXME: despite the following SchemeRegistry functions using locks internally, we still
// have a potential thread-safety issue with the strings being passed in. This is because
// String::hash() will be called during lookup and it potentially modifies the String for
// caching the hash.
if (SchemeRegistry::shouldTreatURLSchemeAsSecure(protocol))
return true;

@@ -552,13 +548,6 @@ bool SecurityOrigin::isSameSchemeHostPort(const SecurityOrigin& other) const
return true;
}

URL SecurityOrigin::urlWithUniqueSecurityOrigin()
{
ASSERT(isMainThread());
static NeverDestroyed<URL> uniqueSecurityOriginURL(ParsedURLString, MAKE_STATIC_STRING_IMPL("data:,"));
return uniqueSecurityOriginURL;
}

bool SecurityOrigin::isLocalHostOrLoopbackIPAddress(const String& host)
{
if (isLoopbackIPAddress(host))
@@ -200,8 +200,6 @@ class SecurityOrigin : public ThreadSafeRefCounted<SecurityOrigin> {
// https://html.spec.whatwg.org/multipage/browsers.html#same-origin
WEBCORE_EXPORT bool isSameOriginAs(const SecurityOrigin&) const;

static URL urlWithUniqueSecurityOrigin();

bool isPotentiallyTrustworthy() const { return m_isPotentiallyTrustworthy; }

static bool isLocalHostOrLoopbackIPAddress(const String& host);
@@ -45,8 +45,15 @@ static SecurityPolicy::LocalLoadPolicy localLoadPolicy = SecurityPolicy::AllowLo
typedef Vector<OriginAccessEntry> OriginAccessWhiteList;
typedef HashMap<String, std::unique_ptr<OriginAccessWhiteList>> OriginAccessMap;

static Lock& originAccessMapLock()
{
static NeverDestroyed<Lock> lock;
return lock;
}

static OriginAccessMap& originAccessMap()
{
ASSERT(originAccessMapLock().isHeld());
static NeverDestroyed<OriginAccessMap> originAccessMap;
return originAccessMap;
}
@@ -146,6 +153,7 @@ bool SecurityPolicy::allowSubstituteDataAccessToLocal()

bool SecurityPolicy::isAccessWhiteListed(const SecurityOrigin* activeOrigin, const SecurityOrigin* targetOrigin)
{
Locker<Lock> locker(originAccessMapLock());
if (OriginAccessWhiteList* list = originAccessMap().get(activeOrigin->toString())) {
for (auto& entry : *list) {
if (entry.matchesOrigin(*targetOrigin))
@@ -163,12 +171,13 @@ bool SecurityPolicy::isAccessToURLWhiteListed(const SecurityOrigin* activeOrigin

void SecurityPolicy::addOriginAccessWhitelistEntry(const SecurityOrigin& sourceOrigin, const String& destinationProtocol, const String& destinationDomain, bool allowDestinationSubdomains)
{
ASSERT(isMainThread());
ASSERT(!sourceOrigin.isUnique());
if (sourceOrigin.isUnique())
return;

String sourceString = sourceOrigin.toString();

Locker<Lock> locker(originAccessMapLock());
OriginAccessMap::AddResult result = originAccessMap().add(sourceString, nullptr);
if (result.isNewEntry)
result.iterator->value = std::make_unique<OriginAccessWhiteList>();
@@ -179,12 +188,13 @@ void SecurityPolicy::addOriginAccessWhitelistEntry(const SecurityOrigin& sourceO

void SecurityPolicy::removeOriginAccessWhitelistEntry(const SecurityOrigin& sourceOrigin, const String& destinationProtocol, const String& destinationDomain, bool allowDestinationSubdomains)
{
ASSERT(isMainThread());
ASSERT(!sourceOrigin.isUnique());
if (sourceOrigin.isUnique())
return;

String sourceString = sourceOrigin.toString();

Locker<Lock> locker(originAccessMapLock());
OriginAccessMap& map = originAccessMap();
OriginAccessMap::iterator it = map.find(sourceString);
if (it == map.end())
@@ -201,7 +211,7 @@ void SecurityPolicy::removeOriginAccessWhitelistEntry(const SecurityOrigin& sour

void SecurityPolicy::resetOriginAccessWhitelists()
{
ASSERT(isMainThread());
Locker<Lock> locker(originAccessMapLock());
originAccessMap().clear();
}

0 comments on commit 517ce50

Please sign in to comment.