Bug 826052: Make HashStore explicitly reject files larger than UINT32_MAX bytes, to avoid precision issues when we pass the file-length as a uint32_t function argument. (Also, skip a redundant file-size lookup.) r=gcp
authorDaniel Holbert <dholbert@cs.stanford.edu>
Thu, 03 Jan 2013 00:38:24 -0800
changeset 117426 10ed2d7d7368743b1dee08eb80ecb562e81f3f2b
parent 117425 b3c897e476a16f2723561e229fa4c899592fa429
child 117427 34f4d5d47caed99e95b6c1a1f1b914cbc08f9016
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewersgcp
bugs826052
milestone20.0a1
Bug 826052: Make HashStore explicitly reject files larger than UINT32_MAX bytes, to avoid precision issues when we pass the file-length as a uint32_t function argument. (Also, skip a redundant file-size lookup.) r=gcp
toolkit/components/url-classifier/HashStore.cpp
toolkit/components/url-classifier/HashStore.h
--- a/toolkit/components/url-classifier/HashStore.cpp
+++ b/toolkit/components/url-classifier/HashStore.cpp
@@ -180,40 +180,37 @@ HashStore::Reset()
 
   rv = storeFile->Remove(false);
   NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
 }
 
 nsresult
-HashStore::CheckChecksum(nsIFile* aStoreFile)
+HashStore::CheckChecksum(nsIFile* aStoreFile,
+                         uint32_t aFileSize)
 {
   // Check for file corruption by
   // comparing the stored checksum to actual checksum of data
   nsAutoCString hash;
   nsAutoCString compareHash;
   char *data;
   uint32_t read;
 
-  int64_t fileSize;
-  nsresult rv = aStoreFile->GetFileSize(&fileSize);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  if (fileSize < 0) {
-    return NS_ERROR_FAILURE;
-  }
-
-  rv = CalculateChecksum(hash, fileSize, true);
+  nsresult rv = CalculateChecksum(hash, aFileSize, true);
   NS_ENSURE_SUCCESS(rv, rv);
 
   compareHash.GetMutableData(&data, hash.Length());
 
+  if (hash.Length() > aFileSize) {
+    NS_WARNING("SafeBrowing file not long enough to store its hash");
+    return NS_ERROR_FAILURE;
+  }
   nsCOMPtr<nsISeekableStream> seekIn = do_QueryInterface(mInputStream);
-  rv = seekIn->Seek(nsISeekableStream::NS_SEEK_SET, fileSize-hash.Length());
+  rv = seekIn->Seek(nsISeekableStream::NS_SEEK_SET, aFileSize - hash.Length());
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = mInputStream->Read(data, hash.Length(), &read);
   NS_ENSURE_SUCCESS(rv, rv);
   NS_ASSERTION(read == hash.Length(), "Could not read hash bytes");
 
   if (!hash.Equals(compareHash)) {
     NS_WARNING("Safebrowing file failed checksum.");
@@ -243,21 +240,27 @@ HashStore::Open()
   } else {
     SUCCESS_OR_RESET(rv);
   }
 
   int64_t fileSize;
   rv = storeFile->GetFileSize(&fileSize);
   NS_ENSURE_SUCCESS(rv, rv);
 
+  if (fileSize < 0 || fileSize > UINT32_MAX) {
+    return NS_ERROR_FAILURE;
+  }
+
+  uint32_t fileSize32 = static_cast<uint32_t>(fileSize);
+
   rv = NS_NewBufferedInputStream(getter_AddRefs(mInputStream), origStream,
-                                 fileSize);
+                                 fileSize32);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  rv = CheckChecksum(storeFile);
+  rv = CheckChecksum(storeFile, fileSize32);
   SUCCESS_OR_RESET(rv);
 
   rv = ReadHeader();
   SUCCESS_OR_RESET(rv);
 
   rv = SanityCheck();
   SUCCESS_OR_RESET(rv);
 
@@ -296,17 +299,17 @@ HashStore::SanityCheck()
     return NS_ERROR_FAILURE;
   }
 
   return NS_OK;
 }
 
 nsresult
 HashStore::CalculateChecksum(nsAutoCString& aChecksum,
-                             int64_t aSize,
+                             uint32_t aFileSize,
                              bool aChecksumPresent)
 {
   aChecksum.Truncate();
 
   // Reset mInputStream to start
   nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(mInputStream);
   nsresult rv = seekable->Seek(nsISeekableStream::NS_SEEK_SET, 0);
 
@@ -319,17 +322,21 @@ HashStore::CalculateChecksum(nsAutoCStri
   rv = hash->Init(nsICryptoHash::MD5);
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (!aChecksumPresent) {
     // Hash entire file
     rv = hash->UpdateFromStream(mInputStream, UINT32_MAX);
   } else {
     // Hash everything but last checksum bytes
-    rv = hash->UpdateFromStream(mInputStream, aSize-CHECKSUM_SIZE);
+    if (aFileSize < CHECKSUM_SIZE) {
+      NS_WARNING("SafeBrowsing file isn't long enough to store its checksum");
+      return NS_ERROR_FAILURE;
+    }
+    rv = hash->UpdateFromStream(mInputStream, aFileSize - CHECKSUM_SIZE);
   }
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = hash->Finish(false, aChecksum);
   NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
 }
--- a/toolkit/components/url-classifier/HashStore.h
+++ b/toolkit/components/url-classifier/HashStore.h
@@ -114,18 +114,19 @@ public:
   // Wipe out all Completes.
   void ClearCompletes();
 
 private:
   nsresult Reset();
 
   nsresult ReadHeader();
   nsresult SanityCheck();
-  nsresult CalculateChecksum(nsAutoCString& aChecksum, int64_t aSize, bool aChecksumPresent);
-  nsresult CheckChecksum(nsIFile* aStoreFile);
+  nsresult CalculateChecksum(nsAutoCString& aChecksum, uint32_t aFileSize,
+                             bool aChecksumPresent);
+  nsresult CheckChecksum(nsIFile* aStoreFile, uint32_t aFileSize);
   void UpdateHeader();
 
   nsresult ReadChunkNumbers();
   nsresult ReadHashes();
   nsresult ReadAddPrefixes();
   nsresult ReadSubPrefixes();
 
   nsresult WriteAddPrefixes(nsIOutputStream* aOut);