Address reviewer comments

raoulstrackx · raoulstrackx · commit 5daff929143f · 2025-09-12T17:15:40.000+02:00
diff --git a/intel-sgx/dcap-artifact-retrieval/src/provisioning_client/mod.rs b/intel-sgx/dcap-artifact-retrieval/src/provisioning_client/mod.rs
@@ -16,7 +16,7 @@ use lru_cache::LruCache;
 use num_enum::TryFromPrimitive;
 use pcs::{
     CpuSvn, DcapArtifactIssuer, EncPpid, Fmspc, PceId, PceIsvsvn, PckCert, PckCerts, PckCrl, PckID, QeId,
-    QeIdentitySigned, TcbInfo, RawTcbEvaluationDataNumbers, Unverified,
+    QeIdentitySigned, TcbComponent, TcbInfo, RawTcbEvaluationDataNumbers, Unverified,
 };
 #[cfg(feature = "reqwest")]
 use reqwest::blocking::{Client as ReqwestClient, Response as ReqwestResponse};
@@ -583,66 +583,65 @@ pub trait ProvisioningClient {
     /// 
     /// Note that PCK certs for some TCB levels may be missing.
     fn pckcerts_with_fallback(&self, pck_id: &PckID) -> Result<PckCerts, Error> {
+        let get_and_collect = |collection: &mut BTreeMap<([u8; 16], u16), PckCert<Unverified>>, cpu_svn: &[u8; 16], pce_svn: u16| -> Result<PckCert<Unverified>, Error> {
+            let pck_cert = self.pckcert(
+                Some(&pck_id.enc_ppid),
+                &pck_id.pce_id,
+                cpu_svn,
+                pce_svn,
+                Some(&pck_id.qe_id),
+            )?;
+
+            // Getting PCK cert using CPUSVN from PCKID
+            let ptcb = pck_cert.platform_tcb()?;
+            collection.insert((ptcb.cpusvn, ptcb.tcb_components.pce_svn()), pck_cert.clone());
+            Ok(pck_cert)
+        };
+
         match self.pckcerts(&pck_id.enc_ppid, pck_id.pce_id) {
             Ok(pck_certs) => return Ok(pck_certs),
             Err(Error::RequestNotSupported) => {} // fallback below
             Err(e) => return Err(e),
         }
         // fallback:
 
-        // NOTE: at least with PCCS, any call to `pckcert()` will return the
-        // "best available" PCK cert for the specified TCB level.
-        let pck_cert = self.pckcert(
-            Some(&pck_id.enc_ppid),
-            &pck_id.pce_id,
-            &pck_id.cpu_svn,
-            pck_id.pce_isvsvn,
-            Some(&pck_id.qe_id),
-        )?;
         // Use BTreeMap to have an ordered PckCerts at the end
         let mut pckcerts_map = BTreeMap::new();
-        // Getting PCK cert using CPUSVN from PCKID
-        {
-            let ptcb = pck_cert.platform_tcb()?;
-            pckcerts_map.insert((ptcb.cpusvn, ptcb.tcb_components.pce_svn()), pck_cert.clone());
-        }
-        // Getting PCK cert using CPUSVN all 1's
-        {
-            if let Ok(pck_cert) = self.pckcert(
-                Some(&pck_id.enc_ppid),
-                &pck_id.pce_id,
-                &[u8::MAX; 16],
-                pck_id.pce_isvsvn,
-                Some(&pck_id.qe_id),
-            ){
-                let ptcb = pck_cert.platform_tcb()?;
-                pckcerts_map.insert((ptcb.cpusvn, ptcb.tcb_components.pce_svn()), pck_cert);
-            }
-        }
+
+        // 1. Use PCK ID to get best available PCK Cert
+        let pck_cert = get_and_collect(&mut pckcerts_map, &pck_id.cpu_svn, pck_id.pce_isvsvn)?;
+
+        // 2. Getting PCK cert using CPUSVN all 1's
+        let _ign_err = get_and_collect(&mut pckcerts_map, &[u8::MAX; 16], pck_id.pce_isvsvn);
+
         let fmspc = pck_cert.sgx_extension()?.fmspc;
         let tcb_info = self.tcbinfo(&fmspc, None)?;
         let tcb_data = tcb_info.data()?;
-        // For every CPUSVN we also try where the late microcode value is higher
-        // than the early microcode value, the CPUSVN where the early
-        // microcode value is set to the late microcode value
-        for (cpu_svn, pce_isvsvn) in tcb_data.iter_tcb_components()
-            .chain(tcb_data.iter_tcb_components_with_late_tcb_override_only())
-        {
-            let p = match self.pckcert(
-                Some(&pck_id.enc_ppid),
-                &pck_id.pce_id,
-                &cpu_svn,
-                pce_isvsvn,
-                Some(&pck_id.qe_id),
-            ) {
-                Ok(cert) => cert,
-                Err(Error::PCSError(StatusCode::NotFound, _)) |
-                Err(Error::PCSError(StatusCode::NonStandard462, _)) => continue,
-                Err(other) => return Err(other)
-            };
-            let ptcb = p.platform_tcb()?;
-            pckcerts_map.insert((ptcb.cpusvn, ptcb.tcb_components.pce_svn()), p);
+        for (cpu_svn, pce_isvsvn) in tcb_data.iter_tcb_components() {
+            // 3. Get PCK based on TCB levels
+            let _ = get_and_collect(&mut pckcerts_map, &cpu_svn, pce_isvsvn)?;
+
+            // 4. If late loaded microcode version is higher than early loaded microcode,
+            //    also try with highest microcode version of both components. We found cases where
+            //    fetching the PCK Cert that exactly matched the TCB level, did not result in a PCK
+            //    Cert for that level
+            const EARLY_UCODE_IDX: usize = 0;
+            const LATE_UCODE_IDX: usize = 1;
+            // Unfortunately the TCB Info does not populate the component type (e.g., curl -v -X GET
+            // "https://api.trustedservices.intel.com/sgx/certification/v4/tcb?fmspc=00906ED50000&tcbEvaluationDataNumber=20"
+            // ). We pick a default as backup, and ensure errors fetching these PckCerts are
+            // ignored.
+            let eary_ucode_idx = tcb_data.tcb_component_index(TcbComponent::EarlyMicrocodeUpdate).unwrap_or(EARLY_UCODE_IDX);
+            let late_ucode_idx = tcb_data.tcb_component_index(TcbComponent::LateMicrocodeUpdate).unwrap_or(LATE_UCODE_IDX);
+            let early_ucode = cpu_svn[eary_ucode_idx];
+            let late_ucode = cpu_svn[late_ucode_idx];
+            if early_ucode < late_ucode {
+                let mut cpu_svn = cpu_svn.clone();
+                cpu_svn[eary_ucode_idx] = late_ucode;
+                let _ign_err = get_and_collect(&mut pckcerts_map, &cpu_svn, pce_isvsvn);
+            }
         }
+
         // BTreeMap by default is Ascending
         let pck_certs: Vec<_> = pckcerts_map.into_iter().rev().map(|(_, v)| v).collect();
         pck_certs
diff --git a/intel-sgx/pcs/src/lib.rs b/intel-sgx/pcs/src/lib.rs
@@ -29,7 +29,7 @@ use {
 };
 
 pub use crate::pckcrl::PckCrl;
-pub use crate::pckcrt::{PckCert, PckCerts, SGXPCKCertificateExtension, SGXType};
+pub use crate::pckcrt::{PckCert, PckCerts, SGXPCKCertificateExtension, SGXType, TcbComponent};
 pub use crate::qe_identity::{EnclaveIdentity, QeIdentity, QeIdentitySigned};
 pub use crate::tcb_info::{AdvisoryID, Fmspc, TcbInfo, TcbData, TcbLevel};
 pub use crate::tcb_evaluation_data_numbers::{RawTcbEvaluationDataNumbers, TcbEvalNumber, TcbEvaluationDataNumbers, TcbPolicy};
diff --git a/intel-sgx/pcs/src/pckcrt.rs b/intel-sgx/pcs/src/pckcrt.rs
@@ -127,6 +127,26 @@ enum IntelSgxTcbComponents {
 #[serde(from = "IntelSgxTcbComponents")]
 pub struct TcbComponents(IntelSgxTcbComponentsV4);
 
+#[derive(PartialEq, Eq, Debug, Clone, Copy)]
+pub enum TcbComponent {
+    EarlyMicrocodeUpdate,
+    LateMicrocodeUpdate,
+}
+
+impl TryFrom<&str> for TcbComponent {
+    type Error = ();
+
+    fn try_from(s: &str) -> Result<Self, Self::Error> {
+        if s == "Early Microcode Update" {
+            Ok(TcbComponent::EarlyMicrocodeUpdate)
+        } else if s == "SGX Late Microcode Update" {
+            Ok(TcbComponent::LateMicrocodeUpdate)
+        } else {
+            Err(())
+        }
+    }
+}
+
 impl TcbComponents {
     pub fn from_raw(raw_cpusvn: [u8; 16], pcesvn: u16) -> Self {
         TcbComponents(IntelSgxTcbComponentsV4 {
@@ -159,48 +179,11 @@ impl TcbComponents {
         out
     }
 
-    /// Returns the CPUSVN with the "Early Microcode Update" SVN overridden by the
-    /// "SGX Late Microcode Update" SVN if the latter is higher.
-    /// We assume there is only at most one component of either type.
-    /// If there are multiple components of either type, only the first occurrence is used.
-    /// 
-    /// For example:
-    /// ```json
-    /// [
-    ///   ...
-    ///   {
-    ///     "svn": 7,
-    ///     "category": "BIOS",
-    ///     "type": "Early Microcode Update"
-    ///   },
-    ///   ...
-    ///   {
-    ///     "svn": 9,
-    ///     "category": "OS/VMM",
-    ///     "type": "SGX Late Microcode Update"
-    ///   },
-    ///   ...
-    /// ]
-    /// ```
-    /// Here 7 will be set to 9. Note: order is random.
-    pub fn cpu_svn_with_late_override_early(&self) -> CpuSvn {
-        let mut out: CpuSvn = self.cpu_svn();
-        let tcb_components = &self.0.sgxtcbcomponents;
-
-        // Find the first index of each relevant component type.
-        let early_idx = tcb_components
+    /// Returns the index of the TCB component
+    pub fn tcb_component_index(&self, comp: TcbComponent) -> Option<usize> {
+        self.0.sgxtcbcomponents
             .iter()
-            .position(|comp| comp.comp_type == "Early Microcode Update");
-        let late_idx = tcb_components
-            .iter()
-            .position(|comp| comp.comp_type == "SGX Late Microcode Update");
-
-        if let (Some(early), Some(late)) = (early_idx, late_idx) {
-            if out[early] < out[late] {
-                out[early] = out[late];
-            }
-        }
-        out
+            .position(|c| TcbComponent::try_from(c.comp_type.as_str()) == Ok(comp))
     }
 }
 
@@ -857,7 +840,7 @@ mod tests {
 
     use super::*;
     #[cfg(not(target_env = "sgx"))]
-    use crate::{TcbInfo, get_cert_subject, get_cert_subject_from_der};
+    use crate::{get_cert_subject, get_cert_subject_from_der};
 
     fn decode_tcb_item<'a, 'b>(reader: &mut BERReaderSeq<'a, 'b>) -> ASN1Result<(ObjectIdentifier, u8)> {
         let oid = reader.next().read_oid()?;
diff --git a/intel-sgx/pcs/src/tcb_info.rs b/intel-sgx/pcs/src/tcb_info.rs
@@ -18,7 +18,7 @@ use {
     pkix::pem::PEM_CERTIFICATE, pkix::x509::GenericCertificate, pkix::FromBer, std::ops::Deref,
 };
 
-use crate::pckcrt::TcbComponents;
+use crate::pckcrt::{TcbComponent, TcbComponents};
 use crate::{io, CpuSvn, Error, PceIsvsvn, Platform, TcbStatus, Unverified, VerificationType, Verified};
 
 #[derive(Debug, Clone, Hash, PartialEq, Eq)]
@@ -301,6 +301,16 @@ impl TcbData<Unverified> {
         }
         Ok(data)
     }
+
+    /// Returns the index of the TCB component
+    pub fn tcb_component_index(&self, comp: TcbComponent) -> Option<usize> {
+        if let Some(c) = self.tcb_levels.first() {
+            c.components().tcb_component_index(comp)
+        } else {
+            None
+        }
+    }
+
 }
 
 impl<V: VerificationType> TcbData<V> {
@@ -326,21 +336,6 @@ impl<V: VerificationType> TcbData<V> {
     pub fn iter_tcb_components(&self) -> impl Iterator<Item = (CpuSvn, PceIsvsvn)> + '_ {
         self.tcb_levels.iter().map(|tcb_level| (tcb_level.tcb.cpu_svn(), tcb_level.tcb.pce_svn()))
     }
-
-    /// For every CPUSVN where the late microcode value is higher
-    /// than the early microcode value, the CPUSVN where the early
-    /// microcode value is set to the late microcode value
-    pub fn iter_tcb_components_with_late_tcb_override_only(&self) -> impl Iterator<Item = (CpuSvn, PceIsvsvn)> + '_ {
-        self.tcb_levels.iter().filter_map(|tcb_level| {
-            let overridden_svn = tcb_level.tcb.cpu_svn_with_late_override_early();
-            let cpu_svn = tcb_level.tcb.cpu_svn();
-            if cpu_svn != overridden_svn {
-                Some((overridden_svn, tcb_level.tcb.pce_svn()))
-            } else {
-                None
-            }
-        })
-    }
 }
 
 #[derive(Clone, Serialize, Deserialize, Debug, Eq, PartialEq)]
