bug 1543685 - handle preference values more safely in cert_storage r=mgoodwin

Previously cert_storage could use negative values as unsigned values when
determining if its data was sufficiently fresh, which could cause assertion
failures when doing time math.
This patch changes the behavior to just use 0 if values are either unavailable
or negative, which means we fail closed and say everything is out of date if we
otherwise don't have the information to make the correct decision.

Differential Revision: https://phabricator.services.mozilla.com/D27196

--HG--
extra : moz-landing-system : lando

security/manager/ssl/cert_storage/src/lib.rs

@@ -84,7 +84,7 @@ struct EnvAndStore {
 struct SecurityState {
     profile_path: PathBuf,
     env_and_store: Option<EnvAndStore>,
-    int_prefs: HashMap<String, i32>,
+    int_prefs: HashMap<String, u32>,
 }
 
 impl SecurityState {
@@ -408,7 +408,7 @@ impl SecurityState {
         )
     }
 
-    pub fn pref_seen(&mut self, name: &str, value: i32) {
+    pub fn pref_seen(&mut self, name: &str, value: u32) {
         self.int_prefs.insert(name.to_owned(), value);
     }
 }
@@ -458,7 +458,8 @@ fn get_path_from_directory_service(key: &str) -> Result<PathBuf, SecurityStateEr
 }
 
 fn get_profile_path() -> Result<PathBuf, SecurityStateError> {
-    Ok(get_path_from_directory_service("ProfD").or_else(|_| get_path_from_directory_service("TmpD"))?)
+    Ok(get_path_from_directory_service("ProfD")
+        .or_else(|_| get_path_from_directory_service("TmpD"))?)
 }
 
 fn get_store_path(profile_path: &PathBuf) -> Result<PathBuf, SecurityStateError> {
@@ -493,7 +494,7 @@ fn do_construct_cert_storage(
     };
 }
 
-fn read_int_pref(name: &str) -> Result<i32, SecurityStateError> {
+fn read_int_pref(name: &str) -> Result<u32, SecurityStateError> {
     let pref_service = match xpcom::services::get_PreferencesService() {
         Some(ps) => ps,
         _ => {
@@ -512,16 +513,20 @@ fn read_int_pref(name: &str) -> Result<i32, SecurityStateError> {
         _ => return Err(SecurityStateError::from("could not build pref name string")),
     };
 
-    let mut pref_value: i32 = -1;
-
+    let mut pref_value: i32 = 0;
     // We can't use GetIntPrefWithDefault because optional_argc is not
     // supported. No matter, we can just check for failure and ignore
     // any NS_ERROR_UNEXPECTED result.
     let res = unsafe { (*prefs).GetIntPref((&pref_name).as_ptr(), (&mut pref_value) as *mut i32) };
-    match res {
-        NS_OK => Ok(pref_value),
-        NS_ERROR_UNEXPECTED => Ok(pref_value),
-        _ => Err(SecurityStateError::from("could not read pref")),
+    let pref_value = match res {
+        NS_OK => pref_value,
+        NS_ERROR_UNEXPECTED => 0,
+        _ => return Err(SecurityStateError::from("could not read pref")),
+    };
+    if pref_value < 0 {
+        Ok(0)
+    } else {
+        Ok(pref_value as u32)
     }
 }
 
@@ -741,7 +746,7 @@ impl CertStorage {
         let runnable = try_ns!(TaskRunnable::new("SetRevocationBySubjectAndPubKey", task));
         try_ns!(runnable.dispatch(&*thread));
         NS_OK
-   }
+    }
 
     unsafe fn SetEnrollment(
         &self,
@@ -960,8 +965,6 @@ impl CertStorage {
     ) -> nserror::nsresult {
         match CStr::from_ptr(topic).to_str() {
             Ok("nsPref:changed") => {
-                let mut pref_value: i32 = 0;
-
                 let prefs: RefPtr<nsIPrefBranch> = match (*subject).query_interface() {
                     Some(pb) => pb,
                     _ => return NS_ERROR_FAILURE,
@@ -982,11 +985,12 @@ impl CertStorage {
                     _ => return NS_ERROR_FAILURE,
                 };
 
+                let mut pref_value: i32 = 0;
                 let res = prefs.GetIntPref((&pref_name).as_ptr(), (&mut pref_value) as *mut i32);
-
                 if !res.succeeded() {
                     return res;
                 }
+                let pref_value = if pref_value < 0 { 0 } else { pref_value as u32 };
 
                 let mut ss = try_ns!(self.security_state.write());
                 // This doesn't use the db -> don't need to make sure it's open.

security/manager/ssl/tests/unit/test_cert_storage_prefs.js

@@ -0,0 +1,25 @@
+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+"use strict";
+
+// Tests that cert_storage properly handles its preference values.
+
+function run_test() {
+  let certStorage = Cc["@mozilla.org/security/certstorage;1"].getService(Ci.nsICertStorage);
+  // Since none of our prefs start with values, looking them up will fail. cert_storage should use
+  // safe fallbacks.
+  ok(!certStorage.isBlocklistFresh(), "checking blocklist freshness shouldn't crash");
+  ok(!certStorage.isWhitelistFresh(), "checking whitelist freshness shouldn't crash");
+  ok(!certStorage.isEnrollmentFresh(), "checking enrollment freshness shouldn't crash");
+
+  // If we set nonsensical values, cert_storage should still use safe fallbacks.
+  Services.prefs.setIntPref("services.blocklist.onecrl.checked", -2);
+  Services.prefs.setIntPref("security.onecrl.maximum_staleness_in_seconds", -7);
+  ok(!certStorage.isBlocklistFresh(), "checking blocklist freshness still shouldn't crash");
+
+  // Clearing prefs shouldn't cause failures.
+  Services.prefs.clearUserPref("services.blocklist.onecrl.checked");
+  ok(!certStorage.isBlocklistFresh(), "checking blocklist freshness again shouldn't crash");
+}

security/manager/ssl/tests/unit/xpcshell.ini

@@ -48,6 +48,7 @@ support-files =
 skip-if = os != 'mac'
 [test_cert_storage.js]
 tags = addons psm blocklist
+[test_cert_storage_prefs.js]
 [test_cert_chains.js]
 run-sequentially = hardcoded ports
 [test_cert_dbKey.js]