From d9bcf18d4fe6c596889d9785d0dab8f78df27972 Mon Sep 17 00:00:00 2001 From: Sree Revoori Date: Tue, 27 Feb 2024 20:33:30 +0000 Subject: [PATCH] Fix all cert lint errors 1. Truncate SerialNumber to 64 bits 2. Count unused bits in KeyUsage 3. Add SubjectKeyIdentifier extension 4. Add AuthorityKeyIdentifier extension 5. Make cert lint errors/warns fatal in CI 6. Update caliptra-cfi revision fixes #74 --- Cargo.lock | 4 +- Cargo.toml | 6 +- dpe/fuzz/Cargo.lock | 4 +- dpe/src/commands/certify_key.rs | 26 +- dpe/src/x509.rs | 331 ++++++++++++++++++-- verification/testing/certifyKey.go | 90 +++++- verification/testing/certs.go | 41 +++ verification/testing/getCertificateChain.go | 5 + 8 files changed, 458 insertions(+), 49 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cc74590a..d06d109b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -176,7 +176,7 @@ checksum = "1fd0f2584146f6f2ef48085050886acf353beff7305ebd1ae69500e27c67f64b" [[package]] name = "caliptra-cfi-derive-git" version = "0.1.0" -source = "git+https://github.com/chipsalliance/caliptra-cfi.git?rev=9f315fcf11fe006e95e62149f54f98620e5244e7#9f315fcf11fe006e95e62149f54f98620e5244e7" +source = "git+https://github.com/chipsalliance/caliptra-cfi.git?rev=a98e499d279e81ae85881991b1e9eee354151189#a98e499d279e81ae85881991b1e9eee354151189" dependencies = [ "paste", "proc-macro2", @@ -187,7 +187,7 @@ dependencies = [ [[package]] name = "caliptra-cfi-lib-git" version = "0.1.0" -source = "git+https://github.com/chipsalliance/caliptra-cfi.git?rev=9f315fcf11fe006e95e62149f54f98620e5244e7#9f315fcf11fe006e95e62149f54f98620e5244e7" +source = "git+https://github.com/chipsalliance/caliptra-cfi.git?rev=a98e499d279e81ae85881991b1e9eee354151189#a98e499d279e81ae85881991b1e9eee354151189" [[package]] name = "cc" diff --git a/Cargo.toml b/Cargo.toml index a1f9bc95..0ead13d4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,7 +11,7 @@ members = [ ] [workspace.dependencies] -caliptra-cfi-lib-git = { git = "https://github.com/chipsalliance/caliptra-cfi.git", package = "caliptra-cfi-lib-git", rev = "9f315fcf11fe006e95e62149f54f98620e5244e7", default-features = false, features = ["cfi", "cfi-counter" ] } -caliptra-cfi-derive-git = { git = "https://github.com/chipsalliance/caliptra-cfi.git", package = "caliptra-cfi-derive-git", rev = "9f315fcf11fe006e95e62149f54f98620e5244e7"} +caliptra-cfi-lib-git = { git = "https://github.com/chipsalliance/caliptra-cfi.git", package = "caliptra-cfi-lib-git", rev = "a98e499d279e81ae85881991b1e9eee354151189", default-features = false, features = ["cfi", "cfi-counter" ] } +caliptra-cfi-derive-git = { git = "https://github.com/chipsalliance/caliptra-cfi.git", package = "caliptra-cfi-derive-git", rev = "a98e499d279e81ae85881991b1e9eee354151189"} zerocopy = "0.6.6" -openssl = "0.10.64" \ No newline at end of file +openssl = "0.10.64" diff --git a/dpe/fuzz/Cargo.lock b/dpe/fuzz/Cargo.lock index b5da6f9e..3b22080d 100644 --- a/dpe/fuzz/Cargo.lock +++ b/dpe/fuzz/Cargo.lock @@ -116,7 +116,7 @@ checksum = "14c189c53d098945499cdfa7ecc63567cf3886b3332b312a5b4585d8d3a6a610" [[package]] name = "caliptra-cfi-derive-git" version = "0.1.0" -source = "git+https://github.com/chipsalliance/caliptra-cfi.git?rev=9f315fcf11fe006e95e62149f54f98620e5244e7#9f315fcf11fe006e95e62149f54f98620e5244e7" +source = "git+https://github.com/chipsalliance/caliptra-cfi.git?rev=a98e499d279e81ae85881991b1e9eee354151189#a98e499d279e81ae85881991b1e9eee354151189" dependencies = [ "paste", "proc-macro2", @@ -127,7 +127,7 @@ dependencies = [ [[package]] name = "caliptra-cfi-lib-git" version = "0.1.0" -source = "git+https://github.com/chipsalliance/caliptra-cfi.git?rev=9f315fcf11fe006e95e62149f54f98620e5244e7#9f315fcf11fe006e95e62149f54f98620e5244e7" +source = "git+https://github.com/chipsalliance/caliptra-cfi.git?rev=a98e499d279e81ae85881991b1e9eee354151189#a98e499d279e81ae85881991b1e9eee354151189" [[package]] name = "cc" diff --git a/dpe/src/commands/certify_key.rs b/dpe/src/commands/certify_key.rs index d0d8be33..423f08d9 100644 --- a/dpe/src/commands/certify_key.rs +++ b/dpe/src/commands/certify_key.rs @@ -15,8 +15,8 @@ use caliptra_cfi_lib_git::cfi_launder; #[cfg(not(feature = "no-cfi"))] use caliptra_cfi_lib_git::{cfi_assert, cfi_assert_eq}; use cfg_if::cfg_if; -use crypto::Crypto; -use platform::{Platform, MAX_ISSUER_NAME_SIZE}; +use crypto::{Crypto, Hasher}; +use platform::{Platform, MAX_ISSUER_NAME_SIZE, MAX_SKI_SIZE}; #[repr(C)] #[derive(Debug, PartialEq, Eq, zerocopy::FromBytes, zerocopy::AsBytes)] @@ -109,10 +109,12 @@ impl CommandExecution for CertifyKeyCmd { let mut subj_serial = [0u8; DPE_PROFILE.get_hash_size() * 2]; env.crypto .get_pubkey_serial(DPE_PROFILE.alg_len(), &pub_key, &mut subj_serial)?; + // The serial number of the subject can be at most 64 bytes + let truncated_subj_serial = &subj_serial[..64]; let subject_name = Name { cn: DirectoryString::PrintableString(b"DPE Leaf"), - serial: DirectoryString::PrintableString(&subj_serial), + serial: DirectoryString::PrintableString(truncated_subj_serial), }; // Get TCI Nodes @@ -122,11 +124,26 @@ impl CommandExecution for CertifyKeyCmd { if tcb_count > MAX_HANDLES { return Err(DpeErrorCode::InternalError); } + + let mut key_identifier = [0u8; MAX_SKI_SIZE]; + // compute key identifier as SHA-256 hash of the DER encoded subject public key + let mut hasher = env.crypto.hash_initialize(crypto::AlgLen::Bit256)?; + hasher.update(&[0x04])?; + hasher.update(pub_key.x.bytes())?; + hasher.update(pub_key.y.bytes())?; + let hashed_pub_key = hasher.finish()?; + if hashed_pub_key.len() < MAX_SKI_SIZE { + return Err(DpeErrorCode::InternalError); + } + // truncate key identifier to 20 bytes + key_identifier.copy_from_slice(&hashed_pub_key.bytes()[..MAX_SKI_SIZE]); + let measurements = MeasurementData { label: &self.label, tci_nodes: &nodes[..tcb_count], is_ca: self.uses_is_ca(), supports_recursive: dpe.support.recursive(), + key_identifier, }; let mut issuer_name = [0u8; MAX_ISSUER_NAME_SIZE]; @@ -577,9 +594,10 @@ mod tests { env.crypto .get_pubkey_serial(DPE_PROFILE.alg_len(), &pub_key, &mut subj_serial) .unwrap(); + let truncated_subj_serial = &subj_serial[..64]; let subject_name = Name { cn: DirectoryString::PrintableString(b"DPE Leaf"), - serial: DirectoryString::PrintableString(&subj_serial), + serial: DirectoryString::PrintableString(truncated_subj_serial), }; let expected_subject_name = format!( "CN={}, serialNumber={}", diff --git a/dpe/src/x509.rs b/dpe/src/x509.rs index f1827ecf..3e1a20ff 100644 --- a/dpe/src/x509.rs +++ b/dpe/src/x509.rs @@ -12,7 +12,7 @@ use crate::{ }; use bitflags::bitflags; use crypto::{EcdsaPub, EcdsaSig}; -use platform::{CertValidity, SignerIdentifier}; +use platform::{CertValidity, SignerIdentifier, MAX_SKI_SIZE}; pub enum DirectoryString<'a> { PrintableString(&'a [u8]), @@ -49,6 +49,7 @@ pub struct MeasurementData<'a> { pub tci_nodes: &'a [TciNodeData], pub is_ca: bool, pub supports_recursive: bool, + pub key_identifier: [u8; MAX_SKI_SIZE], } pub struct CertWriter<'a> { @@ -90,23 +91,23 @@ impl CertWriter<'_> { const CMS_V3: u64 = 3; const CSR_V0: u64 = 0; - const ECDSA_OID: &[u8] = match DPE_PROFILE { + const ECDSA_OID: &'static [u8] = match DPE_PROFILE { // ECDSA with SHA256 DpeProfile::P256Sha256 => &[0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x04, 0x03, 0x02], // ECDSA with SHA384 DpeProfile::P384Sha384 => &[0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x04, 0x03, 0x03], }; - const EC_PUB_OID: &[u8] = &[0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x02, 0x01]; + const EC_PUB_OID: &'static [u8] = &[0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x02, 0x01]; - const CURVE_OID: &[u8] = match DPE_PROFILE { + const CURVE_OID: &'static [u8] = match DPE_PROFILE { // P256 DpeProfile::P256Sha256 => &[0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x03, 0x01, 0x07], // P384 DpeProfile::P384Sha384 => &[0x2B, 0x81, 0x04, 0x00, 0x22], }; - const HASH_OID: &[u8] = match DPE_PROFILE { + const HASH_OID: &'static [u8] = match DPE_PROFILE { // SHA256 DpeProfile::P256Sha256 => &[0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01], // SHA384 @@ -117,34 +118,42 @@ impl CertWriter<'_> { const RDN_SERIALNUMBER_OID: [u8; 3] = [0x55, 0x04, 0x05]; // tcg-dice-MultiTcbInfo 2.23.133.5.4.5 - const MULTI_TCBINFO_OID: &[u8] = &[0x67, 0x81, 0x05, 0x05, 0x04, 0x05]; + const MULTI_TCBINFO_OID: &'static [u8] = &[0x67, 0x81, 0x05, 0x05, 0x04, 0x05]; // tcg-dice-Ueid 2.23.133.5.4.4 - const UEID_OID: &[u8] = &[0x67, 0x81, 0x05, 0x05, 0x04, 0x04]; + const UEID_OID: &'static [u8] = &[0x67, 0x81, 0x05, 0x05, 0x04, 0x04]; // tcg-dice-kp-eca 2.23.133.5.4.100.12 - const ECA_OID: &[u8] = &[0x67, 0x81, 0x05, 0x05, 0x04, 0x64, 0x0C]; + const ECA_OID: &'static [u8] = &[0x67, 0x81, 0x05, 0x05, 0x04, 0x64, 0x0C]; // tcg-dice-kp-attestLoc 2.23.133.5.4.100.9 - const ATTEST_LOC_OID: &[u8] = &[0x67, 0x81, 0x05, 0x05, 0x04, 0x64, 0x09]; + const ATTEST_LOC_OID: &'static [u8] = &[0x67, 0x81, 0x05, 0x05, 0x04, 0x64, 0x09]; // RFC 5280 2.5.29.19 - const BASIC_CONSTRAINTS_OID: &[u8] = &[0x55, 0x1D, 0x13]; + const BASIC_CONSTRAINTS_OID: &'static [u8] = &[0x55, 0x1D, 0x13]; // RFC 5280 2.5.29.15 - const KEY_USAGE_OID: &[u8] = &[0x55, 0x1D, 0x0F]; + const KEY_USAGE_OID: &'static [u8] = &[0x55, 0x1D, 0x0F]; // RFC 5280 2.5.29.37 - const EXTENDED_KEY_USAGE_OID: &[u8] = &[0x55, 0x1D, 0x25]; + const EXTENDED_KEY_USAGE_OID: &'static [u8] = &[0x55, 0x1D, 0x25]; + + // RFC 5280 2.5.29.14 + const SUBJECT_KEY_IDENTIFIER_OID: &'static [u8] = &[0x55, 0x1D, 0x0E]; + + // RFC 5280 2.5.29.35 + const AUTHORITY_KEY_IDENTIFIER_OID: &'static [u8] = &[0x55, 0x1D, 0x23]; // RFC 5652 1.2.840.113549.1.7.2 - const ID_SIGNED_DATA_OID: &[u8] = &[0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, 0x07, 0x02]; + const ID_SIGNED_DATA_OID: &'static [u8] = + &[0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, 0x07, 0x02]; // RFC 5652 1.2.840.113549.1.7.1 - const ID_DATA_OID: &[u8] = &[0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, 0x07, 0x01]; + const ID_DATA_OID: &'static [u8] = &[0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, 0x07, 0x01]; // RFC 2985 1.2.840.113549.1.9.14 - const EXTENSION_REQUEST_OID: &[u8] = &[0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, 0x09, 0x0E]; + const EXTENSION_REQUEST_OID: &'static [u8] = + &[0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, 0x09, 0x0E]; /// Build new CertWriter that writes output to `cert` /// @@ -444,17 +453,76 @@ impl CertWriter<'_> { Self::get_structure_size(size, tagged) } + /// Get the size of an subjectKeyIdentifier extension, including the extension + /// OID and critical bits. + fn get_subject_key_identifier_extension_size( + measurements: &MeasurementData, + tagged: bool, + is_x509: bool, + ) -> Result { + if !measurements.is_ca || !is_x509 { + return Ok(0); + } + let ski_size = measurements.key_identifier.len(); + + // Extension data is sequence -> octet string. To compute size, wrap + // in tagging twice. + let ext_size = Self::get_structure_size(ski_size, /*tagged=*/ true)?; + let size = Self::get_structure_size(Self::SUBJECT_KEY_IDENTIFIER_OID.len(), /*tagged=*/true)? // Extension OID + + Self::get_structure_size(Self::BOOL_SIZE, /*tagged=*/true)? // Critical bool + + Self::get_structure_size(ext_size, /*tagged=*/true)?; // OCTET STRING + + Self::get_structure_size(size, tagged) + } + + /// Get the size of an authorityKeyIdentifier extension, including the extension + /// OID and critical bits. + fn get_authority_key_identifier_extension_size( + measurements: &MeasurementData, + tagged: bool, + is_x509: bool, + ) -> Result { + if !measurements.is_ca || !is_x509 { + return Ok(0); + } + let aki_size = Self::get_key_identifier_size( + &measurements.key_identifier, + true, + /*explicit=*/ true, + )?; + + // Extension data is sequence -> octet string. To compute size, wrap + // in tagging twice. + let ext_size = Self::get_structure_size(aki_size, /*tagged=*/ true)?; + let size = Self::get_structure_size(Self::AUTHORITY_KEY_IDENTIFIER_OID.len(), /*tagged=*/true)? // Extension OID + + Self::get_structure_size(Self::BOOL_SIZE, /*tagged=*/true)? // Critical bool + + Self::get_structure_size(ext_size, /*tagged=*/true)?; // OCTET STRING + + Self::get_structure_size(size, tagged) + } + /// Get the size of the TBS Extensions field. fn get_extensions_size( measurements: &MeasurementData, tagged: bool, explicit: bool, + is_x509: bool, ) -> Result { let mut size = Self::get_multi_tcb_info_size(measurements, /*tagged=*/ true)? + Self::get_ueid_size(measurements, /*tagged=*/ true)? + Self::get_basic_constraints_size(/*tagged=*/ true)? + Self::get_key_usage_size(/*tagged=*/ true)? - + Self::get_extended_key_usage_size(measurements, /*tagged=*/ true)?; + + Self::get_extended_key_usage_size(measurements, /*tagged=*/ true)? + + Self::get_subject_key_identifier_extension_size( + measurements, + /*tagged=*/ true, + is_x509, + )? + + Self::get_authority_key_identifier_extension_size( + measurements, + /*tagged=*/ true, + is_x509, + )?; // Determine whether to include the explicit tag wrapping in the size calculation size = Self::get_structure_size(size, /*tagged=*/ explicit)?; @@ -484,6 +552,7 @@ impl CertWriter<'_> { measurements, /*tagged=*/ true, /*explicit=*/ true, + /*is_x509=*/ true, )?; Self::get_structure_size(tbs_size, tagged) @@ -611,6 +680,21 @@ impl CertWriter<'_> { Self::get_structure_size(explicit_bytes_size, tagged) } + /// Get the size of the ASN.1 KeyIdentifier structure + /// If `tagged`, include the tag and size fields + fn get_key_identifier_size( + key_identifier: &[u8], + tagged: bool, + explicit: bool, + ) -> Result { + let key_identifier_size = key_identifier.len(); + + // Determine whether to include the explicit tag wrapping in the size calculation + let explicit_bytes_size = Self::get_structure_size(key_identifier_size, explicit)?; + + Self::get_structure_size(explicit_bytes_size, tagged) + } + fn get_econtent_size( bytes: &[u8], tagged: bool, @@ -647,6 +731,7 @@ impl CertWriter<'_> { measurements, /*tagged=*/ true, /*explicit=*/ false, + /*is_x509=*/ false, )?, /*tagged=*/ true, )?; @@ -1246,20 +1331,21 @@ impl CertWriter<'_> { // Bit string is 2 bytes: // * Unused bits // * KeyUsage bits - // - // To simplify encoding, no bits are marked as unused, they are just - // set to zero. bytes_written += self.encode_size_field(2)?; - // Unused bits - bytes_written += self.encode_byte(0)?; - - let key_usage = if is_ca { - KeyUsageFlags::DIGITAL_SIGNATURE | KeyUsageFlags::KEY_CERT_SIGN + // Count trailing bits in KeyUsage byte as unused + let (key_usage, unused_bits) = if is_ca { + ( + KeyUsageFlags::DIGITAL_SIGNATURE | KeyUsageFlags::KEY_CERT_SIGN, + 2, + ) } else { - KeyUsageFlags::DIGITAL_SIGNATURE + (KeyUsageFlags::DIGITAL_SIGNATURE, 7) }; + // Unused bits + bytes_written += self.encode_byte(unused_bits)?; + bytes_written += self.encode_byte(key_usage.0)?; Ok(bytes_written) @@ -1315,19 +1401,111 @@ impl CertWriter<'_> { Ok(bytes_written) } + /// AuthorityKeyIdentifier ::= SEQUENCE { + /// keyIdentifier [0] KeyIdentifier OPTIONAL, + /// authorityCertIssuer [1] GeneralNames OPTIONAL, + /// authorityCertSerialNumber [2] CertificateSerialNumber OPTIONAL + /// } + fn encode_authority_key_identifier_extension( + &mut self, + measurements: &MeasurementData, + is_x509: bool, + ) -> Result { + if !measurements.is_ca || !is_x509 { + return Ok(0); + } + + let aki_extension_size = Self::get_authority_key_identifier_extension_size( + measurements, + /*tagged=*/ false, + is_x509, + )?; + + // Encode Extension + let mut bytes_written = self.encode_byte(Self::SEQUENCE_TAG)?; + bytes_written += self.encode_size_field(aki_extension_size)?; + bytes_written += self.encode_oid(Self::AUTHORITY_KEY_IDENTIFIER_OID)?; + + bytes_written += self.encode_byte(Self::BOOL_TAG)?; + bytes_written += self.encode_size_field(Self::BOOL_SIZE)?; + // authority key identifier extension must NOT be marked critical + bytes_written += self.encode_byte(0x00)?; + + // Extension data is sequence -> octet string. To compute size, wrap + // in tagging once. + let key_identifier_size = Self::get_key_identifier_size( + &measurements.key_identifier, + /*tagged=*/ true, + /*explicit=*/ true, + )?; + bytes_written += self.encode_byte(Self::OCTET_STRING_TAG)?; + bytes_written += self.encode_size_field(Self::get_structure_size( + key_identifier_size, + /*tagged=*/ true, + )?)?; + + // Encode extension data sequence + bytes_written += self.encode_byte(Self::SEQUENCE_TAG)?; + bytes_written += self.encode_size_field(key_identifier_size)?; + bytes_written += self.encode_key_identifier(&measurements.key_identifier)?; + + Ok(bytes_written) + } + + fn encode_subject_key_identifier_extension( + &mut self, + measurements: &MeasurementData, + is_x509: bool, + ) -> Result { + if !measurements.is_ca || !is_x509 { + return Ok(0); + } + let ski_extension_size = Self::get_subject_key_identifier_extension_size( + measurements, + /*tagged=*/ false, + is_x509, + )?; + + // Encode Extension + let mut bytes_written = self.encode_byte(Self::SEQUENCE_TAG)?; + bytes_written += self.encode_size_field(ski_extension_size)?; + bytes_written += self.encode_oid(Self::SUBJECT_KEY_IDENTIFIER_OID)?; + + bytes_written += self.encode_byte(Self::BOOL_TAG)?; + bytes_written += self.encode_size_field(Self::BOOL_SIZE)?; + // subject key identifier extension must NOT be marked critical + bytes_written += self.encode_byte(0x00)?; + + // Extension data is sequence -> octet string. To compute size, wrap + // in tagging once. + bytes_written += self.encode_byte(Self::OCTET_STRING_TAG)?; + bytes_written += self.encode_size_field(Self::get_structure_size( + measurements.key_identifier.len(), + /*tagged=*/ true, + )?)?; + + // SubjectKeyIdentifier := OCTET STRING + bytes_written += self.encode_byte(Self::OCTET_STRING_TAG)?; + bytes_written += self.encode_size_field(measurements.key_identifier.len())?; + bytes_written += self.encode_bytes(&measurements.key_identifier)?; + + Ok(bytes_written) + } + fn encode_extensions( &mut self, measurements: &MeasurementData, - explicit: bool, + is_x509: bool, ) -> Result { let mut bytes_written = 0; - if explicit { + if is_x509 { // Extensions is EXPLICIT field number 3 bytes_written += self.encode_byte(Self::CONTEXT_SPECIFIC | Self::CONSTRUCTED | 0x03)?; bytes_written += self.encode_size_field(Self::get_extensions_size( measurements, /*tagged=*/ true, /*explicit=*/ false, + is_x509, )?)?; } @@ -1337,6 +1515,7 @@ impl CertWriter<'_> { measurements, /*tagged=*/ false, /*explicit=*/ false, + is_x509, )?)?; bytes_written += self.encode_multi_tcb_info(measurements)?; @@ -1344,6 +1523,8 @@ impl CertWriter<'_> { bytes_written += self.encode_basic_constraints(measurements)?; bytes_written += self.encode_key_usage(measurements.is_ca)?; bytes_written += self.encode_extended_key_usage(measurements)?; + bytes_written += self.encode_subject_key_identifier_extension(measurements, is_x509)?; + bytes_written += self.encode_authority_key_identifier_extension(measurements, is_x509)?; Ok(bytes_written) } @@ -1448,10 +1629,11 @@ impl CertWriter<'_> { measurements, /*tagged=*/ true, /*explicit=*/ false, + /*is_x509=*/ false, )?)?; // extensions - bytes_written += self.encode_extensions(measurements, /*explicit=*/ false)?; + bytes_written += self.encode_extensions(measurements, /*is_x509=*/ false)?; Ok(bytes_written) } @@ -1572,6 +1754,31 @@ impl CertWriter<'_> { Ok(bytes_written) } + /// Encode a KeyIdentifier + /// + /// KeyIdentifier ::= OCTET STRING + #[allow(clippy::identity_op)] + fn encode_key_identifier(&mut self, key_identifier: &[u8]) -> Result { + // KeyIdentifier is IMPLICIT field number 0 + let mut bytes_written = self.encode_byte(Self::CONTEXT_SPECIFIC | 0x0)?; + bytes_written += self.encode_size_field(Self::get_key_identifier_size( + key_identifier, + /*tagged=*/ true, + /*explicit=*/ false, + )?)?; + + // KeyIdentifier := OCTET STRING + bytes_written += self.encode_tag_field(Self::OCTET_STRING_TAG)?; + bytes_written += self.encode_size_field(Self::get_key_identifier_size( + key_identifier, + /*tagged=*/ false, + /*explicit=*/ false, + )?)?; + bytes_written += self.encode_bytes(key_identifier)?; + + Ok(bytes_written) + } + /// Encode an eContent /// /// eContent [0] EXPLICIT OCTET STRING OPTIONAL @@ -1685,7 +1892,7 @@ impl CertWriter<'_> { bytes_written += self.encode_ecdsa_subject_pubkey_info(pubkey)?; // extensions - bytes_written += self.encode_extensions(measurements, /*explicit=*/ true)?; + bytes_written += self.encode_extensions(measurements, /*is_x509=*/ true)?; Ok(bytes_written) } @@ -1840,7 +2047,8 @@ pub(crate) mod tests { use crate::x509::{CertWriter, DirectoryString, MeasurementData, Name}; use crate::DPE_PROFILE; use crypto::{CryptoBuf, EcdsaPub, EcdsaSig}; - use platform::{ArrayVec, CertValidity}; + use openssl::hash::{Hasher, MessageDigest}; + use platform::{ArrayVec, CertValidity, MAX_SKI_SIZE}; use std::str; use x509_parser::certificate::X509CertificateParser; use x509_parser::nom::Parser; @@ -2084,6 +2292,7 @@ pub(crate) mod tests { tci_nodes: &[node], is_ca: false, supports_recursive: true, + key_identifier: [0u8; MAX_SKI_SIZE], }; let mut not_before = ArrayVec::new(); @@ -2153,11 +2362,19 @@ pub(crate) mod tests { let node = TciNodeData::new(); + let mut hasher = Hasher::new(MessageDigest::sha256()).unwrap(); + hasher.update(&[0x04]).unwrap(); + hasher.update(test_pub.x.bytes()).unwrap(); + hasher.update(test_pub.y.bytes()).unwrap(); + let mut key_identifier = [0u8; MAX_SKI_SIZE]; + let digest = &hasher.finish().unwrap(); + key_identifier.copy_from_slice(&digest[..MAX_SKI_SIZE]); let measurements = MeasurementData { label: &[0; DPE_PROFILE.get_hash_size()], tci_nodes: &[node], is_ca, supports_recursive: true, + key_identifier, }; let mut not_before = ArrayVec::new(); @@ -2251,6 +2468,20 @@ pub(crate) mod tests { Ok(None) => panic!("extended key usage extension not found"), Err(_) => panic!("multiple extended key usage extensions found"), }; + + match cert.get_extension_unique(&oid!(2.5.29 .14)) { + Ok(Some(_)) => panic!("subject key identifier extensions found for non CA certificate"), + Err(_) => panic!("multiple subject key identifier extensions found"), + _ => (), + } + + match cert.get_extension_unique(&oid!(2.5.29 .35)) { + Ok(Some(_)) => { + panic!("authority key identifier extensions found for non CA certificate") + } + Err(_) => panic!("multiple authority key identifier extensions found"), + _ => (), + } } #[test] @@ -2287,5 +2518,45 @@ pub(crate) mod tests { Ok(None) => panic!("extended key usage extension not found"), Err(_) => panic!("multiple extended key usage extensions found"), }; + + let pub_key = &cert.tbs_certificate.subject_pki.subject_public_key.data; + let mut hasher = Hasher::new(MessageDigest::sha256()).unwrap(); + hasher.update(pub_key).unwrap(); + let expected_key_identifier: &[u8] = &hasher.finish().unwrap(); + + match cert.get_extension_unique(&oid!(2.5.29 .14)) { + Ok(Some(subject_key_identifier_ext)) => { + assert!(!subject_key_identifier_ext.critical); + if let ParsedExtension::SubjectKeyIdentifier(key_identifier) = + subject_key_identifier_ext.parsed_extension() + { + assert_eq!(key_identifier.0, &expected_key_identifier[..MAX_SKI_SIZE]); + } else { + panic!("Extension has wrong type"); + } + } + Ok(None) => panic!("subject key identifier extension not found"), + Err(_) => panic!("multiple subject key identifier extensions found"), + } + + match cert.get_extension_unique(&oid!(2.5.29 .35)) { + Ok(Some(extension)) => { + assert!(!extension.critical); + if let ParsedExtension::AuthorityKeyIdentifier(aki) = extension.parsed_extension() { + let key_identifier = aki.key_identifier.clone().unwrap(); + // skip first two bytes - first byte is 0x04 der encoding byte and second byte is size byte + assert_eq!( + &key_identifier.0[2..], + &expected_key_identifier[..MAX_SKI_SIZE] + ); + assert!(aki.authority_cert_issuer.is_none()); + assert!(aki.authority_cert_serial.is_none()); + } else { + panic!("Extension has wrong type"); + } + } + Ok(None) => panic!("subject key identifier extension not found"), + Err(_) => panic!("multiple subject key identifier extensions found"), + } } } diff --git a/verification/testing/certifyKey.go b/verification/testing/certifyKey.go index ff8fdeac..cea3a98f 100644 --- a/verification/testing/certifyKey.go +++ b/verification/testing/certifyKey.go @@ -6,12 +6,14 @@ import ( "bytes" "crypto/ecdsa" "crypto/elliptic" + "crypto/sha256" "crypto/x509" "crypto/x509/pkix" "encoding/asn1" "encoding/binary" "encoding/pem" "fmt" + "hash" "math/big" "reflect" "testing" @@ -141,7 +143,7 @@ func TestCertifyKeyCsr(d client.TestDPEInstance, c client.DPEClient, t *testing. } digestLen := profile.GetDigestSize() - flags := client.CertifyKeyFlags(0) + flags := client.CertifyKeyFlags(client.CertifyAddIsCA) label := make([]byte, digestLen) // Get DPE leaf certificate from CertifyKey @@ -194,7 +196,7 @@ func TestCertifyKeyCsr(d client.TestDPEInstance, c client.DPEClient, t *testing. } // Check fields and extensions in the CSR - checkCertifyKeyExtensions(t, csr.Extensions, flags, label) + checkCertifyKeyExtensions(t, csr.Extensions, flags, label, csr.PublicKey, false) checkPubKey(t, profile, csr.PublicKey, *certifyKeyResp) // Check that CSR is self-signed @@ -330,7 +332,7 @@ func checkCertifyKeyExtendedKeyUsages(t *testing.T, extensions []pkix.Extension, // Checks for KeyUsage Extension as per spec // If IsCA = true, KeyUsage extension MUST contain DigitalSignature and KeyCertSign // If IsCA = false, KeyUsage extension MUST contain only DigitalSignature -func checkCertifyKeyExtensions(t *testing.T, extensions []pkix.Extension, flags client.CertifyKeyFlags, label []byte) { +func checkCertifyKeyExtensions(t *testing.T, extensions []pkix.Extension, flags client.CertifyKeyFlags, label []byte, pubkey any, IsX509 bool) { t.Helper() bc, err := getBasicConstraints(extensions) @@ -341,6 +343,10 @@ func checkCertifyKeyExtensions(t *testing.T, extensions []pkix.Extension, flags checkCertifyKeyBasicConstraints(t, extensions, flags) checkCertifyKeyExtendedKeyUsages(t, extensions, bc.IsCA) checkCertifyKeyTcgUeidExtension(t, extensions, label) + if IsX509 { + checkCertifyKeySubjectKeyIdentifierExtension(t, extensions, bc.IsCA, pubkey) + checkCertifyKeyAuthorityKeyIdentifierExtension(t, extensions, bc.IsCA, pubkey) + } // Check MultiTcbInfo Extension structure _, err = getMultiTcbInfo(extensions) @@ -370,6 +376,71 @@ func checkCertifyKeyExtensions(t *testing.T, extensions []pkix.Extension, flags } +// Validates SubjectKeyIdentifier in certificate returned by CertifyKey command +// against the isCa flag set in the BasicConstraints extension +// The SubjectKeyIdentifier extension MUST be included if isCA is true +// and MUST be excluded if isCA is false. +func checkCertifyKeySubjectKeyIdentifierExtension(t *testing.T, extensions []pkix.Extension, ca bool, pubkey any) { + t.Helper() + + ski, err := getSubjectKeyIdentifier(extensions) + if err != nil { + t.Errorf("[ERROR]: Failed to retrieve SubjectKeyIdentifier extension: %v", err) + } + if ca { + if ski == nil { + t.Errorf("[ERROR]: The certificate is a CA but the SubjectKeyIdentifier extension is not present.") + } + var hasher hash.Hash = sha256.New() + ecdsaPub, ok := pubkey.(*ecdsa.PublicKey) + if !ok { + t.Fatal("[FATAL]: Public key is not a ecdsa key") + } + hasher.Write([]byte{0x04}) + hasher.Write(ecdsaPub.X.Bytes()) + hasher.Write(ecdsaPub.Y.Bytes()) + expectedKeyIdentifier := hasher.Sum(nil)[:20] + if !reflect.DeepEqual(ski, expectedKeyIdentifier) { + t.Errorf("[ERROR]: The value of the key identifier %v is not equal to the hash of the public key %v", ski, expectedKeyIdentifier) + } + } else if !ca && ski != nil { + t.Errorf("[ERROR]: The certificate is not a CA but the SubjectKeyIdentifier extension is present.") + } +} + +// Validates AuthorityKeyIdentifier in certificate returned by CertifyKey command +// against the isCa flag set in the BasicConstraints extension +// The AuthorityKeyIdentifier extension MUST be included if isCA is true +// and MUST be excluded if isCA is false. +func checkCertifyKeyAuthorityKeyIdentifierExtension(t *testing.T, extensions []pkix.Extension, ca bool, pubkey any) { + t.Helper() + + aki, err := getAuthorityKeyIdentifier(extensions) + if err != nil { + t.Errorf("[ERROR]: Failed to retrieve AuthorityKeyIdentifier extension: %v", err) + } + if ca { + if aki.KeyIdentifier == nil { + t.Fatal("[ERROR]: The certificate is a CA but the AuthorityKeyIdentifier extension is not present.") + } + var hasher hash.Hash = sha256.New() + ecdsaPub, ok := pubkey.(*ecdsa.PublicKey) + if !ok { + t.Fatal("[FATAL]: Public key is not a ecdsa key") + } + hasher.Write([]byte{0x04}) + hasher.Write(ecdsaPub.X.Bytes()) + hasher.Write(ecdsaPub.Y.Bytes()) + expectedKeyIdentifier := hasher.Sum(nil)[:20] + // skip first two bytes - first byte is 0x04 der encoding byte and second byte is size byte + if !reflect.DeepEqual(aki.KeyIdentifier[2:], expectedKeyIdentifier) { + t.Errorf("[ERROR]: The value of the key identifier %v is not equal to the hash of the public key %v", aki, expectedKeyIdentifier) + } + } else if !ca && aki.KeyIdentifier != nil { + t.Errorf("[ERROR]: The certificate is not a CA but the AuthorityKeyIdentifier extension is present.") + } +} + // Validates basic constraints in certificate returned by CertifyKey command // against the flag set for input parameter. // The BasicConstraints extension MUST be included @@ -427,6 +498,10 @@ func checkCertificateStructure(t *testing.T, certBytes []byte) *x509.Certificate // strictly worse and mixing the two formats does not lend itself well // to fixed-sized X.509 templating. "e_wrong_time_format_pre2050", + // Certs in the Caliptra cert chain fail this lint currently. + // We will need to truncate the serial numbers for those certs and + // then enable this lint. + "e_subject_dn_serial_number_max_length", }, }) if err != nil { @@ -450,8 +525,6 @@ func checkCertificateStructure(t *testing.T, certBytes []byte) *x509.Certificate details = fmt.Sprintf("%s. ", details) } l := registry.ByName(id) - // TODO(https://github.com/chipsalliance/caliptra-dpe/issues/74): - // Fail the test with Errorf here once we expect it to pass. t.Logf("[LINT %s] %s: %s%s (%s)", level, l.Source, details, l.Description, l.Citation) failed = true } @@ -463,6 +536,7 @@ func checkCertificateStructure(t *testing.T, certBytes []byte) *x509.Certificate Type: "CERTIFICATE", Bytes: certBytes, }))) + t.Fatalf("Certificate lint failed!") } return x509Cert } @@ -487,8 +561,8 @@ func testCertifyKey(d client.TestDPEInstance, c client.DPEClient, t *testing.T, } certifyKeyParams := []CertifyKeyParams{ - {Label: make([]byte, digestLen), Flags: client.CertifyKeyFlags(0)}, - {Label: seqLabel, Flags: client.CertifyKeyFlags(0)}, + {Label: make([]byte, digestLen), Flags: client.CertifyKeyFlags(client.CertifyAddIsCA)}, + {Label: seqLabel, Flags: client.CertifyKeyFlags(client.CertifyAddIsCA)}, } for _, params := range certifyKeyParams { @@ -517,7 +591,7 @@ func testCertifyKey(d client.TestDPEInstance, c client.DPEClient, t *testing.T, checkPubKey(t, profile, leafCert.PublicKey, *certifyKeyResp) // Check all extensions - checkCertifyKeyExtensions(t, leafCert.Extensions, params.Flags, params.Label) + checkCertifyKeyExtensions(t, leafCert.Extensions, params.Flags, params.Label, leafCert.PublicKey, true) // Ensure full certificate chain has valid signatures // This also checks certificate lifetime, signatures as part of cert chain validation diff --git a/verification/testing/certs.go b/verification/testing/certs.go index 4024c66e..4f72b17f 100644 --- a/verification/testing/certs.go +++ b/verification/testing/certs.go @@ -13,6 +13,7 @@ import ( // This file is used to test the certify key command. var ( + OidExtensionSubjectKeyIdentifier = asn1.ObjectIdentifier{2, 5, 29, 14} OidExtensionKeyUsage = asn1.ObjectIdentifier{2, 5, 29, 15} OidExtensionAuthorityKeyIdentifier = asn1.ObjectIdentifier{2, 5, 29, 35} OidExtensionBasicConstraints = asn1.ObjectIdentifier{2, 5, 29, 19} @@ -36,6 +37,12 @@ type BasicConstraints struct { PathLenConstraint int `asn1:"optional"` } +type SubjectKeyIdentifier = []byte + +type AuthorityKeyIdentifier struct { + KeyIdentifier []byte `asn1:"optional,tag:0"` +} + // A tcg-dice-MultiTcbInfo extension. // This extension SHOULD be marked as critical. func getMultiTcbInfo(extensions []pkix.Extension) (TcgMultiTcbInfo, error) { @@ -72,6 +79,40 @@ func getBasicConstraints(extensions []pkix.Extension) (BasicConstraints, error) return bc, nil } +func getSubjectKeyIdentifier(extensions []pkix.Extension) (SubjectKeyIdentifier, error) { + var ski SubjectKeyIdentifier + for _, ext := range extensions { + if ext.Id.Equal(OidExtensionSubjectKeyIdentifier) { + if ext.Critical { + return ski, fmt.Errorf("[ERROR]: SubjectKeyIdentifier extension is marked as CRITICAL") + } + _, err := asn1.Unmarshal(ext.Value, &ski) + if err != nil { + return ski, fmt.Errorf("[ERROR]: Failed to unmarshal SubjectKeyIdentifier extension: %v", err) + } + break + } + } + return ski, nil +} + +func getAuthorityKeyIdentifier(extensions []pkix.Extension) (AuthorityKeyIdentifier, error) { + var aki AuthorityKeyIdentifier + for _, ext := range extensions { + if ext.Id.Equal(OidExtensionAuthorityKeyIdentifier) { + if ext.Critical { + return aki, fmt.Errorf("[ERROR]: AuthorityKeyIdentifier extension is marked as CRITICAL") + } + _, err := asn1.Unmarshal(ext.Value, &aki) + if err != nil { + return aki, fmt.Errorf("[ERROR]: Failed to unmarshal AuthorityKeyIdentifier extension: %v", err) + } + break + } + } + return aki, nil +} + func getUeid(extensions []pkix.Extension) (TcgUeidExtension, error) { var ueid TcgUeidExtension for _, ext := range extensions { diff --git a/verification/testing/getCertificateChain.go b/verification/testing/getCertificateChain.go index 3b76c949..2c333f05 100644 --- a/verification/testing/getCertificateChain.go +++ b/verification/testing/getCertificateChain.go @@ -58,6 +58,10 @@ func checkCertificateChain(t *testing.T, certData []byte) []*x509.Certificate { // strictly worse and mixing the two formats does not lend itself well // to fixed-sized X.509 templating. "e_wrong_time_format_pre2050", + // Certs in the Caliptra cert chain fail this lint currently. + // We will need to truncate the serial numbers for those certs and + // then enable this lint. + "e_subject_dn_serial_number_max_length", }, }) if err != nil { @@ -94,6 +98,7 @@ func checkCertificateChain(t *testing.T, certData []byte) []*x509.Certificate { Type: "CERTIFICATE", Bytes: cert.Raw, }))) + t.Fatalf("Certificate lint failed!") } }