Repository: chipsalliance/caliptra-sw (rust)

Issue:
MB Command Parameter Checking for Run-Time
All Input Data must have parameter checking.  This mostly affects our MB implementation.

For example, if a size value is passed in with the MB command, we should verify that size value against max values and other bounds (like message ptr increments) to ensure that attackers can't attack via MB input.

Pull request description:
Add mailbox command parameter validity checking
For invoke DPE, we check that data_size <= MAX_DATA_SIZE.

Fixes #787 

Patch:
diff --git a/runtime/src/invoke_dpe.rs b/runtime/src/invoke_dpe.rs
index 7874e231..3f3d3baa 100644
--- a/runtime/src/invoke_dpe.rs
+++ b/runtime/src/invoke_dpe.rs
@@ -16,6 +16,11 @@ impl InvokeDpeCmd {
 
     pub(crate) fn execute(drivers: &mut Drivers, cmd_args: &[u8]) -> CaliptraResult<MailboxResp> {
         if let Some(cmd) = InvokeDpeReq::read_from(cmd_args) {
+            // Validate data length
+            if cmd.data_size as usize > cmd.data.len() {
+                return Err(CaliptraError::RUNTIME_MAILBOX_INVALID_PARAMS);
+            }
+
             let pdata = drivers.persistent_data.get();
             let rt_pub_key = pdata.fht.rt_dice_pub_key;
             let mut crypto = DpeCrypto::new(
@@ -37,7 +42,7 @@ impl InvokeDpeCmd {
             };
 
             let locality = drivers.mbox.user();
-            let command = Command::deserialize(&cmd.data)
+            let command = Command::deserialize(&cmd.data[..cmd.data_size as usize])
                 .map_err(|_| CaliptraError::RUNTIME_INVOKE_DPE_FAILED)?;
             let flags = pdata.manifest1.header.flags;
             let mut dpe = &mut drivers.persistent_data.get_mut().dpe;
diff --git a/runtime/src/verify.rs b/runtime/src/verify.rs
index 2dda7613..c03c082c 100644
--- a/runtime/src/verify.rs
+++ b/runtime/src/verify.rs
@@ -62,10 +62,10 @@ impl HmacVerifyCmd {
             let key = Hmac384Key::from(&key);
             let mut out_tag = Array4x12::default();
             let Ok(len) = usize::try_from(cmd.len) else {
-                return Err(CaliptraError::RUNTIME_HMAC_VERIFY_FAILED);
+                return Err(CaliptraError::RUNTIME_MAILBOX_INVALID_PARAMS);
             };
             if len > cmd.msg.len() {
-                return Err(CaliptraError::RUNTIME_HMAC_VERIFY_FAILED);
+                return Err(CaliptraError::RUNTIME_MAILBOX_INVALID_PARAMS);
             }
             let data = Hmac384Data::from(&cmd.msg[0..len]);
             let mut trng = unsafe {


Test patch:
diff --git a/runtime/tests/integration_tests.rs b/runtime/tests/integration_tests.rs
index 01aaf4b8..183f327f 100644
--- a/runtime/tests/integration_tests.rs
+++ b/runtime/tests/integration_tests.rs
@@ -313,6 +313,37 @@ fn test_invoke_dpe_get_profile_cmd() {
     assert_eq!(profile.vendor_id, VENDOR_ID);
     assert_eq!(profile.vendor_sku, VENDOR_SKU);
     assert_eq!(profile.flags, DPE_SUPPORT.bits());
+
+    // Test with data_size too big.
+    let cmd = InvokeDpeReq {
+        data_size: InvokeDpeReq::DATA_MAX_SIZE as u32 + 1,
+        ..cmd
+    };
+
+    let checksum = caliptra_common::checksum::calc_checksum(
+        u32::from(CommandId::INVOKE_DPE),
+        &cmd.as_bytes()[4..],
+    );
+
+    let cmd = InvokeDpeReq {
+        hdr: MailboxReqHeader { chksum: checksum },
+        ..cmd
+    };
+
+    // Make sure the command execution fails.
+    let resp = model
+        .mailbox_execute(u32::from(CommandId::INVOKE_DPE), cmd.as_bytes())
+        .unwrap_err();
+    if let ModelError::MailboxCmdFailed(code) = resp {
+        assert_eq!(
+            code,
+            caliptra_drivers::CaliptraError::RUNTIME_MAILBOX_INVALID_PARAMS.into()
+        );
+    }
+    assert_eq!(
+        model.soc_ifc().cptra_fw_error_non_fatal().read(),
+        caliptra_drivers::CaliptraError::RUNTIME_MAILBOX_INVALID_PARAMS.into()
+    );
 }
 
 #[test]
@@ -336,7 +367,7 @@ fn test_invoke_dpe_get_certificate_chain_cmd() {
     let cmd = InvokeDpeReq {
         hdr: MailboxReqHeader { chksum: 0 },
         data,
-        data_size: cmd_hdr_buf.len() as u32,
+        data_size: (cmd_hdr_buf.len() + dpe_cmd_buf.len()) as u32,
     };
 
     let checksum = caliptra_common::checksum::calc_checksum(
@@ -402,7 +433,7 @@ fn test_invoke_dpe_sign_and_certify_key_cmds() {
     let sign_mbox_cmd = InvokeDpeReq {
         hdr: MailboxReqHeader { chksum: 0 },
         data,
-        data_size: sign_cmd_hdr_buf.len() as u32,
+        data_size: (sign_cmd_hdr_buf.len() + sign_cmd_buf.len()) as u32,
     };
 
     let checksum = caliptra_common::checksum::calc_checksum(
@@ -448,7 +479,7 @@ fn test_invoke_dpe_sign_and_certify_key_cmds() {
     let certify_key_mbox_cmd = InvokeDpeReq {
         hdr: MailboxReqHeader { chksum: 0 },
         data,
-        data_size: certify_key_cmd_hdr_buf.len() as u32,
+        data_size: (certify_key_cmd_hdr_buf.len() + certify_key_cmd_buf.len()) as u32,
     };
 
     let checksum = caliptra_common::checksum::calc_checksum(
@@ -808,4 +839,35 @@ fn test_idev_id_cert() {
 
     let cert = GetIdevCertResp::read_from(resp.as_slice()).unwrap();
     assert!(cmd.tbs_size < cert.cert_size);
+
+    // Test with tbs_size too big.
+    let cmd = GetIdevCertReq {
+        tbs_size: GetIdevCertReq::DATA_MAX_SIZE as u32 + 1,
+        ..cmd
+    };
+
+    let checksum = caliptra_common::checksum::calc_checksum(
+        u32::from(CommandId::GET_IDEV_CERT),
+        &cmd.as_bytes()[4..],
+    );
+
+    let cmd = GetIdevCertReq {
+        hdr: MailboxReqHeader { chksum: checksum },
+        ..cmd
+    };
+
+    // Make sure the command execution fails.
+    let resp = model
+        .mailbox_execute(u32::from(CommandId::GET_IDEV_CERT), cmd.as_bytes())
+        .unwrap_err();
+    if let ModelError::MailboxCmdFailed(code) = resp {
+        assert_eq!(
+            code,
+            caliptra_drivers::CaliptraError::RUNTIME_MAILBOX_INVALID_PARAMS.into()
+        );
+    }
+    assert_eq!(
+        model.soc_ifc().cptra_fw_error_non_fatal().read(),
+        caliptra_drivers::CaliptraError::RUNTIME_MAILBOX_INVALID_PARAMS.into()
+    );
 }