diff --git a/rust/pv/src/verify.rs b/rust/pv/src/verify.rs index 619494b8..ffa6e691 100644 --- a/rust/pv/src/verify.rs +++ b/rust/pv/src/verify.rs @@ -3,10 +3,11 @@ // Copyright IBM Corp. 2023 use core::slice; -use log::debug; +use log::{debug, trace}; +use openssl::error::ErrorStack; use openssl::stack::Stack; use openssl::x509::store::X509Store; -use openssl::x509::{CrlStatus, X509Ref, X509StoreContext, X509}; +use openssl::x509::{CrlStatus, X509NameRef, X509Ref, X509StoreContext, X509StoreContextRef, X509}; use openssl_extensions::crl::{StackableX509Crl, X509StoreContextExtension, X509StoreExtension}; #[cfg(not(test))] @@ -86,8 +87,8 @@ impl HkdVerifier for CertVerifier { if verified_crls.is_empty() { bail_hkd_verify!(NoCrl); } - for crl in &verified_crls { - match crl.get_by_cert(&hkd.to_owned()) { + for crl in verified_crls { + match crl.get_by_serial(hkd.serial_number()) { CrlStatus::NotRevoked => (), _ => bail_hkd_verify!(HdkRevoked), } @@ -98,21 +99,54 @@ impl HkdVerifier for CertVerifier { } impl CertVerifier { + fn quirk_crls( + ctx: &mut X509StoreContextRef, + subject: &X509NameRef, + ) -> Result, ErrorStack> { + match ctx.crls(subject) { + Ok(ret) if !ret.is_empty() => return Ok(ret), + _ => (), + } + + // Armonk/Poughkeepsie fixup + trace!("quirk_crls: Try Locality"); + if let Some(locality_subject) = helper::armonk_locality_fixup(subject) { + match ctx.crls(&locality_subject) { + Ok(ret) if !ret.is_empty() => return Ok(ret), + _ => (), + } + + // reorder + trace!("quirk_crls: Try Locality+Reorder"); + if let Ok(locality_ordered_subject) = helper::reorder_x509_names(&locality_subject) { + match ctx.crls(&locality_ordered_subject) { + Ok(ret) if !ret.is_empty() => return Ok(ret), + _ => (), + } + } + } + + // reorder unchanged loaciliy subject + trace!("quirk_crls: Try Reorder"); + if let Ok(ordered_subject) = helper::reorder_x509_names(subject) { + match ctx.crls(&ordered_subject) { + Ok(ret) if !ret.is_empty() => return Ok(ret), + _ => (), + } + } + // nothing found, return empty stack + Stack::new() + } + ///Download the CLRs that a HKD refers to. pub fn hkd_crls(&self, hkd: &X509Ref) -> Result> { let mut ctx = X509StoreContext::new()?; // Unfortunately we cannot use a dedicated function here and have to use a closure (E0434) // Otherwise, we cannot refer to self + // Search for local CRLs let mut crls = ctx.init_opt(&self.store, None, None, |ctx| { let subject = self.ibm_z_sign_key.subject_name(); - match ctx.crls(subject) { - Ok(crls) => Ok(crls), - _ => { - // reorder the name and try again - let broken_subj = helper::reorder_x509_names(subject)?; - ctx.crls(&broken_subj).or_else(helper::stack_err_hlp) - } - } + Self::quirk_crls(ctx, subject) })?; if !self.offline { diff --git a/rust/pv/src/verify/helper.rs b/rust/pv/src/verify/helper.rs index 6ac540b6..4dd08441 100644 --- a/rust/pv/src/verify/helper.rs +++ b/rust/pv/src/verify/helper.rs @@ -11,7 +11,7 @@ use openssl::{ error::ErrorStack, nid::Nid, ssl::SslFiletype, - stack::{Stack, Stackable}, + stack::Stack, x509::{ store::{File, X509Lookup, X509StoreBuilder, X509StoreRef}, verify::{X509VerifyFlags, X509VerifyParam}, @@ -20,6 +20,7 @@ use openssl::{ }, }; use openssl_extensions::akid::{AkidCheckResult, AkidExtension}; +use std::str::from_utf8; use std::{cmp::Ordering, ffi::c_int, usize}; /// Minimum security level for the keys/certificates used to establish a chain of @@ -34,7 +35,6 @@ const SECURITY_CHAIN_MAX_LEN: c_int = 2; /// verifies that the HKD /// * has enough security bits /// * is inside its validity period -/// * issuer name is the subject name of the [`sign_key`] /// * the Authority Key ID matches the Signing Key ID of the [`sign_key`] pub fn verify_hkd_options(hkd: &X509Ref, sign_key: &X509Ref) -> Result<()> { let hk_pkey = hkd.public_key()?; @@ -48,9 +48,6 @@ pub fn verify_hkd_options(hkd: &X509Ref, sign_key: &X509Ref) -> Result<()> { // verify that the hkd is still valid check_validity_period(hkd.not_before(), hkd.not_after())?; - // check if hkd.issuer_name == issuer.subject - check_x509_name_equal(sign_key.subject_name(), hkd.issuer_name())?; - // verify that the AKID of the hkd matches the SKID of the issuer if let Some(akid) = hkd.akid() { if akid.check(sign_key) != AkidCheckResult::OK { @@ -70,9 +67,6 @@ pub fn verify_crl(crl: &X509CrlRef, issuer: &X509Ref) -> Option<()> { return None; } } - - check_x509_name_equal(crl.issuer_name(), issuer.subject_name()).ok()?; - match crl.verify(issuer.public_key().ok()?.as_ref()).ok()? { true => Some(()), false => None, @@ -191,7 +185,8 @@ pub fn extract_ibm_sign_key(certs: Vec) -> Result<(X509, Stack)> { //Asn1StringRef::as_slice aka ASN1_STRING_get0_data gives a string without \0 delimiter const IBM_Z_COMMON_NAME: &[u8; 43usize] = b"International Business Machines Corporation"; const IBM_Z_COUNTRY_NAME: &[u8; 2usize] = b"US"; -const IBM_Z_LOCALITY_NAME: &[u8; 12usize] = b"Poughkeepsie"; +const IBM_Z_LOCALITY_NAME_POUGHKEEPSIE: &[u8; 12usize] = b"Poughkeepsie"; +const IBM_Z_LOCALITY_NAME_ARMONK: &[u8; 6usize] = b"Armonk"; const IBM_Z_ORGANIZATIONAL_UNIT_NAME_SUFFIX: &str = "Key Signing Service"; const IBM_Z_ORGANIZATION_NAME: &[u8; 43usize] = b"International Business Machines Corporation"; const IBM_Z_STATE: &[u8; 8usize] = b"New York"; @@ -210,7 +205,8 @@ fn is_ibm_signing_cert(cert: &X509) -> bool { if subj.entries().count() != IMB_Z_ENTRY_COUNT || !name_data_eq(subj, Nid::COUNTRYNAME, IBM_Z_COUNTRY_NAME) || !name_data_eq(subj, Nid::STATEORPROVINCENAME, IBM_Z_STATE) - || !name_data_eq(subj, Nid::LOCALITYNAME, IBM_Z_LOCALITY_NAME) + || !(name_data_eq(subj, Nid::LOCALITYNAME, IBM_Z_LOCALITY_NAME_POUGHKEEPSIE) + || name_data_eq(subj, Nid::LOCALITYNAME, IBM_Z_LOCALITY_NAME_ARMONK)) || !name_data_eq(subj, Nid::ORGANIZATIONNAME, IBM_Z_ORGANIZATION_NAME) || !name_data_eq(subj, Nid::COMMONNAME, IBM_Z_COMMON_NAME) { @@ -354,24 +350,6 @@ fn check_validity_period(not_before: &Asn1TimeRef, not_after: &Asn1TimeRef) -> R } } -fn check_x509_name_equal(lhs: &X509NameRef, rhs: &X509NameRef) -> Result<()> { - if lhs.entries().count() != rhs.entries().count() { - bail_hkd_verify!(IssuerMismatch); - } - - for l in lhs.entries() { - // search for the matching value in the rhs names - // found none? -> names are not equal - if !rhs - .entries() - .any(|r| l.data().as_slice() == r.data().as_slice()) - { - bail_hkd_verify!(IssuerMismatch); - } - } - Ok(()) -} - const NIDS_CORRECT_ORDER: [Nid; 6] = [ Nid::COUNTRYNAME, Nid::ORGANIZATIONNAME, @@ -394,13 +372,28 @@ pub fn reorder_x509_names(subject: &X509NameRef) -> std::result::Result( - e: ErrorStack, -) -> std::result::Result, openssl::error::ErrorStack> { - match e.errors().len() { - 0 => Stack::::new(), - _ => Err(e), +/** +* Workaround for potential locality mismatches between CRLs and Certs +* # Return +* fixed subject or none if locality was not Armonk or any OpenSSL error +*/ +pub fn armonk_locality_fixup(subject: &X509NameRef) -> Option { + if !name_data_eq(subject, Nid::LOCALITYNAME, IBM_Z_LOCALITY_NAME_ARMONK) { + return None; } + + let mut ret = X509Name::builder().ok()?; + for entry in subject.entries() { + match entry.object().nid() { + nid @ Nid::LOCALITYNAME => ret + .append_entry_by_nid(nid, from_utf8(IBM_Z_LOCALITY_NAME_POUGHKEEPSIE).ok()?) + .ok()?, + _ => { + ret.append_entry(entry).ok()?; + } + } + } + Some(ret.build()) } #[cfg(test)] @@ -436,20 +429,6 @@ mod test { )); } - #[test] - fn x509_name_equal() { - let sign_crt = load_gen_cert("ibm.crt"); - let hkd = load_gen_cert("host.crt"); - let other = load_gen_cert("inter_ca.crt"); - - assert!(super::check_x509_name_equal(sign_crt.subject_name(), hkd.issuer_name()).is_ok(),); - - assert!(matches!( - super::check_x509_name_equal(other.subject_name(), hkd.subject_name()), - Err(Error::HkdVerify(IssuerMismatch)) - )); - } - #[test] fn is_ibm_z_sign_key() { let ibm_crt = load_gen_cert("ibm.crt"); diff --git a/rust/pv/src/verify/test.rs b/rust/pv/src/verify/test.rs index 15806b2e..2fd6bc34 100644 --- a/rust/pv/src/verify/test.rs +++ b/rust/pv/src/verify/test.rs @@ -74,16 +74,15 @@ fn dist_points() { assert_eq!(res, exp); } -fn verify(offline: bool, ibm_crt: &'static str, ibm_crl: &'static str) { +fn verify(offline: bool, ibm_crt: &'static str, ibm_crl: &'static str, hkd: &'static str) { let root_crt = get_cert_asset_path_string("root_ca.chained.crt"); let inter_crt = get_cert_asset_path_string("inter_ca.crt"); let inter_crl = get_cert_asset_path_string("inter_ca.crl"); let ibm_crt = get_cert_asset_path_string(ibm_crt); let ibm_crl = get_cert_asset_path_string(ibm_crl); let hkd_revoked = load_gen_cert("host_rev.crt"); - let hkd_inv = load_gen_cert("host_invalid_signing_key.crt"); let hkd_exp = load_gen_cert("host_crt_expired.crt"); - let hkd = load_gen_cert("host.crt"); + let hkd = load_gen_cert(hkd); let crls = &[ibm_crl, inter_crl]; let verifier = CertVerifier::new( @@ -102,11 +101,6 @@ fn verify(offline: bool, ibm_crt: &'static str, ibm_crl: &'static str) { Err(Error::HkdVerify(HdkRevoked)) )); - assert!(matches!( - verifier.verify(&hkd_inv), - Err(Error::HkdVerify(IssuerMismatch)) - )); - assert!(matches!( verifier.verify(&hkd_exp), Err(Error::HkdVerify(AfterValidity)) @@ -115,10 +109,40 @@ fn verify(offline: bool, ibm_crt: &'static str, ibm_crl: &'static str) { #[test] fn verify_online() { - verify(false, "ibm.crt", "ibm.crl") + verify(false, "ibm.crt", "ibm.crl", "host.crt") } #[test] fn verify_offline() { - verify(true, "ibm.crt", "ibm.crl") + verify(true, "ibm.crt", "ibm.crl", "host.crt") +} + +#[test] +fn verify_armonk_crt_online() { + verify(false, "ibm_armonk.crt", "ibm.crl", "host.crt") +} + +#[test] +fn verify_armonk_crt_offline() { + verify(true, "ibm_armonk.crt", "ibm.crl", "host.crt") +} + +#[test] +fn verify_armonk_crl_online() { + verify(false, "ibm_armonk.crt", "ibm_armonk.crl", "host.crt") +} + +#[test] +fn verify_armonk_crl_offline() { + verify(true, "ibm_armonk.crt", "ibm_armonk.crl", "host.crt") +} + +#[test] +fn verify_armonk_hkd_online() { + verify(false, "ibm_armonk.crt", "ibm_armonk.crl", "host_armonk.crt") +} + +#[test] +fn verify_armonk_hkd_offline() { + verify(true, "ibm_armonk.crt", "ibm_armonk.crl", "host_armonk.crt") }