CrossThreadRecord allows you to control uint64_t responseBodySize

rdar://124960263 https://bugs.webkit.org/show_bug.cgi?id=278357 Reviewed by Sihui Liu. There are two problems in this bug that we fix: (1) In the function CacheStorageCache::putRecords, uint64_t responseBodySize is added and subtracted from int64_t spaceRequested. We fix this mismatch by changing the type of spaceRequested to CheckedUint64 and then checking for overflow. Some of the records being added already exist in the cache. We keep track of whether the new version of that record needs more or less space than the existing record and request additional space only if needed. (2) When the client sends the records over IPC, they also send the record's size--and this size is used to calculate and then allocate space. But because the client is sending the size, it's possible an attacker sends a false value for the size. This size is calculated on the client side in CacheStorageConnection::computeRecordBodySize. It does not return the exact size of the body, but rather a size that has a random padding added. We do this for security concerns: whatwg/storage#31. Since this size is random, we cannot check the size by re-calculating it. What we can do is ensure that the size is greater than the actual size because a smaller size would be a clear indication that the size has been tampered with. So we add a check to at least ensure that the size is greater since all the randomly padded sizes are indeed greater than the true sizes. This check is done in NetworkStorageManager::cacheStoragePutRecords using MESSAGE_CHECK so the WebContent process can be killed if the size is invalid, indicating that the process is compromised. * Source/WebKit/NetworkProcess/storage/CacheStorageCache.cpp: (WebKit::CacheStorageCache::putRecords): * Source/WebKit/NetworkProcess/storage/CacheStorageDiskStore.cpp: (WebKit::encodeRecordBody): (WebKit::CacheStorageDiskStore::computeRealBodySizeForStorage): (WebKit::CacheStorageDiskStore::writeRecords): * Source/WebKit/NetworkProcess/storage/CacheStorageDiskStore.h: * Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp: (WebKit::NetworkStorageManager::cacheStoragePutRecords): * Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h: Originally-landed-as: 280938.255@safari-7619-branch (8150063). rdar://138929915
robert-jenner · Nov 5, 2024 · e6191f5 · e6191f5
1 parent 0188a98
commit e6191f5
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 11 deletions.
diff --git a/Source/WebKit/NetworkProcess/storage/CacheStorageCache.cpp b/Source/WebKit/NetworkProcess/storage/CacheStorageCache.cpp
@@ -29,11 +29,13 @@
 #include "CacheStorageDiskStore.h"
 #include "CacheStorageManager.h"
 #include "CacheStorageMemoryStore.h"
+#include "Logging.h"
 #include <WebCore/CacheQueryOptions.h>
 #include <WebCore/CrossOriginAccessControl.h>
 #include <WebCore/HTTPHeaderMap.h>
 #include <WebCore/OriginAccessPatterns.h>
 #include <WebCore/ResourceError.h>
+#include <wtf/CheckedArithmetic.h>
 #include <wtf/Scope.h>
 #include <wtf/TZoneMallocInlines.h>
 
@@ -289,22 +291,38 @@ void CacheStorageCache::putRecords(Vector<WebCore::DOMCacheEngine::CrossThreadRe
     if (!manager)
         return callback(makeUnexpected(WebCore::DOMCacheEngine::Error::Internal));
 
-    int64_t spaceRequested = 0;
+    CheckedUint64 spaceRequested = 0;
+    CheckedUint64 spaceAvailable = 0;
+    bool isSpaceRequestedValid = true;
     auto cacheStorageRecords = WTF::map(WTFMove(records), [&](WebCore::DOMCacheEngine::CrossThreadRecord&& record) {
-        spaceRequested += record.responseBodySize;
-        if (auto* existingRecord = findExistingRecord(record.request))
-            spaceRequested -= existingRecord->size();
+        if (isSpaceRequestedValid) {
+            spaceRequested += record.responseBodySize;
+            if (auto* existingRecord = findExistingRecord(record.request))
+                spaceAvailable += existingRecord->size();
+            if (spaceRequested.hasOverflowed() || spaceAvailable.hasOverflowed())
+                isSpaceRequestedValid = false;
+            else {
+                uint64_t spaceUsed = std::min(spaceRequested, spaceAvailable);
+                spaceRequested -= spaceUsed;
+                spaceAvailable -= spaceUsed;
+            }
+        }
         return toCacheStorageRecord(WTFMove(record), manager->salt(), m_uniqueName);
     });
 
     // The request still needs to go through quota check to keep ordering.
-    if (spaceRequested < 0)
+    if (!isSpaceRequestedValid)
         spaceRequested = 0;
 
-    manager->requestSpace(spaceRequested, [this, weakThis = WeakPtr { *this }, records = WTFMove(cacheStorageRecords), callback = WTFMove(callback)](bool granted) mutable {
+    manager->requestSpace(spaceRequested, [this, weakThis = WeakPtr { *this }, records = WTFMove(cacheStorageRecords), callback = WTFMove(callback), isSpaceRequestedValid](bool granted) mutable {
         if (!weakThis)
             return callback(makeUnexpected(WebCore::DOMCacheEngine::Error::Internal));
 
+        if (!isSpaceRequestedValid) {
+            RELEASE_LOG_ERROR(Storage, "CacheStorageCache::putRecords failed because the amount of space requested is invalid");
+            return callback(makeUnexpected(WebCore::DOMCacheEngine::Error::Internal));
+        }
+
         if (!granted)
             return callback(makeUnexpected(WebCore::DOMCacheEngine::Error::QuotaExceeded));
 

diff --git a/Source/WebKit/NetworkProcess/storage/CacheStorageDiskStore.cpp b/Source/WebKit/NetworkProcess/storage/CacheStorageDiskStore.cpp
@@ -29,6 +29,7 @@
 #include "CacheStorageRecord.h"
 #include "Logging.h"
 #include "NetworkCacheCoders.h"
+#include <WebCore/DOMCacheEngine.h>
 #include <WebCore/ResourceResponse.h>
 #include <wtf/PageBlock.h>
 #include <wtf/RefCounted.h>
@@ -461,9 +462,9 @@ static Vector<uint8_t> encodeRecordHeader(CacheStorageRecord&& record)
     return { encoder.span() };
 }
 
-static Vector<uint8_t> encodeRecordBody(const CacheStorageRecord& record)
+static Vector<uint8_t> encodeRecordBody(const WebCore::DOMCacheEngine::ResponseBody& body)
 {
-    return WTF::switchOn(record.responseBody, [](const Ref<WebCore::FormData>& formData) {
+    return WTF::switchOn(body, [](const Ref<WebCore::FormData>& formData) {
         // FIXME: Store form data body.
         return Vector<uint8_t> { };
     }, [&](const Ref<WebCore::SharedBuffer>& buffer) {
@@ -473,6 +474,11 @@ static Vector<uint8_t> encodeRecordBody(const CacheStorageRecord& record)
     });
 }
 
+size_t CacheStorageDiskStore::computeRealBodySizeForStorage(const WebCore::DOMCacheEngine::ResponseBody& body)
+{
+    return encodeRecordBody(body).size();
+}
+
 static Vector<uint8_t> encodeRecord(const NetworkCache::Key& key, const Vector<uint8_t>& headerData, bool isBodyInline, const Vector<uint8_t>& bodyData, const SHA1::Digest& bodyHash, FileSystem::Salt salt)
 {
     WTF::Persistence::Encoder encoder;
@@ -504,7 +510,7 @@ void CacheStorageDiskStore::writeRecords(Vector<CacheStorageRecord>&& records, W
     Vector<Vector<uint8_t>> recordBlobDatas;
     for (auto&& record : records) {
         recordFiles.append(recordFilePath(record.info.key()));
-        auto bodyData = encodeRecordBody(record);
+        auto bodyData = encodeRecordBody(record.responseBody);
         auto bodyHash = computeSHA1(bodyData.span(), m_salt);
         bool shouldCreateBlob = shouldStoreBodyAsBlob(bodyData);
         auto recordInfoKey = record.info.key();

diff --git a/Source/WebKit/NetworkProcess/storage/CacheStorageDiskStore.h b/Source/WebKit/NetworkProcess/storage/CacheStorageDiskStore.h
@@ -31,11 +31,16 @@
 #include <wtf/WorkQueue.h>
 #include <wtf/text/WTFString.h>
 
+namespace WebCore::DOMCacheEngine {
+using ResponseBody = std::variant<std::nullptr_t, Ref<FormData>, Ref<SharedBuffer>>;
+}
+
 namespace WebKit {
 
 class CacheStorageDiskStore final : public CacheStorageStore {
 public:
     static Ref<CacheStorageDiskStore> create(const String& cacheName, const String& path, Ref<WorkQueue>&&);
+    static size_t computeRealBodySizeForStorage(const WebCore::DOMCacheEngine::ResponseBody&);
 
 private:
     CacheStorageDiskStore(const String& cacheName, const String& path, Ref<WorkQueue>&&);

diff --git a/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp b/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp
@@ -29,6 +29,7 @@
 #include "BackgroundFetchChange.h"
 #include "BackgroundFetchStoreManager.h"
 #include "CacheStorageCache.h"
+#include "CacheStorageDiskStore.h"
 #include "CacheStorageManager.h"
 #include "CacheStorageRegistry.h"
 #include "FileSystemStorageHandleRegistry.h"
@@ -50,6 +51,7 @@
 #include "StorageAreaRegistry.h"
 #include "UnifiedOriginStorageLevel.h"
 #include "WebsiteDataType.h"
+#include <WebCore/DOMCacheEngine.h>
 #include <WebCore/IDBRequestData.h>
 #include <WebCore/SecurityOriginData.h>
 #include <WebCore/ServiceWorkerContextData.h>
@@ -1826,12 +1828,15 @@ void NetworkStorageManager::cacheStorageRemoveRecords(WebCore::DOMCacheIdentifie
     cache->removeRecords(WTFMove(request), WTFMove(options), WTFMove(callback));
 }
 
-void NetworkStorageManager::cacheStoragePutRecords(WebCore::DOMCacheIdentifier cacheIdentifier, Vector<WebCore::DOMCacheEngine::CrossThreadRecord>&& records, WebCore::DOMCacheEngine::RecordIdentifiersCallback&& callback)
+void NetworkStorageManager::cacheStoragePutRecords(IPC::Connection& connection, WebCore::DOMCacheIdentifier cacheIdentifier, Vector<WebCore::DOMCacheEngine::CrossThreadRecord>&& records, WebCore::DOMCacheEngine::RecordIdentifiersCallback&& callback)
 {
     RefPtr cache = protectedCacheStorageRegistry()->cache(cacheIdentifier);
     if (!cache)
         return callback(makeUnexpected(WebCore::DOMCacheEngine::Error::Internal));
 
+    for (auto& record : records)
+        MESSAGE_CHECK_COMPLETION(record.responseBodySize >= CacheStorageDiskStore::computeRealBodySizeForStorage(record.responseBody), connection, callback(makeUnexpected(WebCore::DOMCacheEngine::Error::Internal)));
+
     cache->putRecords(WTFMove(records), WTFMove(callback));
 }
 

diff --git a/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h b/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h
@@ -229,7 +229,7 @@ class NetworkStorageManager final : public IPC::WorkQueueMessageReceiver, public
     void unlockCacheStorage(IPC::Connection&, const WebCore::ClientOrigin&);
     void cacheStorageRetrieveRecords(WebCore::DOMCacheIdentifier, WebCore::RetrieveRecordsOptions&&, WebCore::DOMCacheEngine::CrossThreadRecordsCallback&&);
     void cacheStorageRemoveRecords(WebCore::DOMCacheIdentifier, WebCore::ResourceRequest&&, WebCore::CacheQueryOptions&&, WebCore::DOMCacheEngine::RecordIdentifiersCallback&&);
-    void cacheStoragePutRecords(WebCore::DOMCacheIdentifier, Vector<WebCore::DOMCacheEngine::CrossThreadRecord>&&, WebCore::DOMCacheEngine::RecordIdentifiersCallback&&);
+    void cacheStoragePutRecords(IPC::Connection&, WebCore::DOMCacheIdentifier, Vector<WebCore::DOMCacheEngine::CrossThreadRecord>&&, WebCore::DOMCacheEngine::RecordIdentifiersCallback&&);
     void cacheStorageClearMemoryRepresentation(const WebCore::ClientOrigin&, CompletionHandler<void()>&&);
     void cacheStorageRepresentation(CompletionHandler<void(String&&)>&&);