Remove panic from DPE

There are a few more places for potential panic in DPE which this PR fixes.
chipsalliance · Aug 25, 2023 · 49455b1 · 49455b1
1 parent a28b6b7
commit 49455b1
Show file tree

Hide file tree

Showing 5 changed files with 68 additions and 47 deletions.
diff --git a/crypto/src/lib.rs b/crypto/src/lib.rs
@@ -13,7 +13,6 @@ pub use signer::*;
 pub mod openssl;
 
 mod signer;
-use core::fmt::{Error, Write};
 
 #[derive(Debug, Clone, Copy)]
 #[cfg_attr(test, derive(strum_macros::EnumIter))]
@@ -103,7 +102,7 @@ pub trait Crypto {
         serial: &mut [u8],
     ) -> Result<(), CryptoError> {
         if serial.len() < algs.size() * 2 {
-            return Err(CryptoError::CryptoLibError);
+            return Err(CryptoError::Size);
         }
 
         let mut hasher = self.hash_initialize(algs)?;
@@ -112,11 +111,28 @@ pub trait Crypto {
         hasher.update(pub_key.y.bytes())?;
         let digest = hasher.finish()?;
 
-        let mut w = BufWriter {
-            buf: serial,
-            offset: 0,
-        };
-        w.write_hex_str(digest.bytes())
+        self.write_hex_str(digest.bytes(), serial)
+    }
+
+    fn write_hex_str(&mut self, src: &[u8], dest: &mut [u8]) -> Result<(), CryptoError> {
+        if dest.len() != src.len() * 2 {
+            return Err(CryptoError::Size);
+        }
+
+        let mut curr_idx = 0;
+        const HEX_CHARS: &[u8; 16] = b"0123456789abcdef";
+        for &b in src {
+            let h1 = (b >> 4) as usize;
+            let h2 = (b & 0xF) as usize;
+            if h1 >= HEX_CHARS.len() || h2 >= HEX_CHARS.len() || curr_idx + 1 >= dest.len() {
+                return Err(CryptoError::CryptoLibError);
+            }
+            dest[curr_idx] = HEX_CHARS[h1];
+            dest[curr_idx + 1] = HEX_CHARS[h2];
+            curr_idx += 2;
+        }
+
+        Ok(())
     }
 
     /// Initialize a running hash. Returns an object that will be able to complete the rest.
@@ -206,36 +222,6 @@ pub trait Crypto {
         digest: &Digest,
     ) -> Result<HmacSig, CryptoError>;
 }
-
-/// Writer for a static buffer
-struct BufWriter<'a> {
-    buf: &'a mut [u8],
-    offset: usize,
-}
-
-impl Write for BufWriter<'_> {
-    fn write_str(&mut self, s: &str) -> Result<(), Error> {
-        if s.len() > self.buf.len().saturating_sub(self.offset) {
-            return Err(Error);
-        }
-
-        self.buf[self.offset..self.offset + s.len()].copy_from_slice(s.as_bytes());
-        self.offset += s.len();
-
-        Ok(())
-    }
-}
-
-impl BufWriter<'_> {
-    fn write_hex_str(&mut self, src: &[u8]) -> Result<(), CryptoError> {
-        for &b in src {
-            write!(self, "{b:02x}").map_err(|_| CryptoError::CryptoLibError)?;
-        }
-
-        Ok(())
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

diff --git a/dpe/src/commands/certify_key.rs b/dpe/src/commands/certify_key.rs
@@ -85,7 +85,9 @@ impl CommandExecution for CertifyKeyCmd {
         const INITIALIZER: TciNodeData = TciNodeData::new();
         let mut nodes = [INITIALIZER; MAX_HANDLES];
         let tcb_count = dpe.get_tcb_nodes(idx, &mut nodes)?;
-
+        if tcb_count > MAX_HANDLES {
+            return Err(DpeErrorCode::InternalError);
+        }
         let measurements = MeasurementData {
             label: &self.label,
             tci_nodes: &nodes[..tcb_count],
@@ -106,6 +108,9 @@ impl CommandExecution for CertifyKeyCmd {
             Self::FORMAT_X509 => {
                 let mut tbs_buffer = [0u8; MAX_CERT_SIZE];
                 let mut tbs_writer = X509CertWriter::new(&mut tbs_buffer, true);
+                if issuer_len > MAX_CHUNK_SIZE {
+                    return Err(DpeErrorCode::InternalError);
+                }
                 let mut bytes_written = tbs_writer.encode_ecdsa_tbs(
                     /*serial=*/
                     &subject_name.serial[..20], // Serial number must be truncated to 20 bytes
@@ -114,6 +119,9 @@ impl CommandExecution for CertifyKeyCmd {
                     &pub_key,
                     &measurements,
                 )?;
+                if bytes_written > MAX_CERT_SIZE {
+                    return Err(DpeErrorCode::InternalError);
+                }
 
                 let tbs_digest = env
                     .crypto
@@ -143,8 +151,16 @@ impl CommandExecution for CertifyKeyCmd {
 
         Ok(Response::CertifyKey(CertifyKeyResp {
             new_context_handle: dpe.contexts[idx].handle,
-            derived_pubkey_x: pub_key.x.bytes().try_into().unwrap(),
-            derived_pubkey_y: pub_key.y.bytes().try_into().unwrap(),
+            derived_pubkey_x: pub_key
+                .x
+                .bytes()
+                .try_into()
+                .map_err(|_| DpeErrorCode::InternalError)?,
+            derived_pubkey_y: pub_key
+                .y
+                .bytes()
+                .try_into()
+                .map_err(|_| DpeErrorCode::InternalError)?,
             cert_size,
             cert,
             resp_hdr: ResponseHdr::new(DpeErrorCode::NoError),

diff --git a/dpe/src/commands/sign.rs b/dpe/src/commands/sign.rs
@@ -104,8 +104,14 @@ impl CommandExecution for SignCmd {
 
         Ok(Response::Sign(SignResp {
             new_context_handle: dpe.contexts[idx].handle,
-            sig_r_or_hmac: r.bytes().try_into().unwrap(),
-            sig_s: s.bytes().try_into().unwrap(),
+            sig_r_or_hmac: r
+                .bytes()
+                .try_into()
+                .map_err(|_| DpeErrorCode::InternalError)?,
+            sig_s: s
+                .bytes()
+                .try_into()
+                .map_err(|_| DpeErrorCode::InternalError)?,
             resp_hdr: ResponseHdr::new(DpeErrorCode::NoError),
         }))
     }

diff --git a/dpe/src/context.rs b/dpe/src/context.rs
@@ -154,7 +154,11 @@ impl TryFrom<&[u8]> for ContextHandle {
             return Err(DpeErrorCode::InternalError);
         }
 
-        Ok(ContextHandle(raw[0..Self::SIZE].try_into().unwrap()))
+        Ok(ContextHandle(
+            raw[0..Self::SIZE]
+                .try_into()
+                .map_err(|_| DpeErrorCode::InternalError)?,
+        ))
     }
 }
 

diff --git a/dpe/src/x509.rs b/dpe/src/x509.rs
@@ -417,11 +417,14 @@ impl X509CertWriter<'_> {
     fn encode_bytes(&mut self, bytes: &[u8]) -> Result<usize, DpeErrorCode> {
         let size = bytes.len();
 
-        if size > self.certificate.len().saturating_sub(self.offset) {
+        if self.offset >= self.certificate.len() || self.offset + size > self.certificate.len() {
             return Err(DpeErrorCode::InternalError);
         }
 
-        self.certificate[self.offset..self.offset + size].copy_from_slice(bytes);
+        self.certificate
+            .get_mut(self.offset..self.offset + size)
+            .ok_or(DpeErrorCode::InternalError)?
+            .copy_from_slice(bytes);
         self.offset += size;
 
         Ok(size)
@@ -476,6 +479,9 @@ impl X509CertWriter<'_> {
             bytes_written += self.encode_byte(0)?;
         }
 
+        if integer_offset >= integer.len() {
+            return Err(DpeErrorCode::InternalError);
+        }
         bytes_written += self.encode_bytes(&integer[integer_offset..])?;
 
         Ok(bytes_written)
@@ -786,9 +792,12 @@ impl X509CertWriter<'_> {
         bytes_written += self.encode_size_field(Self::BOOL_SIZE)?;
         bytes_written += self.encode_byte(crit)?;
 
-        let tcb_infos_size =
+        let tcb_infos_size = if !measurements.tci_nodes.is_empty() {
             Self::get_tcb_info_size(&measurements.tci_nodes[0], /*tagged=*/ true)?
-                * measurements.tci_nodes.len();
+                * measurements.tci_nodes.len()
+        } else {
+            0
+        };
         bytes_written += self.encode_byte(Self::OCTET_STRING_TAG)?;
         bytes_written += self.encode_size_field(Self::get_structure_size(
             tcb_infos_size,