Bug 1433636 - Put a limit on the length of Safe Browsing metadata values. r?gcp draft
authorFrancois Marier <francois@mozilla.com>
Tue, 30 Jan 2018 14:21:33 -0800
changeset 750773 dd7c7a1e33009795dcf42f74dff398629dbbd100
parent 748899 fd995039d89708923b5673ecebc652967d40bd4e
push id97742
push userfmarier@mozilla.com
push dateFri, 02 Feb 2018 20:26:50 +0000
reviewersgcp
bugs1433636
milestone60.0a1
Bug 1433636 - Put a limit on the length of Safe Browsing metadata values. r?gcp Disk corruption can lead to the stored length of a value to be unreasonably large and trigger an OOM. Since values are all currently <= 32 bytes, we can safely enforce a 256-byte upper bound. MozReview-Commit-ID: XygReOpEK3
toolkit/components/telemetry/Histograms.json
toolkit/components/url-classifier/LookupCache.cpp
toolkit/components/url-classifier/LookupCacheV4.cpp
toolkit/components/url-classifier/LookupCacheV4.h
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -4874,22 +4874,31 @@
     "high": 5000,
     "n_buckets": 15,
     "bug_numbers": [1336865],
     "description": "Time spent constructing Variable-Length PrefixSet from file (ms)"
   },
   "URLCLASSIFIER_VLPS_LOAD_CORRUPT": {
     "record_in_processes": ["main", "content"],
     "alert_emails": ["francois@mozilla.com", "safebrowsing-telemetry@mozilla.org"],
-    "expires_in_version": "63",
+    "expires_in_version": "never",
     "releaseChannelCollection": "opt-out",
     "kind": "boolean",
     "bug_numbers": [1305581],
     "description": "Whether or not a variable-length prefix set loaded from disk is corrupted (true = file corrupted)."
   },
+  "URLCLASSIFIER_VLPS_METADATA_CORRUPT": {
+    "record_in_processes": ["main", "content"],
+    "alert_emails": ["francois@mozilla.com", "safebrowsing-telemetry@mozilla.org"],
+    "expires_in_version": "never",
+    "releaseChannelCollection": "opt-out",
+    "kind": "boolean",
+    "bug_numbers": [1433636],
+    "description": "Whether or not the metadata for a variable-length prefix set loaded from disk is corrupted (true = file corrupted)."
+  },
   "URLCLASSIFIER_VLPS_LONG_PREFIXES": {
     "record_in_processes": ["main", "content"],
     "alert_emails": ["francois@mozilla.com", "safebrowsing-telemetry@mozilla.org"],
     "expires_in_version": "63",
     "releaseChannelCollection": "opt-out",
     "kind": "enumerated",
     "n_values": 32,
     "bug_numbers": [1322523],
--- a/toolkit/components/url-classifier/LookupCache.cpp
+++ b/toolkit/components/url-classifier/LookupCache.cpp
@@ -74,17 +74,17 @@ LookupCache::LookupCache(const nsACStrin
   , mRootStoreDirectory(aRootStoreDir)
 {
   UpdateRootDirHandle(mRootStoreDirectory);
 }
 
 nsresult
 LookupCache::Open()
 {
-  LOG(("Loading PrefixSet"));
+  LOG(("Loading PrefixSet for %s", mTableName.get()));
   nsresult rv = LoadPrefixSet();
   NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
 }
 
 nsresult
 LookupCache::UpdateRootDirHandle(nsIFile* aNewRootStoreDirectory)
--- a/toolkit/components/url-classifier/LookupCacheV4.cpp
+++ b/toolkit/components/url-classifier/LookupCacheV4.cpp
@@ -14,16 +14,17 @@ extern mozilla::LazyLogModule gUrlClassi
 #define LOG_ENABLED() MOZ_LOG_TEST(gUrlClassifierDbServiceLog, mozilla::LogLevel::Debug)
 
 #define METADATA_SUFFIX NS_LITERAL_CSTRING(".metadata")
 
 namespace mozilla {
 namespace safebrowsing {
 
 const int LookupCacheV4::VER = 4;
+const uint32_t LookupCacheV4::MAX_METADATA_VALUE_LENGTH = 256;
 
 // Prefixes coming from updates and VLPrefixSet are both stored in the HashTable
 // where the (key, value) pair is a prefix size and a lexicographic-sorted string.
 // The difference is prefixes from updates use std:string(to avoid additional copies)
 // and prefixes from VLPrefixSet use nsCString.
 // This class provides a common interface for the partial update algorithm to make it
 // easier to operate on two different kind prefix string map..
 class VLPrefixSet
@@ -162,16 +163,18 @@ LookupCacheV4::LoadFromFile(nsIFile* aFi
 {
   nsresult rv = mVLPrefixSet->LoadFromFile(aFile);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   nsCString state, checksum;
   rv = LoadMetadata(state, checksum);
+  Telemetry::Accumulate(Telemetry::URLCLASSIFIER_VLPS_METADATA_CORRUPT,
+                        rv == NS_ERROR_FILE_CORRUPTED);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   rv = VerifyChecksum(checksum);
   Telemetry::Accumulate(Telemetry::URLCLASSIFIER_VLPS_LOAD_CORRUPT,
                         rv == NS_ERROR_FILE_CORRUPTED);
 
@@ -398,16 +401,18 @@ LookupCacheV4::VerifyChecksum(const nsAC
 //////////////////////////////////////////////////////////////////////////
 // A set of lightweight functions for reading/writing value from/to file.
 
 namespace {
 
 template<typename T>
 struct ValueTraits
 {
+  static_assert(sizeof(T) <= LookupCacheV4::MAX_METADATA_VALUE_LENGTH,
+                "LookupCacheV4::MAX_METADATA_VALUE_LENGTH is too small.");
   static uint32_t Length(const T& aValue) { return sizeof(T); }
   static char* WritePtr(T& aValue, uint32_t aLength) { return (char*)&aValue; }
   static const char* ReadPtr(const T& aValue) { return (char*)&aValue; }
   static bool IsFixedLength() { return true; }
 };
 
 template<>
 struct ValueTraits<nsACString>
@@ -430,16 +435,18 @@ struct ValueTraits<nsACString>
     return aValue.BeginReading();
   }
 };
 
 template<typename T> static nsresult
 WriteValue(nsIOutputStream *aOutputStream, const T& aValue)
 {
   uint32_t writeLength = ValueTraits<T>::Length(aValue);
+  MOZ_ASSERT(writeLength <= LookupCacheV4::MAX_METADATA_VALUE_LENGTH,
+             "LookupCacheV4::MAX_METADATA_VALUE_LENGTH is too small.");
   if (!ValueTraits<T>::IsFixedLength()) {
     // We need to write out the variable value length.
     nsresult rv = WriteValue(aOutputStream, writeLength);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   // Write out the value.
   auto valueReadPtr = ValueTraits<T>::ReadPtr(aValue);
@@ -462,16 +469,22 @@ ReadValue(nsIInputStream* aInputStream, 
   if (ValueTraits<T>::IsFixedLength()) {
     readLength = ValueTraits<T>::Length(aValue);
   } else {
     // Read the variable value length from file.
     nsresult rv = ReadValue(aInputStream, readLength);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
+  // Sanity-check the readLength in case of disk corruption
+  // (see bug 1433636).
+  if (readLength > LookupCacheV4::MAX_METADATA_VALUE_LENGTH) {
+    return NS_ERROR_FILE_CORRUPTED;
+  }
+
   // Read the value.
   uint32_t read;
   auto valueWritePtr = ValueTraits<T>::WritePtr(aValue, readLength);
   rv = aInputStream->Read(valueWritePtr, readLength, &read);
   if (NS_FAILED(rv) || read != readLength) {
     LOG(("Failed to read the value."));
     return NS_FAILED(rv) ? rv : NS_ERROR_FAILURE;
   }
--- a/toolkit/components/url-classifier/LookupCacheV4.h
+++ b/toolkit/components/url-classifier/LookupCacheV4.h
@@ -42,16 +42,17 @@ public:
                        PrefixStringMap& aOutputMap);
 
   nsresult AddFullHashResponseToCache(const FullHashResponseMap& aResponseMap);
 
   nsresult WriteMetadata(TableUpdateV4* aTableUpdate);
   nsresult LoadMetadata(nsACString& aState, nsACString& aChecksum);
 
   static const int VER;
+  static const uint32_t MAX_METADATA_VALUE_LENGTH;
 
 protected:
   virtual nsresult ClearPrefixes() override;
   virtual nsresult StoreToFile(nsIFile* aFile) override;
   virtual nsresult LoadFromFile(nsIFile* aFile) override;
   virtual size_t SizeOfPrefixSet() override;
 
 private: