Network process crashes decoding invalid cache entry on 32bit system
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 Jun 2015 17:03:02 +0000 (17:03 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 Jun 2015 17:03:02 +0000 (17:03 +0000)
https://bugs.webkit.org/show_bug.cgi?id=145842
rdar://problem/21228334

Reviewed by Anders Carlsson.

After cache scheme changes we may end up decoding invalid cache entries. This is by design,
we should just fail decoding and delete these entries.

However Decoder::bufferIsLargeEnoughToContain test in some cases would allow corrupted large
sizes due to overflow in 32bit pointer math and we would crash when allocating a string.

* NetworkProcess/cache/NetworkCacheCoders.cpp:
(WebKit::NetworkCache::Coder<CString>::decode):
(WebKit::NetworkCache::decodeStringText):
(WebKit::NetworkCache::Coder<WebCore::CertificateInfo>::decode):
(WebKit::NetworkCache::Coder<MD5::Digest>::encode):
* NetworkProcess/cache/NetworkCacheCoders.h:
* NetworkProcess/cache/NetworkCacheDecoder.cpp:
(WebKit::NetworkCache::Decoder::Decoder):
(WebKit::NetworkCache::Decoder::bufferIsLargeEnoughToContain):

    Reshuffle to avoid sum.

(WebKit::NetworkCache::Decoder::decodeFixedLengthData):
* NetworkProcess/cache/NetworkCacheDecoder.h:
(WebKit::NetworkCache::Decoder::bufferSize):
(WebKit::NetworkCache::Decoder::currentOffset):
(WebKit::NetworkCache::Decoder::length): Deleted.
(WebKit::NetworkCache::Decoder::isInvalid): Deleted.
(WebKit::NetworkCache::Decoder::markInvalid): Deleted.

    Remove these, they are not really used or needed.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@185458 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit2/ChangeLog
Source/WebKit2/NetworkProcess/cache/NetworkCacheCoders.cpp
Source/WebKit2/NetworkProcess/cache/NetworkCacheCoders.h
Source/WebKit2/NetworkProcess/cache/NetworkCacheDecoder.cpp
Source/WebKit2/NetworkProcess/cache/NetworkCacheDecoder.h

index 23ccc71..c08ffba 100644 (file)
@@ -1,3 +1,39 @@
+2015-06-11  Antti Koivisto  <antti@apple.com>
+
+        Network process crashes decoding invalid cache entry on 32bit system
+        https://bugs.webkit.org/show_bug.cgi?id=145842
+        rdar://problem/21228334
+
+        Reviewed by Anders Carlsson.
+
+        After cache scheme changes we may end up decoding invalid cache entries. This is by design,
+        we should just fail decoding and delete these entries.
+
+        However Decoder::bufferIsLargeEnoughToContain test in some cases would allow corrupted large
+        sizes due to overflow in 32bit pointer math and we would crash when allocating a string.
+
+        * NetworkProcess/cache/NetworkCacheCoders.cpp:
+        (WebKit::NetworkCache::Coder<CString>::decode):
+        (WebKit::NetworkCache::decodeStringText):
+        (WebKit::NetworkCache::Coder<WebCore::CertificateInfo>::decode):
+        (WebKit::NetworkCache::Coder<MD5::Digest>::encode):
+        * NetworkProcess/cache/NetworkCacheCoders.h:
+        * NetworkProcess/cache/NetworkCacheDecoder.cpp:
+        (WebKit::NetworkCache::Decoder::Decoder):
+        (WebKit::NetworkCache::Decoder::bufferIsLargeEnoughToContain):
+
+            Reshuffle to avoid sum.
+
+        (WebKit::NetworkCache::Decoder::decodeFixedLengthData):
+        * NetworkProcess/cache/NetworkCacheDecoder.h:
+        (WebKit::NetworkCache::Decoder::bufferSize):
+        (WebKit::NetworkCache::Decoder::currentOffset):
+        (WebKit::NetworkCache::Decoder::length): Deleted.
+        (WebKit::NetworkCache::Decoder::isInvalid): Deleted.
+        (WebKit::NetworkCache::Decoder::markInvalid): Deleted.
+
+            Remove these, they are not really used or needed.
+
 2015-06-10  Anders Carlsson  <andersca@apple.com>
 
         Rewrite WKPluginSiteDataManager using WebsiteDataStore functions
index d7c59b0..6dbb35b 100644 (file)
@@ -76,10 +76,8 @@ bool Coder<CString>::decode(Decoder& decoder, CString& result)
     }
 
     // Before allocating the string, make sure that the decoder buffer is big enough.
-    if (!decoder.bufferIsLargeEnoughToContain<char>(length)) {
-        decoder.markInvalid();
+    if (!decoder.bufferIsLargeEnoughToContain<char>(length))
         return false;
-    }
 
     char* buffer;
     CString string = CString::newUninitialized(length, buffer);
@@ -114,11 +112,9 @@ template <typename CharacterType>
 static inline bool decodeStringText(Decoder& decoder, uint32_t length, String& result)
 {
     // Before allocating the string, make sure that the decoder buffer is big enough.
-    if (!decoder.bufferIsLargeEnoughToContain<CharacterType>(length)) {
-        decoder.markInvalid();
+    if (!decoder.bufferIsLargeEnoughToContain<CharacterType>(length))
         return false;
-    }
-    
+
     CharacterType* buffer;
     String string = String::createUninitialized(length, buffer);
     if (!decoder.decodeFixedLengthData(reinterpret_cast<uint8_t*>(buffer), length * sizeof(CharacterType)))
@@ -167,11 +163,7 @@ bool Coder<WebCore::CertificateInfo>::decode(Decoder& decoder, WebCore::Certific
     if (!decoder.decodeFixedLengthData(data.data(), data.size()))
         return false;
     IPC::ArgumentDecoder argumentDecoder(data.data(), data.size());
-    if (!argumentDecoder.decode(certificateInfo)) {
-        decoder.markInvalid();
-        return false;
-    }
-    return true;
+    return argumentDecoder.decode(certificateInfo);
 }
 
 void Coder<MD5::Digest>::encode(Encoder& encoder, const MD5::Digest& digest)
index 3090687..198c5a7 100644 (file)
@@ -150,10 +150,8 @@ template<typename T, size_t inlineCapacity> struct VectorCoder<true, T, inlineCa
         // Since we know the total size of the elements, we can allocate the vector in
         // one fell swoop. Before allocating we must however make sure that the decoder buffer
         // is big enough.
-        if (!decoder.bufferIsLargeEnoughToContain<T>(size)) {
-            decoder.markInvalid();
+        if (!decoder.bufferIsLargeEnoughToContain<T>(size))
             return false;
-        }
 
         Vector<T, inlineCapacity> temp;
         temp.resize(size);
@@ -194,7 +192,6 @@ template<typename KeyArg, typename MappedArg, typename HashArg, typename KeyTrai
 
             if (!tempHashMap.add(key, value).isNewEntry) {
                 // The hash map already has the specified key, bail.
-                decoder.markInvalid();
                 return false;
             }
         }
@@ -228,7 +225,6 @@ template<typename KeyArg, typename HashArg, typename KeyTraitsArg> struct Coder<
 
             if (!tempHashSet.add(key).isNewEntry) {
                 // The hash map already has the specified key, bail.
-                decoder.markInvalid();
                 return false;
             }
         }
index a9b0886..285429f 100644 (file)
@@ -47,7 +47,7 @@ Decoder::~Decoder()
 
 bool Decoder::bufferIsLargeEnoughToContain(size_t size) const
 {
-    return m_bufferPosition + size <= m_bufferEnd;
+    return size <= static_cast<size_t>(m_bufferEnd - m_bufferPosition);
 }
 
 bool Decoder::decodeFixedLengthData(uint8_t* data, size_t size)
index 1d76773..899bcee 100644 (file)
@@ -42,9 +42,6 @@ public:
     size_t length() const { return m_bufferEnd - m_buffer; }
     size_t currentOffset() const { return m_bufferPosition - m_buffer; }
 
-    bool isInvalid() const { return m_bufferPosition > m_bufferEnd; }
-    void markInvalid() { m_bufferPosition = m_bufferEnd + 1; }
-
     bool verifyChecksum();
 
     bool decodeFixedLengthData(uint8_t*, size_t);