From b3666e5fd0ec354cd7f4b6cb937c8fe098b7b9a4 Mon Sep 17 00:00:00 2001 From: Dana Keeler Date: Wed, 20 Mar 2019 00:01:47 +0000 Subject: [PATCH] Bug 1429796 - cert_storage: create rkv environment and store only once to avoid races r=mgoodwin,jcj This patch also base64-decodes the API inputs before storing in the DB in anticipation of being able to pass binary data directly (bug 1535752). This patch additionally whitelists the DB backing file in talos. Differential Revision: https://phabricator.services.mozilla.com/D23430 --HG-- extra : moz-landing-system : lando --- security/manager/ssl/cert_storage/src/lib.rs | 334 ++++++++---------- .../talos/talos/xtalos/xperf_whitelist.json | 6 + 2 files changed, 156 insertions(+), 184 deletions(-) diff --git a/security/manager/ssl/cert_storage/src/lib.rs b/security/manager/ssl/cert_storage/src/lib.rs index 4f459f51153f..d7a46b4c3202 100644 --- a/security/manager/ssl/cert_storage/src/lib.rs +++ b/security/manager/ssl/cert_storage/src/lib.rs @@ -29,13 +29,20 @@ use style::gecko_bindings::structs::nsresult; use xpcom::interfaces::{nsICertStorage, nsIFile, nsIObserver, nsIPrefBranch, nsISupports}; use xpcom::{nsIID, GetterAddrefs, RefPtr, XpCom}; -use rkv::{Rkv, StoreOptions, Value}; +use rkv::{Rkv, SingleStore, StoreOptions, Value}; const PREFIX_REV_IS: &str = "is"; const PREFIX_REV_SPK: &str = "spk"; const PREFIX_CRLITE: &str = "crlite"; const PREFIX_WL: &str = "wl"; +fn make_key(prefix: &str, part_a: &[u8], part_b: &[u8]) -> Vec { + let mut key = prefix.as_bytes().to_owned(); + key.extend_from_slice(part_a); + key.extend_from_slice(part_b); + key +} + #[allow(non_camel_case_types, non_snake_case)] /// `SecurityStateError` is a type to represent errors in accessing or @@ -57,7 +64,8 @@ impl From for SecurityStateError { /// `SecurityState` pub struct SecurityState { - path: PathBuf, + env: Rkv, + store: SingleStore, int_prefs: HashMap, } @@ -67,8 +75,13 @@ impl SecurityState { store_path.push("security_state"); create_dir_all(store_path.as_path())?; + let env = Rkv::new(store_path.as_path())?; + let mut options = StoreOptions::create(); + options.create = true; + let store = env.open_single("cert_storage", options)?; let mut ss = SecurityState { - path: store_path, + env: env, + store: store, int_prefs: HashMap::new(), }; @@ -88,70 +101,67 @@ impl SecurityState { let file = BufReader::new(f); // Add the data from revocations.txt - let mut dn: Option = None; - let mut l; + let mut dn: Option> = None; for line in file.lines() { - l = match line.map_err(|_| SecurityStateError::from("io error reading line data")) { - Ok(data) => data.to_owned(), + let l = match line.map_err(|_| SecurityStateError::from("io error reading line data")) { + Ok(data) => data, Err(e) => return Err(e), }; - let l_sans_prefix = match l.len() { - 0 => "".to_owned(), - _ => l[1..].to_owned(), - }; - // In future, we can maybe log migration failures. For now, ignore the error - // and attempt to continue. - let _ = match l.chars().next() { - Some('#') => Ok(()), - Some('\t') => match &dn { - Some(name) => self.set_revocation_by_subject_and_pub_key( - &name, - &l_sans_prefix, - nsICertStorage::STATE_ENFORCE as i16, - ), - None => Err(SecurityStateError::from("pubkey with no subject")), - }, - Some(' ') => match &dn { - Some(name) => self.set_revocation_by_issuer_and_serial( - &name, - &l_sans_prefix, - nsICertStorage::STATE_ENFORCE as i16, - ), - None => Err(SecurityStateError::from("serial with no issuer")), - }, - Some(_) => { - dn = Some(l); - Ok(()) + if l.len() == 0 || l.starts_with("#") { + continue; + } + let leading_char = match l.chars().next() { + Some(c) => c, + None => { + return Err(SecurityStateError::from( + "couldn't get char from non-empty str?", + )); } - None => Ok(()), }; + // In future, we can maybe log migration failures. For now, ignore decoding and storage + // errors and attempt to continue. + // Check if we have a new DN + if leading_char != '\t' && leading_char != ' ' { + if let Ok(decoded_dn) = base64::decode(&l) { + dn = Some(decoded_dn); + } + continue; + } + let l_sans_prefix = match base64::decode(&l[1..]) { + Ok(decoded) => decoded, + Err(_) => continue, + }; + if let Some(name) = &dn { + if leading_char == '\t' { + let _ = self.set_revocation_by_subject_and_pub_key( + name, + &l_sans_prefix, + nsICertStorage::STATE_ENFORCE as i16, + ); + } else { + let _ = self.set_revocation_by_issuer_and_serial( + name, + &l_sans_prefix, + nsICertStorage::STATE_ENFORCE as i16, + ); + } + } } Ok(()) } - fn write_entry(&mut self, key: &str, value: i16) -> Result<(), SecurityStateError> { - let p = self.path.as_path(); - let env = Rkv::new(p)?; - let store = env.open_single("cert_storage", StoreOptions::create())?; - - let mut writer = env.write()?; - store.put(&mut writer, key, &Value::I64(value as i64))?; + fn write_entry(&mut self, key: &[u8], value: i16) -> Result<(), SecurityStateError> { + let mut writer = self.env.write()?; + self.store + .put(&mut writer, key, &Value::I64(value as i64))?; writer.commit()?; Ok(()) } - fn read_entry(&self, key: &str) -> Result, SecurityStateError> { - // TODO: figure out a way to de-dupe getting the env / store - let p = self.path.as_path(); - let env = Rkv::new(p)?; - let store = env.open_single("cert_storage", StoreOptions::create())?; - - let reader = env.read()?; - match store.get(&reader, key) { - // There's no tidy way in rkv::Value to get an owned value. The - // only way I can see to get such functionality is to go via - // primitives. + fn read_entry(&self, key: &[u8]) -> Result, SecurityStateError> { + let reader = self.env.read()?; + match self.store.get(&reader, key) { Ok(Some(Value::I64(i))) if i <= (std::i16::MAX as i64) && i >= (std::i16::MIN as i64) => { @@ -166,66 +176,53 @@ impl SecurityState { pub fn set_revocation_by_issuer_and_serial( &mut self, - issuer: &str, - serial: &str, + issuer: &[u8], + serial: &[u8], state: i16, ) -> Result<(), SecurityStateError> { - self.write_entry(&format!("{}:{}:{}", PREFIX_REV_IS, issuer, serial), state) + self.write_entry(&make_key(PREFIX_REV_IS, issuer, serial), state) } pub fn set_revocation_by_subject_and_pub_key( &mut self, - subject: &str, - pub_key_hash: &str, + subject: &[u8], + pub_key_hash: &[u8], state: i16, ) -> Result<(), SecurityStateError> { - self.write_entry( - &format!("{}:{}:{}", PREFIX_REV_SPK, subject, pub_key_hash), - state, - ) + self.write_entry(&make_key(PREFIX_REV_SPK, subject, pub_key_hash), state) } pub fn set_enrollment( &mut self, - issuer: &str, - serial: &str, + issuer: &[u8], + serial: &[u8], state: i16, ) -> Result<(), SecurityStateError> { - self.write_entry(&format!("{}:{}:{}", PREFIX_CRLITE, issuer, serial), state) + self.write_entry(&make_key(PREFIX_CRLITE, issuer, serial), state) } pub fn set_whitelist( &mut self, - issuer: &str, - serial: &str, + issuer: &[u8], + serial: &[u8], state: i16, ) -> Result<(), SecurityStateError> { - self.write_entry(&format!("{}:{}:{}", PREFIX_WL, issuer, serial), state) + self.write_entry(&make_key(PREFIX_WL, issuer, serial), state) } pub fn get_revocation_state( &self, - issuer: &str, - serial: &str, - subject: &str, - pub_key: &str, + issuer: &[u8], + serial: &[u8], + subject: &[u8], + pub_key: &[u8], ) -> Result { - let pub_key_hash = match base64::decode(pub_key) { - Ok(pk) => { - let mut digest = Sha256::default(); - digest.input(&pk); - let digest_result = digest.result(); - base64::encode(&digest_result) - } - Err(_) => { - return Err(SecurityStateError::from( - "problem base64 decoding public key", - )); - } - }; + let mut digest = Sha256::default(); + digest.input(pub_key); + let pub_key_hash = digest.result(); - let subject_pubkey = format!("{}:{}:{}", PREFIX_REV_SPK, subject, pub_key_hash); - let issuer_serial = format!("{}:{}:{}", PREFIX_REV_IS, issuer, serial); + let subject_pubkey = make_key(PREFIX_REV_SPK, subject, &pub_key_hash); + let issuer_serial = make_key(PREFIX_REV_IS, issuer, serial); let st: i16 = match self.read_entry(&issuer_serial) { Ok(Some(value)) => value, @@ -233,7 +230,7 @@ impl SecurityState { Err(_) => { return Err(SecurityStateError::from( "problem reading revocation state (from issuer / serial)", - )) + )); } }; @@ -247,17 +244,17 @@ impl SecurityState { Err(_) => { return Err(SecurityStateError::from( "problem reading revocation state (from subject / pubkey)", - )) + )); } } } pub fn get_enrollment_state( &self, - issuer: &str, - serial: &str, + issuer: &[u8], + serial: &[u8], ) -> Result { - let issuer_serial = format!("{}:{}:{}", PREFIX_CRLITE, issuer, serial); + let issuer_serial = make_key(PREFIX_CRLITE, issuer, serial); match self.read_entry(&issuer_serial) { Ok(Some(value)) => Ok(value), Ok(None) => Ok(nsICertStorage::STATE_UNSET as i16), @@ -267,10 +264,10 @@ impl SecurityState { pub fn get_whitelist_state( &self, - issuer: &str, - serial: &str, + issuer: &[u8], + serial: &[u8], ) -> Result { - let issuer_serial = format!("{}:{}:{}", PREFIX_WL, issuer, serial); + let issuer_serial = make_key(PREFIX_WL, issuer, serial); match self.read_entry(&issuer_serial) { Ok(Some(value)) => Ok(value), Ok(None) => Ok(nsICertStorage::STATE_UNSET as i16), @@ -407,7 +404,7 @@ fn read_int_pref(name: &str) -> Result { _ => { return Err(SecurityStateError::from( "could not get preferences service", - )) + )); } }; @@ -429,7 +426,7 @@ fn read_int_pref(name: &str) -> Result { if !res.succeeded() { match res.0 { r if r == nsresult::NS_ERROR_UNEXPECTED as u32 => (), - _ => return Err(SecurityStateError::from("could not read pref")), // nserror::nsresult(err), + _ => return Err(SecurityStateError::from("could not read pref")), } } Ok(pref_value) @@ -454,10 +451,18 @@ pub extern "C" fn cert_storage_constructor( } } +macro_rules! try_ns { + ($e:expr) => { + match $e { + Ok(value) => value, + Err(_) => return nserror::NS_ERROR_FAILURE, + } + }; +} + #[derive(xpcom)] #[xpimplements(nsICertStorage, nsIObserver)] #[refcnt = "atomic"] - struct InitCertStorage { security_state: RwLock, } @@ -507,15 +512,10 @@ impl CertStorage { if issuer.is_null() || serial.is_null() { return nserror::NS_ERROR_FAILURE; } - let mut ss = match self.security_state.write() { - Err(_) => return nserror::NS_ERROR_FAILURE, - Ok(write_guard) => write_guard, - }; - match ss.set_revocation_by_issuer_and_serial( - (*issuer).as_str_unchecked(), - (*serial).as_str_unchecked(), - state, - ) { + let issuer_decoded = try_ns!(base64::decode(&*issuer)); + let serial_decoded = try_ns!(base64::decode(&*serial)); + let mut ss = try_ns!(self.security_state.write()); + match ss.set_revocation_by_issuer_and_serial(&issuer_decoded, &serial_decoded, state) { Ok(_) => nserror::NS_OK, _ => nserror::NS_ERROR_FAILURE, } @@ -530,15 +530,10 @@ impl CertStorage { if subject.is_null() || pub_key_base64.is_null() { return nserror::NS_ERROR_FAILURE; } - let mut ss = match self.security_state.write() { - Err(_) => return nserror::NS_ERROR_FAILURE, - Ok(write_guard) => write_guard, - }; - match ss.set_revocation_by_subject_and_pub_key( - (*subject).as_str_unchecked(), - (*pub_key_base64).as_str_unchecked(), - state, - ) { + let subject_decoded = try_ns!(base64::decode(&*subject)); + let pub_key_decoded = try_ns!(base64::decode(&*pub_key_base64)); + let mut ss = try_ns!(self.security_state.write()); + match ss.set_revocation_by_subject_and_pub_key(&subject_decoded, &pub_key_decoded, state) { Ok(_) => nserror::NS_OK, _ => nserror::NS_ERROR_FAILURE, } @@ -553,15 +548,10 @@ impl CertStorage { if issuer.is_null() || serial.is_null() { return nserror::NS_ERROR_FAILURE; } - let mut ss = match self.security_state.write() { - Err(_) => return nserror::NS_ERROR_FAILURE, - Ok(write_guard) => write_guard, - }; - match ss.set_enrollment( - (*issuer).as_str_unchecked(), - (*serial).as_str_unchecked(), - state, - ) { + let issuer_decoded = try_ns!(base64::decode(&*issuer)); + let serial_decoded = try_ns!(base64::decode(&*serial)); + let mut ss = try_ns!(self.security_state.write()); + match ss.set_enrollment(&issuer_decoded, &serial_decoded, state) { Ok(_) => nserror::NS_OK, _ => nserror::NS_ERROR_FAILURE, } @@ -576,15 +566,10 @@ impl CertStorage { if issuer.is_null() || serial.is_null() { return nserror::NS_ERROR_FAILURE; } - let mut ss = match self.security_state.write() { - Err(_) => return nserror::NS_ERROR_FAILURE, - Ok(write_guard) => write_guard, - }; - match ss.set_whitelist( - (*issuer).as_str_unchecked(), - (*serial).as_str_unchecked(), - state, - ) { + let issuer_decoded = try_ns!(base64::decode(&*issuer)); + let serial_decoded = try_ns!(base64::decode(&*serial)); + let mut ss = try_ns!(self.security_state.write()); + match ss.set_whitelist(&issuer_decoded, &serial_decoded, state) { Ok(_) => nserror::NS_OK, _ => nserror::NS_ERROR_FAILURE, } @@ -601,18 +586,20 @@ impl CertStorage { if issuer.is_null() || serial.is_null() || subject.is_null() || pub_key_base64.is_null() { return nserror::NS_ERROR_FAILURE; } - - let ss = match self.security_state.read() { - Err(_) => return nserror::NS_ERROR_FAILURE, - Ok(read_guard) => read_guard, - }; - + // TODO (bug 1535752): If we're calling this function when we already have binary data (e.g. + // in a TrustDomain::GetCertTrust callback), we should be able to pass in the binary data + // directly. See also bug 1535486. + let issuer_decoded = try_ns!(base64::decode(&*issuer)); + let serial_decoded = try_ns!(base64::decode(&*serial)); + let subject_decoded = try_ns!(base64::decode(&*subject)); + let pub_key_decoded = try_ns!(base64::decode(&*pub_key_base64)); + let ss = try_ns!(self.security_state.read()); *state = nsICertStorage::STATE_UNSET as i16; match ss.get_revocation_state( - (*issuer).as_str_unchecked(), - (*serial).as_str_unchecked(), - (*subject).as_str_unchecked(), - (*pub_key_base64).as_str_unchecked(), + &issuer_decoded, + &serial_decoded, + &subject_decoded, + &pub_key_decoded, ) { Ok(st) => { *state = st; @@ -628,14 +615,14 @@ impl CertStorage { serial: *const nsACString, state: *mut i16, ) -> nserror::nsresult { - let ss = match self.security_state.read() { - Err(_) => return nserror::NS_ERROR_FAILURE, - Ok(read_guard) => read_guard, - }; - + if issuer.is_null() || serial.is_null() { + return nserror::NS_ERROR_FAILURE; + } + let issuer_decoded = try_ns!(base64::decode(&*issuer)); + let serial_decoded = try_ns!(base64::decode(&*serial)); + let ss = try_ns!(self.security_state.read()); *state = nsICertStorage::STATE_UNSET as i16; - - match ss.get_enrollment_state((*issuer).as_str_unchecked(), (*serial).as_str_unchecked()) { + match ss.get_enrollment_state(&issuer_decoded, &serial_decoded) { Ok(st) => { *state = st; nserror::NS_OK @@ -650,14 +637,14 @@ impl CertStorage { serial: *const nsACString, state: *mut i16, ) -> nserror::nsresult { - let ss = match self.security_state.read() { - Err(_) => return nserror::NS_ERROR_FAILURE, - Ok(read_guard) => read_guard, - }; - + if issuer.is_null() || serial.is_null() { + return nserror::NS_ERROR_FAILURE; + } + let issuer_decoded = try_ns!(base64::decode(&*issuer)); + let serial_decoded = try_ns!(base64::decode(&*serial)); + let ss = try_ns!(self.security_state.read()); *state = nsICertStorage::STATE_UNSET as i16; - - match ss.get_whitelist_state((*issuer).as_str_unchecked(), (*serial).as_str_unchecked()) { + match ss.get_whitelist_state(&issuer_decoded, &serial_decoded) { Ok(st) => { *state = st; nserror::NS_OK @@ -668,12 +655,7 @@ impl CertStorage { unsafe fn IsBlocklistFresh(&self, fresh: *mut bool) -> nserror::nsresult { *fresh = false; - - let ss = match self.security_state.read() { - Err(_) => return nserror::NS_ERROR_FAILURE, - Ok(read_guard) => read_guard, - }; - + let ss = try_ns!(self.security_state.read()); *fresh = match ss.is_blocklist_fresh() { Ok(is_fresh) => is_fresh, Err(_) => false, @@ -684,12 +666,7 @@ impl CertStorage { unsafe fn IsWhitelistFresh(&self, fresh: *mut bool) -> nserror::nsresult { *fresh = false; - - let ss = match self.security_state.read() { - Err(_) => return nserror::NS_ERROR_FAILURE, - Ok(read_guard) => read_guard, - }; - + let ss = try_ns!(self.security_state.read()); *fresh = match ss.is_whitelist_fresh() { Ok(is_fresh) => is_fresh, Err(_) => false, @@ -700,12 +677,7 @@ impl CertStorage { unsafe fn IsEnrollmentFresh(&self, fresh: *mut bool) -> nserror::nsresult { *fresh = false; - - let ss = match self.security_state.read() { - Err(_) => return nserror::NS_ERROR_FAILURE, - Ok(read_guard) => read_guard, - }; - + let ss = try_ns!(self.security_state.read()); *fresh = match ss.is_enrollment_fresh() { Ok(is_fresh) => is_fresh, Err(_) => false, @@ -744,19 +716,13 @@ impl CertStorage { _ => return nserror::NS_ERROR_FAILURE, }; - let res = prefs.GetIntPref( - (&pref_name).as_ptr(), - (&mut pref_value) as *mut i32, - ); + let res = prefs.GetIntPref((&pref_name).as_ptr(), (&mut pref_value) as *mut i32); if !res.succeeded() { return res; } - let mut ss = match self.security_state.write() { - Err(_) => return nserror::NS_ERROR_FAILURE, - Ok(write_guard) => write_guard, - }; + let mut ss = try_ns!(self.security_state.write()); ss.pref_seen(name_string.as_str_unchecked(), pref_value); } _ => (), diff --git a/testing/talos/talos/xtalos/xperf_whitelist.json b/testing/talos/talos/xtalos/xperf_whitelist.json index a5353c4e28cd..e08df5a77930 100644 --- a/testing/talos/talos/xtalos/xperf_whitelist.json +++ b/testing/talos/talos/xtalos/xperf_whitelist.json @@ -524,6 +524,12 @@ "minbytes": 0, "maxbytes": 32768 }, + "{profile}\\security_state\\data.mdb": { + "mincount": 0, + "maxcount": 4, + "minbytes": 0, + "maxbytes": 608 + }, "{profile}\\sessioncheckpoints.json": { "mincount": 0, "maxcount": 2,