Changeset 228972 in webkit


Ignore:
Timestamp:
Feb 23, 2018 10:36:07 PM (6 years ago)
Author:
Chris Dumez
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:
Location:
trunk/Source/WebCore
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r228971 r228972  
     12018-02-23  Chris Dumez  <cdumez@apple.com>
     2
     3        Crash under SchemeRegistry::shouldTreatURLSchemeAsLocal(WTF::String const&)
     4        https://bugs.webkit.org/show_bug.cgi?id=183066
     5        <rdar://problem/37804111>
     6
     7        Reviewed by Ryosuke Niwa.
     8
     9        SecurityOrigin objects are constructed on various threads. However, someone added a
     10        shouldTreatAsPotentiallyTrustworthy() call to the SecurityOrigin constructor which
     11        was not thread safe. This is because this function relies on SchemeRegistry::shouldTreatURLSchemeAsSecure()
     12        and SchemeRegistry::shouldTreatURLSchemeAsLocal() which were relying on global static HashMaps without
     13        locks.
     14
     15        Update SecurityOrigin to initialize m_isPotentiallyTrustworthy lazily, to avoid paying
     16        initialization cost in the constructor. This is only queries by SecurityContext::isSecureContext().
     17
     18        Make SchemeRegistry::shouldTreatURLSchemeAsLocal() and SchemeRegistry::shouldTreatURLSchemeAsSecure()
     19        thread-safe, since they are needed to initialize SecurityOrigin::m_isPotentiallyTrustworthy from
     20        various threads.
     21
     22        SchemeRegistry::shouldTreatURLSchemeAsSecure() is only called from SecurityOrigin (which requires
     23        thread-safety), and getUserMedia() which is not hot code so the extra locking there should not
     24        be an issue.
     25
     26        SchemeRegistry::shouldTreatURLSchemeAsLocal() is called from SecurityOrigin (which requires thread-
     27        safety). It is also called from isQuickLookPreviewURL(), MHTMLArchive::create(), Page::userStyleSheetLocationChanged(),
     28        isRemoteWebArchive() and HTMLPlugInImageElement. All these are not hot code so I do not think
     29        we need a fast path.
     30
     31        * page/SecurityOrigin.cpp:
     32        (WebCore::isLoopbackIPAddress):
     33        (WebCore::shouldTreatAsPotentiallyTrustworthy):
     34        (WebCore::SecurityOrigin::isPotentiallyTrustworthy const):
     35        (WebCore::SecurityOrigin::isLocalHostOrLoopbackIPAddress):
     36        * page/SecurityOrigin.h:
     37        * platform/SchemeRegistry.cpp:
     38        (WebCore::localURLSchemesLock):
     39        (WebCore::localURLSchemes):
     40        (WebCore::secureSchemesLock):
     41        (WebCore::secureSchemes):
     42        (WebCore::SchemeRegistry::registerURLSchemeAsLocal):
     43        (WebCore::SchemeRegistry::removeURLSchemeRegisteredAsLocal):
     44        (WebCore::SchemeRegistry::shouldTreatURLSchemeAsLocal):
     45        (WebCore::SchemeRegistry::registerURLSchemeAsSecure):
     46        (WebCore::SchemeRegistry::shouldTreatURLSchemeAsSecure):
     47        * platform/SchemeRegistry.h:
     48
    1492018-02-23  Christopher Reid  <chris.reid@sony.com>
    250

  • trunk/Source/WebCore/Modules/mediastream/UserMediaRequest.cpp

    r226804 r228972  
    8888{
    8989    auto& response = documentLoader.response();
    90     if (SecurityOrigin::isLocalHostOrLoopbackIPAddress(documentLoader.response().url()))
     90    if (SecurityOrigin::isLocalHostOrLoopbackIPAddress(documentLoader.response().url().host()))
    9191        return true;
    9292    return SchemeRegistry::shouldTreatURLSchemeAsSecure(response.url().protocol().toStringWithoutCopying())

  • trunk/Source/WebCore/page/SecurityOrigin.cpp

    r228149 r228972  
    100100}
    101101
    102 static bool isLoopbackIPAddress(const URL& url)
     102static bool isLoopbackIPAddress(const String& host)
    103103{
    104104    // The IPv6 loopback address is 0:0:0:0:0:0:0:1, which compresses to ::1.
    105     ASSERT(url.isValid());
    106     auto host = url.host();
    107105    if (host == "[::1]")
    108106        return true;
    … …  
    124122
    125123// https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy (Editor's Draft, 17 November 2016)
     124static bool shouldTreatAsPotentiallyTrustworthy(const String& protocol, const String& host)
     125{
     126    // FIXME: despite the following SchemeRegistry functions using locks internally, we still
     127    // have a potential thread-safety issue with the strings being passed in. This is because
     128    // String::hash() will be called during lookup and it potentially modifies the String for
     129    // caching the hash.
     130    if (SchemeRegistry::shouldTreatURLSchemeAsSecure(protocol))
     131        return true;
     132
     133    if (SecurityOrigin::isLocalHostOrLoopbackIPAddress(host))
     134        return true;
     135
     136    if (SchemeRegistry::shouldTreatURLSchemeAsLocal(protocol))
     137        return true;
     138
     139    return false;
     140}
     141
    126142bool shouldTreatAsPotentiallyTrustworthy(const URL& url)
    127143{
    128     if (!url.isValid())
    129         return false;
    130 
    131     if (SchemeRegistry::shouldTreatURLSchemeAsSecure(url.protocol().toStringWithoutCopying()))
    132         return true;
    133 
    134     if (SecurityOrigin::isLocalHostOrLoopbackIPAddress(url))
    135         return true;
    136 
    137     if (SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol().toStringWithoutCopying()))
    138         return true;
    139 
    140     return false;
     144    return shouldTreatAsPotentiallyTrustworthy(url.protocol().toStringWithoutCopying(), url.host());
    141145}
    142146
    … …  
    158162        m_filePath = url.fileSystemPath(); // In case enforceFilePathSeparation() is called.
    159163
    160     m_isPotentiallyTrustworthy = shouldTreatAsPotentiallyTrustworthy(url);
     164    if (!url.isValid())
     165        m_isPotentiallyTrustworthy = IsPotentiallyTrustworthy::No;
    161166}
    162167
    … …  
    166171    , m_domain { emptyString() }
    167172    , m_isUnique { true }
    168     , m_isPotentiallyTrustworthy { true }
     173    , m_isPotentiallyTrustworthy { IsPotentiallyTrustworthy::Yes }
    169174{
    170175}
    … …  
    217222    m_domainWasSetInDOM = true;
    218223    m_domain = newDomain.convertToASCIILowercase();
     224}
     225
     226bool SecurityOrigin::isPotentiallyTrustworthy() const
     227{
     228    // This code is using an enum instead of an std::optional for thread-safety. Worst case scenario, several thread will read
     229    // 'Unknown' value concurrently and they'll all call shouldTreatAsPotentiallyTrustworthy() and get the same result.
     230    if (m_isPotentiallyTrustworthy == IsPotentiallyTrustworthy::Unknown)
     231        m_isPotentiallyTrustworthy = shouldTreatAsPotentiallyTrustworthy(m_protocol, m_host) ? IsPotentiallyTrustworthy::Yes : IsPotentiallyTrustworthy::No;
     232    return m_isPotentiallyTrustworthy == IsPotentiallyTrustworthy::Yes;
    219233}
    220234
    … …  
    584598}
    585599
    586 bool SecurityOrigin::isLocalHostOrLoopbackIPAddress(const URL& url)
    587 {
    588     if (isLoopbackIPAddress(url))
     600bool SecurityOrigin::isLocalHostOrLoopbackIPAddress(const String& host)
     601{
     602    if (isLoopbackIPAddress(host))
    589603        return true;
    590604
    591605    // FIXME: Ensure that localhost resolves to the loopback address.
    592     if (equalLettersIgnoringASCIICase(url.host(), "localhost"))
     606    if (equalLettersIgnoringASCIICase(host, "localhost"))
    593607        return true;
    594608

  • trunk/Source/WebCore/page/SecurityOrigin.h

    r228149 r228972  
    202202    static URL urlWithUniqueSecurityOrigin();
    203203
    204     bool isPotentiallyTrustworthy() const { return m_isPotentiallyTrustworthy; }
    205 
    206     static bool isLocalHostOrLoopbackIPAddress(const URL&);
     204    WEBCORE_EXPORT bool isPotentiallyTrustworthy() const;
     205
     206    static bool isLocalHostOrLoopbackIPAddress(const String& host);
    207207
    208208private:
    … …  
    233233    bool m_enforcesFilePathSeparation { false };
    234234    bool m_needsStorageAccessFromFileURLsQuirk { false };
    235     bool m_isPotentiallyTrustworthy { false };
     235    enum class IsPotentiallyTrustworthy : uint8_t { No, Yes, Unknown };
     236    mutable IsPotentiallyTrustworthy m_isPotentiallyTrustworthy { IsPotentiallyTrustworthy::Unknown };
    236237};
    237238

  • trunk/Source/WebCore/platform/SchemeRegistry.cpp

    r226088 r228972  
    114114}
    115115
     116static Lock& localURLSchemesLock()
     117{
     118    static NeverDestroyed<Lock> lock;
     119    return lock;
     120}
     121
    116122static URLSchemesMap& localURLSchemes()
    117123{
     124    ASSERT(localURLSchemesLock().isHeld());
    118125    static NeverDestroyed<URLSchemesMap> localSchemes = builtinLocalURLSchemes();
    119126    return localSchemes;
    … …  
    140147}
    141148
     149static Lock& secureSchemesLock()
     150{
     151    static NeverDestroyed<Lock> lock;
     152    return lock;
     153}
     154
    142155static URLSchemesMap& secureSchemes()
    143156{
     157    ASSERT(secureSchemesLock().isHeld());
    144158    static auto secureSchemes = makeNeverDestroyedSchemeSet(builtinSecureSchemes);
    145159    return secureSchemes;
    … …  
    202216void SchemeRegistry::registerURLSchemeAsLocal(const String& scheme)
    203217{
     218    if (scheme.isNull())
     219        return;
     220
     221    Locker<Lock> locker(localURLSchemesLock());
    204222    localURLSchemes().add(scheme);
    205223}
    … …  
    207225void SchemeRegistry::removeURLSchemeRegisteredAsLocal(const String& scheme)
    208226{
     227    Locker<Lock> locker(localURLSchemesLock());
    209228    if (builtinLocalURLSchemes().contains(scheme))
    210229        return;
    211230
    212231    localURLSchemes().remove(scheme);
    213 }
    214 
    215 const URLSchemesMap& SchemeRegistry::localSchemes()
    216 {
    217     return localURLSchemes();
    218232}
    219233
    … …  
    276290bool SchemeRegistry::shouldTreatURLSchemeAsLocal(const String& scheme)
    277291{
    278     return !scheme.isNull() && localURLSchemes().contains(scheme);
     292    if (scheme.isNull())
     293        return false;
     294
     295    Locker<Lock> locker(localURLSchemesLock());
     296    return localURLSchemes().contains(scheme);
    279297}
    280298
    … …  
    307325    if (scheme.isNull())
    308326        return;
     327
     328    Locker<Lock> locker(secureSchemesLock());
    309329    secureSchemes().add(scheme);
    310330}
    … …  
    312332bool SchemeRegistry::shouldTreatURLSchemeAsSecure(const String& scheme)
    313333{
    314     return !scheme.isNull() && secureSchemes().contains(scheme);
     334    if (scheme.isNull())
     335        return false;
     336
     337    Locker<Lock> locker(secureSchemesLock());
     338    return secureSchemes().contains(scheme);
    315339}
    316340

  • trunk/Source/WebCore/platform/SchemeRegistry.h

    r226088 r228972  
    4040    WEBCORE_EXPORT static void registerURLSchemeAsLocal(const String&);
    4141    static void removeURLSchemeRegisteredAsLocal(const String&);
    42     static const URLSchemesMap& localSchemes();
    4342
    4443    WEBCORE_EXPORT static bool shouldTreatURLSchemeAsLocal(const String&);
Note: See TracChangeset for help on using the changeset viewer.