From 1a1341430b87b84e6917608f54ea1069ab6659d9 Mon Sep 17 00:00:00 2001 From: Dana Keeler Date: Thu, 6 Jun 2019 16:42:41 +0000 Subject: [PATCH] bug 1488865 - import CRLite enrollment state r=jcj,KevinJacobs This patch saves the CRLite enrollment state of every preloaded intermediate to cert_storage. This is an intermediate (hah) step towards actually checking CRLite state. We still have to implement downloading and updating the CRLite bloom filter cascades and implement checking these filters when we encounter a certificate issued from an enrolled intermediate (this work will be done in future bugs). Differential Revision: https://phabricator.services.mozilla.com/D33074 --HG-- extra : moz-landing-system : lando --- .../manager/ssl/RemoteSecuritySettings.jsm | 43 +++++++- security/manager/ssl/cert_storage/src/lib.rs | 101 ++++++++++++++++-- security/manager/ssl/nsICertStorage.idl | 34 ++++++ .../tests/unit/test_cert_storage_broken_db.js | 18 ++++ .../tests/unit/test_cert_storage_direct.js | 95 ++++++++++++++++ .../unit/test_cert_storage_preexisting.js | 8 ++ .../test_cert_storage_preexisting/data.mdb | Bin 36864 -> 45056 bytes .../test_cert_storage_preexisting/lock.mdb | Bin 8192 -> 8192 bytes .../tests/unit/test_intermediate_preloads.js | 50 ++++++++- security/manager/ssl/tests/unit/xpcshell.ini | 4 +- 10 files changed, 332 insertions(+), 21 deletions(-) diff --git a/security/manager/ssl/RemoteSecuritySettings.jsm b/security/manager/ssl/RemoteSecuritySettings.jsm index 1dcd36ebebfb..97cefacd9878 100644 --- a/security/manager/ssl/RemoteSecuritySettings.jsm +++ b/security/manager/ssl/RemoteSecuritySettings.jsm @@ -85,6 +85,15 @@ function bytesToString(bytes) { return String.fromCharCode.apply(null, bytes); } +class CRLiteState { + constructor(subject, spkiHash, state) { + this.subject = subject; + this.spkiHash = spkiHash; + this.state = state; + } +} +CRLiteState.prototype.QueryInterface = ChromeUtils.generateQI([Ci.nsICRLiteState]); + class CertInfo { constructor(cert, subject) { this.cert = cert; @@ -224,11 +233,7 @@ this.RemoteSecuritySettings = class RemoteSecuritySettings { } // This method returns a promise to RemoteSettingsClient.maybeSync method. - async onSync(event) { - const { - data: {deleted}, - } = event; - + async onSync({ data: { current, created, updated, deleted } }) { if (!Services.prefs.getBoolPref(INTERMEDIATES_ENABLED_PREF, true)) { log.debug("Intermediate Preloading is disabled"); return; @@ -236,6 +241,34 @@ this.RemoteSecuritySettings = class RemoteSecuritySettings { log.debug(`Removing ${deleted.length} Intermediate certificates`); await this.removeCerts(deleted); + + let certStorage = Cc["@mozilla.org/security/certstorage;1"].getService(Ci.nsICertStorage); + let hasPriorCRLiteData = await new Promise((resolve) => { + certStorage.hasPriorData(Ci.nsICertStorage.DATA_TYPE_CRLITE, (rv, hasPriorData) => { + if (rv == Cr.NS_OK) { + resolve(hasPriorData); + } else { + resolve(false); + } + }); + }); + if (!hasPriorCRLiteData) { + deleted = []; + updated = []; + created = current; + } + const toAdd = created.concat(updated.map(u => u.new)); + let entries = []; + for (let entry of deleted) { + entries.push(new CRLiteState(entry.subjectDN, entry.pubKeyHash, + Ci.nsICertStorage.STATE_UNSET)); + } + for (let entry of toAdd) { + entries.push(new CRLiteState(entry.subjectDN, entry.pubKeyHash, + entry.crlite_enrolled ? Ci.nsICertStorage.STATE_ENFORCE + : Ci.nsICertStorage.STATE_UNSET)); + } + await new Promise((resolve) => certStorage.setCRLiteState(entries, resolve)); } /** diff --git a/security/manager/ssl/cert_storage/src/lib.rs b/security/manager/ssl/cert_storage/src/lib.rs index 3aaed181843f..a968a0fa24f4 100644 --- a/security/manager/ssl/cert_storage/src/lib.rs +++ b/security/manager/ssl/cert_storage/src/lib.rs @@ -47,7 +47,7 @@ use std::time::{Duration, SystemTime}; use storage_variant::VariantType; use thin_vec::ThinVec; use xpcom::interfaces::{ - nsICertInfo, nsICertStorage, nsICertStorageCallback, nsIFile, + nsICRLiteState, nsICertInfo, nsICertStorage, nsICertStorageCallback, nsIFile, nsIIssuerAndSerialRevocationState, nsIObserver, nsIPrefBranch, nsIRevocationState, nsISubjectAndPubKeyRevocationState, nsISupports, nsIThread, }; @@ -55,6 +55,7 @@ use xpcom::{nsIID, GetterAddrefs, RefPtr, ThreadBoundRefPtr, XpCom}; const PREFIX_REV_IS: &str = "is"; const PREFIX_REV_SPK: &str = "spk"; +const PREFIX_CRLITE: &str = "crlite"; const PREFIX_SUBJECT: &str = "subject"; const PREFIX_CERT: &str = "cert"; const PREFIX_DATA_TYPE: &str = "datatype"; @@ -289,9 +290,10 @@ impl SecurityState { } } - pub fn set_revocations( + pub fn set_batch_state( &mut self, entries: &[(Vec, i16)], + typ: u8, ) -> Result<(), SecurityStateError> { self.reopen_store_read_write()?; { @@ -300,13 +302,10 @@ impl SecurityState { None => return Err(SecurityStateError::from("env and store not initialized?")), }; let mut writer = env_and_store.env.write()?; - // Make a note that we have prior revocation data now. + // Make a note that we have prior data of the given type now. env_and_store.store.put( &mut writer, - &make_key!( - PREFIX_DATA_TYPE, - &[nsICertStorage::DATA_TYPE_REVOCATION as u8] - ), + &make_key!(PREFIX_DATA_TYPE, &[typ]), &Value::Bool(true), )?; @@ -361,6 +360,23 @@ impl SecurityState { } } + pub fn get_crlite_state( + &self, + subject: &[u8], + pub_key: &[u8], + ) -> Result { + let mut digest = Sha256::default(); + digest.input(pub_key); + let pub_key_hash = digest.result(); + + let subject_pubkey = make_key!(PREFIX_CRLITE, subject, &pub_key_hash); + match self.read_entry(&subject_pubkey) { + Ok(Some(value)) => Ok(value), + Ok(None) => Ok(nsICertStorage::STATE_UNSET as i16), + Err(_) => Err(SecurityStateError::from("problem reading crlite state")), + } + } + pub fn is_data_fresh( &self, update_pref: &str, @@ -1099,7 +1115,7 @@ impl CertStorage { let task = Box::new(SecurityStateTask::new( &*callback, &self.security_state, - move |ss| ss.set_revocations(&entries), + move |ss| ss.set_batch_state(&entries, nsICertStorage::DATA_TYPE_REVOCATION as u8), )); let thread = try_ns!(self.thread.lock()); let runnable = try_ns!(TaskRunnable::new("SetRevocations", task)); @@ -1116,7 +1132,7 @@ impl CertStorage { state: *mut i16, ) -> nserror::nsresult { // TODO (bug 1541212): We really want to restrict this to non-main-threads only, but we - // can't do so until bug 1406854 and bug 1534600 are fixed. + // can't do so until bug 1406854 is fixed. if issuer.is_null() || serial.is_null() || subject.is_null() || pub_key.is_null() { return NS_ERROR_NULL_POINTER; } @@ -1143,6 +1159,71 @@ impl CertStorage { NS_OK } + unsafe fn SetCRLiteState( + &self, + crlite_state: *const ThinVec>, + callback: *const nsICertStorageCallback, + ) -> nserror::nsresult { + if !is_main_thread() { + return NS_ERROR_NOT_SAME_THREAD; + } + if crlite_state.is_null() || callback.is_null() { + return NS_ERROR_NULL_POINTER; + } + + let crlite_state = &*crlite_state; + let mut crlite_entries = Vec::with_capacity(crlite_state.len()); + + // By continuing when an nsICRLiteState attribute value is invalid, we prevent errors + // relating to individual entries from causing sync to fail. + for crlite_entry in crlite_state { + let mut state: i16 = 0; + try_ns!(crlite_entry.GetState(&mut state).to_result(), or continue); + + let mut subject = nsCString::new(); + try_ns!(crlite_entry.GetSubject(&mut *subject).to_result(), or continue); + let subject = try_ns!(base64::decode(&subject), or continue); + + let mut pub_key_hash = nsCString::new(); + try_ns!(crlite_entry.GetSpkiHash(&mut *pub_key_hash).to_result(), or continue); + let pub_key_hash = try_ns!(base64::decode(&pub_key_hash), or continue); + + crlite_entries.push((make_key!(PREFIX_CRLITE, &subject, &pub_key_hash), state)); + } + + let task = Box::new(SecurityStateTask::new( + &*callback, + &self.security_state, + move |ss| ss.set_batch_state(&crlite_entries, nsICertStorage::DATA_TYPE_CRLITE as u8), + )); + let thread = try_ns!(self.thread.lock()); + let runnable = try_ns!(TaskRunnable::new("SetCRLiteState", task)); + try_ns!(runnable.dispatch(&*thread)); + NS_OK + } + + unsafe fn GetCRLiteState( + &self, + subject: *const ThinVec, + pub_key: *const ThinVec, + state: *mut i16, + ) -> nserror::nsresult { + // TODO (bug 1541212): We really want to restrict this to non-main-threads only, but we + // can't do so until bug 1406854 is fixed. + if subject.is_null() || pub_key.is_null() { + return NS_ERROR_NULL_POINTER; + } + *state = nsICertStorage::STATE_UNSET as i16; + let ss = get_security_state!(self); + match ss.get_crlite_state(&*subject, &*pub_key) { + Ok(st) => { + *state = st; + NS_OK + } + _ => NS_ERROR_FAILURE, + } + } + unsafe fn AddCerts( &self, certs: *const ThinVec>, @@ -1212,7 +1293,7 @@ impl CertStorage { certs: *mut ThinVec>, ) -> nserror::nsresult { // TODO (bug 1541212): We really want to restrict this to non-main-threads only, but we - // can't do so until bug 1406854 and bug 1534600 are fixed. + // can't do so until bug 1406854 is fixed. if subject.is_null() || certs.is_null() { return NS_ERROR_NULL_POINTER; } diff --git a/security/manager/ssl/nsICertStorage.idl b/security/manager/ssl/nsICertStorage.idl index 74f516bdc4df..e6126290c0bb 100644 --- a/security/manager/ssl/nsICertStorage.idl +++ b/security/manager/ssl/nsICertStorage.idl @@ -55,6 +55,21 @@ interface nsISubjectAndPubKeyRevocationState : nsIRevocationState { readonly attribute ACString pubKey; }; +/** + * An interface representing the CRLite enrollment state of a certificate + * identified by its subject and subject public key info hash. + * subject is a base 64-encoded DER subject distinguished name. + * spkiHash is a base 64-encoded SHA-256 hash of a DER subject public key info. + * state is nsICertStorage.STATE_ENFORCE or STATE_UNSET, meaning the certificate + * is or is not enrolled in CRLite, respectively. + */ +[scriptable, uuid(5d0d22be-185f-4cf0-b73b-c5a911273e77)] +interface nsICRLiteState : nsISupports { + readonly attribute ACString subject; + readonly attribute ACString spkiHash; + readonly attribute short state; +}; + /** * An interface representing a certificate to add to storage. Consists of the * base64-encoded DER bytes of the certificate (cert), the base64-encoded DER @@ -75,6 +90,7 @@ interface nsICertInfo : nsISupports { interface nsICertStorage : nsISupports { const octet DATA_TYPE_REVOCATION = 1; const octet DATA_TYPE_CERTIFICATE = 2; + const octet DATA_TYPE_CRLITE = 3; /** * Asynchronously check if the backing storage has stored data of the given @@ -124,6 +140,24 @@ interface nsICertStorage : nsISupports { [must_use] boolean isBlocklistFresh(); + /** + * Asynchronously set a batch of CRLite enrollment state. See the + * documentation for nsICRLiteState. + * Must only be called from the main thread. + */ + [must_use] + void setCRLiteState(in Array crliteState, + in nsICertStorageCallback callback); + + /** + * Get the CRLite enrollment state of a certificate identified by the given + * subject distinguished name and subject public key info (both as DER bytes). + * STATE_ENFORCE indicates the certificate is enrolled, whereas STATE_UNSET + * indicates it is not. + */ + [must_use] + short getCRLiteState(in Array subject, in Array spki); + /** * Trust flags to use when adding a adding a certificate. * TRUST_INHERIT indicates a certificate inherits trust from another diff --git a/security/manager/ssl/tests/unit/test_cert_storage_broken_db.js b/security/manager/ssl/tests/unit/test_cert_storage_broken_db.js index 35dd10630a78..237b3650b744 100644 --- a/security/manager/ssl/tests/unit/test_cert_storage_broken_db.js +++ b/security/manager/ssl/tests/unit/test_cert_storage_broken_db.js @@ -32,6 +32,13 @@ async function check_has_prior_cert_data(certStorage, expectedResult) { `should ${expectedResult ? "have" : "not have"} prior cert data`); } +async function check_has_prior_crlite_data(certStorage, expectedResult) { + let hasPriorCRLiteData = await call_has_prior_data(certStorage, + Ci.nsICertStorage.DATA_TYPE_CRLITE); + Assert.equal(hasPriorCRLiteData, expectedResult, + `should ${expectedResult ? "have" : "not have"} prior CRLite data`); +} + add_task({ skip_if: () => !AppConstants.MOZ_NEW_CERT_STORAGE, }, async function() { @@ -44,6 +51,7 @@ add_task({ let certStorage = Cc["@mozilla.org/security/certstorage;1"].getService(Ci.nsICertStorage); check_has_prior_revocation_data(certStorage, false); check_has_prior_cert_data(certStorage, false); + check_has_prior_crlite_data(certStorage, false); let result = await new Promise((resolve) => { certStorage.setRevocations([], resolve); @@ -52,6 +60,7 @@ add_task({ check_has_prior_revocation_data(certStorage, true); check_has_prior_cert_data(certStorage, false); + check_has_prior_crlite_data(certStorage, false); result = await new Promise((resolve) => { certStorage.addCerts([], resolve); @@ -60,4 +69,13 @@ add_task({ check_has_prior_revocation_data(certStorage, true); check_has_prior_cert_data(certStorage, true); + check_has_prior_crlite_data(certStorage, false); + + result = await new Promise((resolve) => { + certStorage.setCRLiteState([], resolve); + }); + Assert.equal(result, Cr.NS_OK, "setCRLiteState should succeed"); + check_has_prior_revocation_data(certStorage, true); + check_has_prior_cert_data(certStorage, true); + check_has_prior_crlite_data(certStorage, true); }); diff --git a/security/manager/ssl/tests/unit/test_cert_storage_direct.js b/security/manager/ssl/tests/unit/test_cert_storage_direct.js index c00d1e60a6cd..6756dfe43233 100644 --- a/security/manager/ssl/tests/unit/test_cert_storage_direct.js +++ b/security/manager/ssl/tests/unit/test_cert_storage_direct.js @@ -155,3 +155,98 @@ add_task({ storedCerts = certStorage.findCertsBySubject(stringToArray("batch subject to remove")); Assert.equal(storedCerts.length, 0, "shouldn't have any certificates now"); }); + +class CRLiteState { + constructor(subject, spkiHash, state) { + this.subject = btoa(subject); + this.spkiHash = spkiHash; + this.state = state; + } +} +if (AppConstants.MOZ_NEW_CERT_STORAGE) { + CRLiteState.prototype.QueryInterface = ChromeUtils.generateQI([Ci.nsICRLiteState]); +} + +async function addCRLiteState(state) { + let result = await new Promise((resolve) => { + certStorage.setCRLiteState(state, resolve); + }); + Assert.equal(result, Cr.NS_OK, "setCRLiteState should succeed"); +} + +add_task({ + skip_if: () => !AppConstants.MOZ_NEW_CERT_STORAGE, +}, async function test_crlite_state() { + // echo -n "some spki 1" | sha256sum | xxd -r -p | base64 + let crliteState1 = new CRLiteState("some subject 1", + "bDlKlhR5ptlvuxclnZ3RQHznG8/3pgIybrRJ/Zvn9L8=", + Ci.nsICertStorage.STATE_ENFORCE); + // echo -n "some spki 2" | sha256sum | xxd -r -p | base64 + let crliteState2 = new CRLiteState("some subject 2", + "ZlXvlHhtdx4yKwkhZqg7Opv5T1ofwzorlsCoLf0wnlY=", + Ci.nsICertStorage.STATE_UNSET); + // echo -n "some spki 3" | sha256sum | xxd -r -p | base64 + let crliteState3 = new CRLiteState("some subject 3", + "pp1SRn6njaHX/c+b2uf82JPeBkWhPfTBp/Mxb3xkjRM=", + Ci.nsICertStorage.STATE_ENFORCE); + await addCRLiteState([crliteState1, crliteState2, crliteState3]); + + let state1 = certStorage.getCRLiteState(stringToArray("some subject 1"), + stringToArray("some spki 1")); + Assert.equal(state1, Ci.nsICertStorage.STATE_ENFORCE); + let state2 = certStorage.getCRLiteState(stringToArray("some subject 2"), + stringToArray("some spki 2")); + Assert.equal(state2, Ci.nsICertStorage.STATE_UNSET); + let state3 = certStorage.getCRLiteState(stringToArray("some subject 3"), + stringToArray("some spki 3")); + Assert.equal(state3, Ci.nsICertStorage.STATE_ENFORCE); + + // Check that if we never set the state of a particular subject/spki pair, we get "unset" when we + // look it up. + let stateNeverSet = certStorage.getCRLiteState(stringToArray("some unknown subject"), + stringToArray("some unknown spki")); + Assert.equal(stateNeverSet, Ci.nsICertStorage.STATE_UNSET); + + // "some subject 1"/"some spki 1" and "some subject 3"/"some spki 3" both have their CRLite state + // set. However, the combination of "some subject 3"/"some spki1" should not. + let stateDifferentSubjectSPKI = certStorage.getCRLiteState(stringToArray("some subject 3"), + stringToArray("some spki 1")); + Assert.equal(stateDifferentSubjectSPKI, Ci.nsICertStorage.STATE_UNSET); + + let anotherStateDifferentSubjectSPKI = certStorage.getCRLiteState(stringToArray("some subject 1"), + stringToArray("some spki 2")); + Assert.equal(anotherStateDifferentSubjectSPKI, Ci.nsICertStorage.STATE_UNSET); + let yetAnotherStateDifferentSubjectSPKI = + certStorage.getCRLiteState(stringToArray("some subject 2"), stringToArray("some spki 1")); + Assert.equal(yetAnotherStateDifferentSubjectSPKI, Ci.nsICertStorage.STATE_UNSET); + + crliteState3 = new CRLiteState("some subject 3", + "pp1SRn6njaHX/c+b2uf82JPeBkWhPfTBp/Mxb3xkjRM=", + Ci.nsICertStorage.STATE_UNSET); + await addCRLiteState([crliteState3]); + state3 = certStorage.getCRLiteState(stringToArray("some subject 3"), + stringToArray("some spki 3")); + Assert.equal(state3, Ci.nsICertStorage.STATE_UNSET); + + crliteState2 = new CRLiteState("some subject 2", + "ZlXvlHhtdx4yKwkhZqg7Opv5T1ofwzorlsCoLf0wnlY=", + Ci.nsICertStorage.STATE_ENFORCE); + await addCRLiteState([crliteState2]); + state2 = certStorage.getCRLiteState(stringToArray("some subject 2"), + stringToArray("some spki 2")); + Assert.equal(state2, Ci.nsICertStorage.STATE_ENFORCE); + + // Inserting a subject/spki pair with a state value outside of our expected + // values will succeed. However, since our data type is a signed 16-bit value, + // values outside that range will be truncated. The least significant 16 bits + // of 2013003773 are FFFD, which when interpreted as a signed 16-bit integer + // comes out to -3. + // echo -n "some spki 4" | sha256sum | xxd -r -p | base64 + let bogusValueState = new CRLiteState("some subject 4", + "1eA0++hCqzt8vpzREYSqHAqpEOLchZca1Gx8viCVYzc=", + 2013003773); + await addCRLiteState([bogusValueState]); + let bogusValueStateValue = certStorage.getCRLiteState(stringToArray("some subject 4"), + stringToArray("some spki 4")); + Assert.equal(bogusValueStateValue, -3); +}); diff --git a/security/manager/ssl/tests/unit/test_cert_storage_preexisting.js b/security/manager/ssl/tests/unit/test_cert_storage_preexisting.js index 2a2395ee532e..19ac635fb86b 100644 --- a/security/manager/ssl/tests/unit/test_cert_storage_preexisting.js +++ b/security/manager/ssl/tests/unit/test_cert_storage_preexisting.js @@ -37,4 +37,12 @@ add_task({ }); }); Assert.equal(hasPriorCertData, true, "should have prior cert data"); + + let hasPriorCRLiteData = await new Promise((resolve) => { + certStorage.hasPriorData(Ci.nsICertStorage.DATA_TYPE_CRLITE, (rv, hasPriorData) => { + Assert.equal(rv, Cr.NS_OK, "hasPriorData should succeed"); + resolve(hasPriorData); + }); + }); + Assert.equal(hasPriorCRLiteData, true, "should have prior cert data"); }); diff --git a/security/manager/ssl/tests/unit/test_cert_storage_preexisting/data.mdb b/security/manager/ssl/tests/unit/test_cert_storage_preexisting/data.mdb index ada575ff1277853215950c48d7fb3c6a19203e77..df4cb182a71352b80e1b5ac228adc59384171739 100644 GIT binary patch delta 207 zcmZozz|`=7iIHJ4qkumn*Tg`5K_&(W;DXXD8z;KzPrl|aCBO_7;e^tg3_jRTQs9{U zBVUAFgy9DN8~z)c1r3h!Pf}ppe6Lj8l BC}scv delta 235 zcmZp8z|^pSX@bWj0S-piiGlipEDR993Z 0, "should have some entries"); + // simulate a sync (syncAndDownload doesn't actually... sync.) + await remoteSecSetting.client.emit("sync", { + "data": { + current: data, + created: data, + deleted: [], + updated: [], + }, + }); + + let crliteStateAfter = certStorage.getCRLiteState( + intermediateCert.tbsCertificate.subject._der._bytes, + intermediateCert.tbsCertificate.subjectPublicKeyInfo._der._bytes); + equal(crliteStateAfter, Ci.nsICertStorage.STATE_ENFORCE, "crlite state should be set after"); + // check that ee cert 2 does not verify - since we don't know the issuer of // this certificate await checkCertErrorGeneric(certDB, ee_cert_2, SEC_ERROR_UNKNOWN_ISSUER, @@ -339,7 +374,14 @@ add_task({ let resultsBefore = certStorage.findCertsBySubject(stringToArray(atob(subject))); equal(resultsBefore.length, 1, "should find the intermediate in cert storage before"); // simulate a sync where we deleted the entry - await remoteSecSetting.client.emit("sync", { "data": { deleted: [ data[0] ] } }); + await remoteSecSetting.client.emit("sync", { + "data": { + current: [], + created: [], + deleted: [ data[0] ], + updated: [], + }, + }); let resultsAfter = certStorage.findCertsBySubject(stringToArray(atob(subject))); equal(resultsAfter.length, 0, "shouldn't find intermediate in cert storage now"); }); diff --git a/security/manager/ssl/tests/unit/xpcshell.ini b/security/manager/ssl/tests/unit/xpcshell.ini index 64f97b026beb..1f7156af2754 100644 --- a/security/manager/ssl/tests/unit/xpcshell.ini +++ b/security/manager/ssl/tests/unit/xpcshell.ini @@ -100,7 +100,7 @@ skip-if = toolkit == 'android' # have a way to test it on OS X. skip-if = os != 'win' [test_ev_certs.js] -tags = blocklist +tags = blocklist psm run-sequentially = hardcoded ports [test_forget_about_site_security_headers.js] skip-if = toolkit == 'android' @@ -111,7 +111,7 @@ skip-if = toolkit == 'android' [test_hmac.js] [test_intermediate_basic_usage_constraints.js] [test_intermediate_preloads.js] -tags = blocklist +tags = blocklist psm # Bug 1520297 - do something to handle tighter resource constraints on Android skip-if = toolkit == 'android' [test_imminent_distrust.js]