- Revision
- 260440
- Author
- [email protected]
- Date
- 2020-04-21 10:30:19 -0700 (Tue, 21 Apr 2020)
Log Message
[IPC hardening] ShareableResource::create() should validate its parameters
<https://webkit.org/b/210779>
<rdar://problem/60887693>
Reviewed by Chris Dumez.
* NetworkProcess/cache/NetworkCache.cpp:
(WebKit::NetworkCache::Cache::store):
* NetworkProcess/cache/NetworkCacheEntry.cpp:
(WebKit::NetworkCache::Entry::initializeShareableResourceHandleFromStorageRecord const):
* WebProcess/WebPage/ios/WebPageIOS.mm:
(WebKit::WebPage::didFinishLoadForQuickLookDocumentInMainFrame):
- Handle nullptr returned from ShareableResource::create().
* Shared/ShareableResource.cpp:
(WebKit::ShareableResource::create):
- Change to return RefPtr<>.
- Validate parameters before calling constructor. Return
nullptr on failure.
(WebKit::ShareableResource::ShareableResource):
- Remove assert and comment since checks are now done in
ShareableResource::create().
* Shared/ShareableResource.h:
(WebKit::ShareableResource::create):
- Change to return RefPtr<>.
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (260439 => 260440)
--- trunk/Source/WebKit/ChangeLog 2020-04-21 17:28:37 UTC (rev 260439)
+++ trunk/Source/WebKit/ChangeLog 2020-04-21 17:30:19 UTC (rev 260440)
@@ -1,3 +1,31 @@
+2020-04-21 David Kilzer <[email protected]>
+
+ [IPC hardening] ShareableResource::create() should validate its parameters
+ <https://webkit.org/b/210779>
+ <rdar://problem/60887693>
+
+ Reviewed by Chris Dumez.
+
+ * NetworkProcess/cache/NetworkCache.cpp:
+ (WebKit::NetworkCache::Cache::store):
+ * NetworkProcess/cache/NetworkCacheEntry.cpp:
+ (WebKit::NetworkCache::Entry::initializeShareableResourceHandleFromStorageRecord const):
+ * WebProcess/WebPage/ios/WebPageIOS.mm:
+ (WebKit::WebPage::didFinishLoadForQuickLookDocumentInMainFrame):
+ - Handle nullptr returned from ShareableResource::create().
+
+ * Shared/ShareableResource.cpp:
+ (WebKit::ShareableResource::create):
+ - Change to return RefPtr<>.
+ - Validate parameters before calling constructor. Return
+ nullptr on failure.
+ (WebKit::ShareableResource::ShareableResource):
+ - Remove assert and comment since checks are now done in
+ ShareableResource::create().
+ * Shared/ShareableResource.h:
+ (WebKit::ShareableResource::create):
+ - Change to return RefPtr<>.
+
2020-04-21 Wenson Hsieh <[email protected]>
[Cocoa] Tighten up some more IPC message checks in WebPasteboardProxy
Modified: trunk/Source/WebKit/NetworkProcess/cache/NetworkCache.cpp (260439 => 260440)
--- trunk/Source/WebKit/NetworkProcess/cache/NetworkCache.cpp 2020-04-21 17:28:37 UTC (rev 260439)
+++ trunk/Source/WebKit/NetworkProcess/cache/NetworkCache.cpp 2020-04-21 17:30:19 UTC (rev 260440)
@@ -493,7 +493,11 @@
#if ENABLE(SHAREABLE_RESOURCE)
if (auto sharedMemory = bodyData.tryCreateSharedMemory()) {
mappedBody.shareableResource = ShareableResource::create(sharedMemory.releaseNonNull(), 0, bodyData.size());
- ASSERT(mappedBody.shareableResource);
+ if (!mappedBody.shareableResource) {
+ if (completionHandler)
+ completionHandler(mappedBody);
+ return;
+ }
mappedBody.shareableResource->createHandle(mappedBody.shareableResourceHandle);
}
#endif
Modified: trunk/Source/WebKit/NetworkProcess/cache/NetworkCacheEntry.cpp (260439 => 260440)
--- trunk/Source/WebKit/NetworkProcess/cache/NetworkCacheEntry.cpp 2020-04-21 17:28:37 UTC (rev 260439)
+++ trunk/Source/WebKit/NetworkProcess/cache/NetworkCacheEntry.cpp 2020-04-21 17:30:19 UTC (rev 260440)
@@ -176,6 +176,8 @@
return;
auto shareableResource = ShareableResource::create(sharedMemory.releaseNonNull(), 0, m_sourceStorageRecord.body.size());
+ if (!shareableResource)
+ return;
shareableResource->createHandle(m_shareableResourceHandle);
}
#endif
Modified: trunk/Source/WebKit/Shared/ShareableResource.cpp (260439 => 260440)
--- trunk/Source/WebKit/Shared/ShareableResource.cpp 2020-04-21 17:28:37 UTC (rev 260439)
+++ trunk/Source/WebKit/Shared/ShareableResource.cpp 2020-04-21 17:30:19 UTC (rev 260440)
@@ -106,8 +106,17 @@
return resource->wrapInSharedBuffer();
}
-Ref<ShareableResource> ShareableResource::create(Ref<SharedMemory>&& sharedMemory, unsigned offset, unsigned size)
+RefPtr<ShareableResource> ShareableResource::create(Ref<SharedMemory>&& sharedMemory, unsigned offset, unsigned size)
{
+ auto totalSize = CheckedSize(offset) + size;
+ if (totalSize.hasOverflowed()) {
+ LOG_ERROR("Failed to create ShareableResource from SharedMemory due to overflow.");
+ return nullptr;
+ }
+ if (totalSize.unsafeGet() > sharedMemory->size()) {
+ LOG_ERROR("Failed to create ShareableResource from SharedMemory due to mismatched buffer size.");
+ return nullptr;
+ }
return adoptRef(*new ShareableResource(WTFMove(sharedMemory), offset, size));
}
@@ -125,9 +134,6 @@
, m_offset(offset)
, m_size(size)
{
- ASSERT(m_offset + m_size <= m_sharedMemory->size());
-
- // FIXME (NetworkProcess): This data was received from another process. If it is bogus, should we assume that process is compromised and we should kill it?
}
ShareableResource::~ShareableResource()
Modified: trunk/Source/WebKit/Shared/ShareableResource.h (260439 => 260440)
--- trunk/Source/WebKit/Shared/ShareableResource.h 2020-04-21 17:28:37 UTC (rev 260439)
+++ trunk/Source/WebKit/Shared/ShareableResource.h 2020-04-21 17:30:19 UTC (rev 260440)
@@ -65,7 +65,7 @@
};
// Create a shareable resource that uses malloced memory.
- static Ref<ShareableResource> create(Ref<SharedMemory>&&, unsigned offset, unsigned size);
+ static RefPtr<ShareableResource> create(Ref<SharedMemory>&&, unsigned offset, unsigned size);
// Create a shareable resource from a handle.
static RefPtr<ShareableResource> map(const Handle&);
Modified: trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm (260439 => 260440)
--- trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm 2020-04-21 17:28:37 UTC (rev 260439)
+++ trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm 2020-04-21 17:30:19 UTC (rev 260440)
@@ -4378,7 +4378,7 @@
ShareableResource::Handle handle;
auto shareableResource = ShareableResource::create(sharedMemory.releaseNonNull(), 0, buffer.size());
- if (!shareableResource->createHandle(handle))
+ if (!shareableResource || !shareableResource->createHandle(handle))
return;
send(Messages::WebPageProxy::DidFinishLoadForQuickLookDocumentInMainFrame(handle));
