Changeset 228972 in webkit
- Timestamp:
- Feb 23, 2018 10:36:07 PM (6 years ago)
- Location:
- trunk/Source/WebCore
- Files:
-
- 6 edited
Legend:
- Unmodified
- Added
- Removed
-
trunk/Source/WebCore/ChangeLog
r228971 r228972 1 2018-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 1 49 2018-02-23 Christopher Reid <chris.reid@sony.com> 2 50 -
trunk/Source/WebCore/Modules/mediastream/UserMediaRequest.cpp
r226804 r228972 88 88 { 89 89 auto& response = documentLoader.response(); 90 if (SecurityOrigin::isLocalHostOrLoopbackIPAddress(documentLoader.response().url() ))90 if (SecurityOrigin::isLocalHostOrLoopbackIPAddress(documentLoader.response().url().host())) 91 91 return true; 92 92 return SchemeRegistry::shouldTreatURLSchemeAsSecure(response.url().protocol().toStringWithoutCopying()) -
trunk/Source/WebCore/page/SecurityOrigin.cpp
r228149 r228972 100 100 } 101 101 102 static bool isLoopbackIPAddress(const URL& url)102 static bool isLoopbackIPAddress(const String& host) 103 103 { 104 104 // 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();107 105 if (host == "[::1]") 108 106 return true; … … 124 122 125 123 // https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy (Editor's Draft, 17 November 2016) 124 static 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 126 142 bool shouldTreatAsPotentiallyTrustworthy(const URL& url) 127 143 { 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()); 141 145 } 142 146 … … 158 162 m_filePath = url.fileSystemPath(); // In case enforceFilePathSeparation() is called. 159 163 160 m_isPotentiallyTrustworthy = shouldTreatAsPotentiallyTrustworthy(url); 164 if (!url.isValid()) 165 m_isPotentiallyTrustworthy = IsPotentiallyTrustworthy::No; 161 166 } 162 167 … … 166 171 , m_domain { emptyString() } 167 172 , m_isUnique { true } 168 , m_isPotentiallyTrustworthy { true}173 , m_isPotentiallyTrustworthy { IsPotentiallyTrustworthy::Yes } 169 174 { 170 175 } … … 217 222 m_domainWasSetInDOM = true; 218 223 m_domain = newDomain.convertToASCIILowercase(); 224 } 225 226 bool 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; 219 233 } 220 234 … … 584 598 } 585 599 586 bool SecurityOrigin::isLocalHostOrLoopbackIPAddress(const URL& url)587 { 588 if (isLoopbackIPAddress( url))600 bool SecurityOrigin::isLocalHostOrLoopbackIPAddress(const String& host) 601 { 602 if (isLoopbackIPAddress(host)) 589 603 return true; 590 604 591 605 // FIXME: Ensure that localhost resolves to the loopback address. 592 if (equalLettersIgnoringASCIICase( url.host(), "localhost"))606 if (equalLettersIgnoringASCIICase(host, "localhost")) 593 607 return true; 594 608 -
trunk/Source/WebCore/page/SecurityOrigin.h
r228149 r228972 202 202 static URL urlWithUniqueSecurityOrigin(); 203 203 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); 207 207 208 208 private: … … 233 233 bool m_enforcesFilePathSeparation { false }; 234 234 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 }; 236 237 }; 237 238 -
trunk/Source/WebCore/platform/SchemeRegistry.cpp
r226088 r228972 114 114 } 115 115 116 static Lock& localURLSchemesLock() 117 { 118 static NeverDestroyed<Lock> lock; 119 return lock; 120 } 121 116 122 static URLSchemesMap& localURLSchemes() 117 123 { 124 ASSERT(localURLSchemesLock().isHeld()); 118 125 static NeverDestroyed<URLSchemesMap> localSchemes = builtinLocalURLSchemes(); 119 126 return localSchemes; … … 140 147 } 141 148 149 static Lock& secureSchemesLock() 150 { 151 static NeverDestroyed<Lock> lock; 152 return lock; 153 } 154 142 155 static URLSchemesMap& secureSchemes() 143 156 { 157 ASSERT(secureSchemesLock().isHeld()); 144 158 static auto secureSchemes = makeNeverDestroyedSchemeSet(builtinSecureSchemes); 145 159 return secureSchemes; … … 202 216 void SchemeRegistry::registerURLSchemeAsLocal(const String& scheme) 203 217 { 218 if (scheme.isNull()) 219 return; 220 221 Locker<Lock> locker(localURLSchemesLock()); 204 222 localURLSchemes().add(scheme); 205 223 } … … 207 225 void SchemeRegistry::removeURLSchemeRegisteredAsLocal(const String& scheme) 208 226 { 227 Locker<Lock> locker(localURLSchemesLock()); 209 228 if (builtinLocalURLSchemes().contains(scheme)) 210 229 return; 211 230 212 231 localURLSchemes().remove(scheme); 213 }214 215 const URLSchemesMap& SchemeRegistry::localSchemes()216 {217 return localURLSchemes();218 232 } 219 233 … … 276 290 bool SchemeRegistry::shouldTreatURLSchemeAsLocal(const String& scheme) 277 291 { 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); 279 297 } 280 298 … … 307 325 if (scheme.isNull()) 308 326 return; 327 328 Locker<Lock> locker(secureSchemesLock()); 309 329 secureSchemes().add(scheme); 310 330 } … … 312 332 bool SchemeRegistry::shouldTreatURLSchemeAsSecure(const String& scheme) 313 333 { 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); 315 339 } 316 340 -
trunk/Source/WebCore/platform/SchemeRegistry.h
r226088 r228972 40 40 WEBCORE_EXPORT static void registerURLSchemeAsLocal(const String&); 41 41 static void removeURLSchemeRegisteredAsLocal(const String&); 42 static const URLSchemesMap& localSchemes();43 42 44 43 WEBCORE_EXPORT static bool shouldTreatURLSchemeAsLocal(const String&);
Note: See TracChangeset
for help on using the changeset viewer.