Title: [228972] trunk/Source/WebCore
Revision
228972
Author
cdu...@apple.com
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  <cdu...@apple.com>
+
+        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  <chris.r...@sony.com>
 
         [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&);
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to