From f82a30519edc97d9e3a4e1f780d82e7a84fac9da Mon Sep 17 00:00:00 2001 From: Dana Keeler Date: Fri, 12 Apr 2019 22:34:23 +0000 Subject: [PATCH] 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 | 32 +++++++++++-------- .../ssl/tests/unit/test_cert_storage_prefs.js | 25 +++++++++++++++ security/manager/ssl/tests/unit/xpcshell.ini | 1 + 3 files changed, 44 insertions(+), 14 deletions(-) create mode 100644 security/manager/ssl/tests/unit/test_cert_storage_prefs.js diff --git a/security/manager/ssl/cert_storage/src/lib.rs b/security/manager/ssl/cert_storage/src/lib.rs index d6019cfdfdaf..9a277169e950 100644 --- a/security/manager/ssl/cert_storage/src/lib.rs +++ b/security/manager/ssl/cert_storage/src/lib.rs @@ -84,7 +84,7 @@ struct EnvAndStore { struct SecurityState { profile_path: PathBuf, env_and_store: Option, - int_prefs: HashMap, + int_prefs: HashMap, } 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 Result { - 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 { @@ -493,7 +494,7 @@ fn do_construct_cert_storage( }; } -fn read_int_pref(name: &str) -> Result { +fn read_int_pref(name: &str) -> Result { let pref_service = match xpcom::services::get_PreferencesService() { Some(ps) => ps, _ => { @@ -512,16 +513,20 @@ fn read_int_pref(name: &str) -> Result { _ => 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 = 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. diff --git a/security/manager/ssl/tests/unit/test_cert_storage_prefs.js b/security/manager/ssl/tests/unit/test_cert_storage_prefs.js new file mode 100644 index 000000000000..f03b438f28a1 --- /dev/null +++ b/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"); +} diff --git a/security/manager/ssl/tests/unit/xpcshell.ini b/security/manager/ssl/tests/unit/xpcshell.ini index ebad1a912702..fde9e9451490 100644 --- a/security/manager/ssl/tests/unit/xpcshell.ini +++ b/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]