- Revision
- 228972
- Author
- [email protected]
- Date
- 2018-02-23 22:36:07 -0800 (Fri, 23 Feb 2018)
Log Message
Crash under SchemeRegistry::shouldTreatURLSchemeAsLocal(WTF::String const&)
https://bugs.webkit.org/show_bug.cgi?id=183066
<rdar://problem/37804111>
Reviewed by Ryosuke Niwa.
SecurityOrigin objects are constructed on various threads. However, someone added a
shouldTreatAsPotentiallyTrustworthy() call to the SecurityOrigin constructor which
was not thread safe. This is because this function relies on SchemeRegistry::shouldTreatURLSchemeAsSecure()
and SchemeRegistry::shouldTreatURLSchemeAsLocal() which were relying on global static HashMaps without
locks.
Update SecurityOrigin to initialize m_isPotentiallyTrustworthy lazily, to avoid paying
initialization cost in the constructor. This is only queries by SecurityContext::isSecureContext().
Make SchemeRegistry::shouldTreatURLSchemeAsLocal() and SchemeRegistry::shouldTreatURLSchemeAsSecure()
thread-safe, since they are needed to initialize SecurityOrigin::m_isPotentiallyTrustworthy from
various threads.
SchemeRegistry::shouldTreatURLSchemeAsSecure() is only called from SecurityOrigin (which requires
thread-safety), and getUserMedia() which is not hot code so the extra locking there should not
be an issue.
SchemeRegistry::shouldTreatURLSchemeAsLocal() is called from SecurityOrigin (which requires thread-
safety). It is also called from isQuickLookPreviewURL(), MHTMLArchive::create(), Page::userStyleSheetLocationChanged(),
isRemoteWebArchive() and HTMLPlugInImageElement. All these are not hot code so I do not think
we need a fast path.
* page/SecurityOrigin.cpp:
(WebCore::isLoopbackIPAddress):
(WebCore::shouldTreatAsPotentiallyTrustworthy):
(WebCore::SecurityOrigin::isPotentiallyTrustworthy const):
(WebCore::SecurityOrigin::isLocalHostOrLoopbackIPAddress):
* page/SecurityOrigin.h:
* platform/SchemeRegistry.cpp:
(WebCore::localURLSchemesLock):
(WebCore::localURLSchemes):
(WebCore::secureSchemesLock):
(WebCore::secureSchemes):
(WebCore::SchemeRegistry::registerURLSchemeAsLocal):
(WebCore::SchemeRegistry::removeURLSchemeRegisteredAsLocal):
(WebCore::SchemeRegistry::shouldTreatURLSchemeAsLocal):
(WebCore::SchemeRegistry::registerURLSchemeAsSecure):
(WebCore::SchemeRegistry::shouldTreatURLSchemeAsSecure):
* platform/SchemeRegistry.h:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (228971 => 228972)
--- trunk/Source/WebCore/ChangeLog 2018-02-24 02:01:36 UTC (rev 228971)
+++ trunk/Source/WebCore/ChangeLog 2018-02-24 06:36:07 UTC (rev 228972)
@@ -1,3 +1,51 @@
+2018-02-23 Chris Dumez <[email protected]>
+
+ Crash under SchemeRegistry::shouldTreatURLSchemeAsLocal(WTF::String const&)
+ https://bugs.webkit.org/show_bug.cgi?id=183066
+ <rdar://problem/37804111>
+
+ Reviewed by Ryosuke Niwa.
+
+ SecurityOrigin objects are constructed on various threads. However, someone added a
+ shouldTreatAsPotentiallyTrustworthy() call to the SecurityOrigin constructor which
+ was not thread safe. This is because this function relies on SchemeRegistry::shouldTreatURLSchemeAsSecure()
+ and SchemeRegistry::shouldTreatURLSchemeAsLocal() which were relying on global static HashMaps without
+ locks.
+
+ Update SecurityOrigin to initialize m_isPotentiallyTrustworthy lazily, to avoid paying
+ initialization cost in the constructor. This is only queries by SecurityContext::isSecureContext().
+
+ Make SchemeRegistry::shouldTreatURLSchemeAsLocal() and SchemeRegistry::shouldTreatURLSchemeAsSecure()
+ thread-safe, since they are needed to initialize SecurityOrigin::m_isPotentiallyTrustworthy from
+ various threads.
+
+ SchemeRegistry::shouldTreatURLSchemeAsSecure() is only called from SecurityOrigin (which requires
+ thread-safety), and getUserMedia() which is not hot code so the extra locking there should not
+ be an issue.
+
+ SchemeRegistry::shouldTreatURLSchemeAsLocal() is called from SecurityOrigin (which requires thread-
+ safety). It is also called from isQuickLookPreviewURL(), MHTMLArchive::create(), Page::userStyleSheetLocationChanged(),
+ isRemoteWebArchive() and HTMLPlugInImageElement. All these are not hot code so I do not think
+ we need a fast path.
+
+ * page/SecurityOrigin.cpp:
+ (WebCore::isLoopbackIPAddress):
+ (WebCore::shouldTreatAsPotentiallyTrustworthy):
+ (WebCore::SecurityOrigin::isPotentiallyTrustworthy const):
+ (WebCore::SecurityOrigin::isLocalHostOrLoopbackIPAddress):
+ * page/SecurityOrigin.h:
+ * platform/SchemeRegistry.cpp:
+ (WebCore::localURLSchemesLock):
+ (WebCore::localURLSchemes):
+ (WebCore::secureSchemesLock):
+ (WebCore::secureSchemes):
+ (WebCore::SchemeRegistry::registerURLSchemeAsLocal):
+ (WebCore::SchemeRegistry::removeURLSchemeRegisteredAsLocal):
+ (WebCore::SchemeRegistry::shouldTreatURLSchemeAsLocal):
+ (WebCore::SchemeRegistry::registerURLSchemeAsSecure):
+ (WebCore::SchemeRegistry::shouldTreatURLSchemeAsSecure):
+ * platform/SchemeRegistry.h:
+
2018-02-23 Christopher Reid <[email protected]>
[Curl] Cookie Database files are wrongfully getting deleted when the database is opened
Modified: trunk/Source/WebCore/Modules/mediastream/UserMediaRequest.cpp (228971 => 228972)
--- trunk/Source/WebCore/Modules/mediastream/UserMediaRequest.cpp 2018-02-24 02:01:36 UTC (rev 228971)
+++ trunk/Source/WebCore/Modules/mediastream/UserMediaRequest.cpp 2018-02-24 06:36:07 UTC (rev 228972)
@@ -87,7 +87,7 @@
static bool isSecure(DocumentLoader& documentLoader)
{
auto& response = documentLoader.response();
- if (SecurityOrigin::isLocalHostOrLoopbackIPAddress(documentLoader.response().url()))
+ if (SecurityOrigin::isLocalHostOrLoopbackIPAddress(documentLoader.response().url().host()))
return true;
return SchemeRegistry::shouldTreatURLSchemeAsSecure(response.url().protocol().toStringWithoutCopying())
&& response.certificateInfo()
Modified: trunk/Source/WebCore/page/SecurityOrigin.cpp (228971 => 228972)
--- trunk/Source/WebCore/page/SecurityOrigin.cpp 2018-02-24 02:01:36 UTC (rev 228971)
+++ trunk/Source/WebCore/page/SecurityOrigin.cpp 2018-02-24 06:36:07 UTC (rev 228972)
@@ -99,11 +99,9 @@
return false;
}
-static bool isLoopbackIPAddress(const URL& url)
+static bool isLoopbackIPAddress(const String& host)
{
// The IPv6 loopback address is 0:0:0:0:0:0:0:1, which compresses to ::1.
- ASSERT(url.isValid());
- auto host = url.host();
if (host == "[::1]")
return true;
@@ -123,23 +121,29 @@
}
// https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy (Editor's Draft, 17 November 2016)
-bool shouldTreatAsPotentiallyTrustworthy(const URL& url)
+static bool shouldTreatAsPotentiallyTrustworthy(const String& protocol, const String& host)
{
- if (!url.isValid())
- return false;
-
- if (SchemeRegistry::shouldTreatURLSchemeAsSecure(url.protocol().toStringWithoutCopying()))
+ // 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;
- if (SecurityOrigin::isLocalHostOrLoopbackIPAddress(url))
+ if (SecurityOrigin::isLocalHostOrLoopbackIPAddress(host))
return true;
- if (SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol().toStringWithoutCopying()))
+ if (SchemeRegistry::shouldTreatURLSchemeAsLocal(protocol))
return true;
return false;
}
+bool shouldTreatAsPotentiallyTrustworthy(const URL& url)
+{
+ return shouldTreatAsPotentiallyTrustworthy(url.protocol().toStringWithoutCopying(), url.host());
+}
+
SecurityOrigin::SecurityOrigin(const URL& url)
: m_protocol { url.protocol().isNull() ? emptyString() : url.protocol().toString().convertToASCIILowercase() }
, m_host { url.host().isNull() ? emptyString() : url.host().convertToASCIILowercase() }
@@ -157,7 +161,8 @@
if (m_canLoadLocalResources)
m_filePath = url.fileSystemPath(); // In case enforceFilePathSeparation() is called.
- m_isPotentiallyTrustworthy = shouldTreatAsPotentiallyTrustworthy(url);
+ if (!url.isValid())
+ m_isPotentiallyTrustworthy = IsPotentiallyTrustworthy::No;
}
SecurityOrigin::SecurityOrigin()
@@ -165,7 +170,7 @@
, m_host { emptyString() }
, m_domain { emptyString() }
, m_isUnique { true }
- , m_isPotentiallyTrustworthy { true }
+ , m_isPotentiallyTrustworthy { IsPotentiallyTrustworthy::Yes }
{
}
@@ -218,6 +223,15 @@
m_domain = newDomain.convertToASCIILowercase();
}
+bool SecurityOrigin::isPotentiallyTrustworthy() const
+{
+ // This code is using an enum instead of an std::optional for thread-safety. Worst case scenario, several thread will read
+ // 'Unknown' value concurrently and they'll all call shouldTreatAsPotentiallyTrustworthy() and get the same result.
+ if (m_isPotentiallyTrustworthy == IsPotentiallyTrustworthy::Unknown)
+ m_isPotentiallyTrustworthy = shouldTreatAsPotentiallyTrustworthy(m_protocol, m_host) ? IsPotentiallyTrustworthy::Yes : IsPotentiallyTrustworthy::No;
+ return m_isPotentiallyTrustworthy == IsPotentiallyTrustworthy::Yes;
+}
+
bool SecurityOrigin::isSecure(const URL& url)
{
// Invalid URLs are secure, as are URLs which have a secure protocol.
@@ -583,13 +597,13 @@
return uniqueSecurityOriginURL;
}
-bool SecurityOrigin::isLocalHostOrLoopbackIPAddress(const URL& url)
+bool SecurityOrigin::isLocalHostOrLoopbackIPAddress(const String& host)
{
- if (isLoopbackIPAddress(url))
+ if (isLoopbackIPAddress(host))
return true;
// FIXME: Ensure that localhost resolves to the loopback address.
- if (equalLettersIgnoringASCIICase(url.host(), "localhost"))
+ if (equalLettersIgnoringASCIICase(host, "localhost"))
return true;
return false;
Modified: trunk/Source/WebCore/page/SecurityOrigin.h (228971 => 228972)
--- trunk/Source/WebCore/page/SecurityOrigin.h 2018-02-24 02:01:36 UTC (rev 228971)
+++ trunk/Source/WebCore/page/SecurityOrigin.h 2018-02-24 06:36:07 UTC (rev 228972)
@@ -201,9 +201,9 @@
static URL urlWithUniqueSecurityOrigin();
- bool isPotentiallyTrustworthy() const { return m_isPotentiallyTrustworthy; }
+ WEBCORE_EXPORT bool isPotentiallyTrustworthy() const;
- static bool isLocalHostOrLoopbackIPAddress(const URL&);
+ static bool isLocalHostOrLoopbackIPAddress(const String& host);
private:
SecurityOrigin();
@@ -232,7 +232,8 @@
StorageBlockingPolicy m_storageBlockingPolicy { AllowAllStorage };
bool m_enforcesFilePathSeparation { false };
bool m_needsStorageAccessFromFileURLsQuirk { false };
- bool m_isPotentiallyTrustworthy { false };
+ enum class IsPotentiallyTrustworthy : uint8_t { No, Yes, Unknown };
+ mutable IsPotentiallyTrustworthy m_isPotentiallyTrustworthy { IsPotentiallyTrustworthy::Unknown };
};
bool shouldTreatAsPotentiallyTrustworthy(const URL&);
Modified: trunk/Source/WebCore/platform/SchemeRegistry.cpp (228971 => 228972)
--- trunk/Source/WebCore/platform/SchemeRegistry.cpp 2018-02-24 02:01:36 UTC (rev 228971)
+++ trunk/Source/WebCore/platform/SchemeRegistry.cpp 2018-02-24 06:36:07 UTC (rev 228972)
@@ -113,8 +113,15 @@
return schemes;
}
+static Lock& localURLSchemesLock()
+{
+ static NeverDestroyed<Lock> lock;
+ return lock;
+}
+
static URLSchemesMap& localURLSchemes()
{
+ ASSERT(localURLSchemesLock().isHeld());
static NeverDestroyed<URLSchemesMap> localSchemes = builtinLocalURLSchemes();
return localSchemes;
}
@@ -139,8 +146,15 @@
return schemes;
}
+static Lock& secureSchemesLock()
+{
+ static NeverDestroyed<Lock> lock;
+ return lock;
+}
+
static URLSchemesMap& secureSchemes()
{
+ ASSERT(secureSchemesLock().isHeld());
static auto secureSchemes = makeNeverDestroyedSchemeSet(builtinSecureSchemes);
return secureSchemes;
}
@@ -201,11 +215,16 @@
void SchemeRegistry::registerURLSchemeAsLocal(const String& scheme)
{
+ if (scheme.isNull())
+ return;
+
+ Locker<Lock> locker(localURLSchemesLock());
localURLSchemes().add(scheme);
}
void SchemeRegistry::removeURLSchemeRegisteredAsLocal(const String& scheme)
{
+ Locker<Lock> locker(localURLSchemesLock());
if (builtinLocalURLSchemes().contains(scheme))
return;
@@ -212,11 +231,6 @@
localURLSchemes().remove(scheme);
}
-const URLSchemesMap& SchemeRegistry::localSchemes()
-{
- return localURLSchemes();
-}
-
static URLSchemesMap& schemesAllowingLocalStorageAccessInPrivateBrowsing()
{
static NeverDestroyed<URLSchemesMap> schemesAllowingLocalStorageAccessInPrivateBrowsing;
@@ -275,7 +289,11 @@
bool SchemeRegistry::shouldTreatURLSchemeAsLocal(const String& scheme)
{
- return !scheme.isNull() && localURLSchemes().contains(scheme);
+ if (scheme.isNull())
+ return false;
+
+ Locker<Lock> locker(localURLSchemesLock());
+ return localURLSchemes().contains(scheme);
}
void SchemeRegistry::registerURLSchemeAsNoAccess(const String& scheme)
@@ -306,12 +324,18 @@
{
if (scheme.isNull())
return;
+
+ Locker<Lock> locker(secureSchemesLock());
secureSchemes().add(scheme);
}
bool SchemeRegistry::shouldTreatURLSchemeAsSecure(const String& scheme)
{
- return !scheme.isNull() && secureSchemes().contains(scheme);
+ if (scheme.isNull())
+ return false;
+
+ Locker<Lock> locker(secureSchemesLock());
+ return secureSchemes().contains(scheme);
}
void SchemeRegistry::registerURLSchemeAsEmptyDocument(const String& scheme)
Modified: trunk/Source/WebCore/platform/SchemeRegistry.h (228971 => 228972)
--- trunk/Source/WebCore/platform/SchemeRegistry.h 2018-02-24 02:01:36 UTC (rev 228971)
+++ trunk/Source/WebCore/platform/SchemeRegistry.h 2018-02-24 06:36:07 UTC (rev 228972)
@@ -39,7 +39,6 @@
public:
WEBCORE_EXPORT static void registerURLSchemeAsLocal(const String&);
static void removeURLSchemeRegisteredAsLocal(const String&);
- static const URLSchemesMap& localSchemes();
WEBCORE_EXPORT static bool shouldTreatURLSchemeAsLocal(const String&);
WEBCORE_EXPORT static bool isBuiltinScheme(const String&);
