dcap-ql: Fix length validation logic for quote signatures

intel-sgx/dcap-provider/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "dcap-provider"
-version = "0.4.0"
+version = "0.4.1"
 authors = ["Fortanix, Inc."]
 edition = "2018"
 license = "MPL-2.0"
@@ -34,7 +34,7 @@ crate-type = ["cdylib"]
 
 [dependencies]
 # Project dependencies
-"dcap-ql" = { version = "0.4.0", path = "../dcap-ql", features = ["link"] }
+"dcap-ql" = { version = "0.5.0", path = "../dcap-ql", features = ["link"] }
 "report-test" = { version = "0.4.0", path = "../report-test" }
 
 # External dependencies

intel-sgx/dcap-provider/src/lib.rs

@@ -116,13 +116,18 @@ fn parse_certdata(
         return Err(Quote3Error::NoPlatformCertData);
     }
 
-    let sig = quote
+    let (sig, trailing) = quote
         .signature::<quote::Quote3SignatureEcdsaP256>()
         .map_err(|e| {
             error!("PPID query: {}", e);
             Quote3Error::NoPlatformCertData
         })?;
 
+    if !trailing.is_empty() {
+        error!("trailing data after quote signature: {} bytes", trailing.len());
+        return Err(Quote3Error::NoPlatformCertData);
+    }
+
     let cd = sig
         .certification_data::<quote::Qe3CertDataPpid>()
         .map_err(|e| {
@@ -368,7 +373,7 @@ pub extern "C" fn sgx_ql_free_quote_config(p_cert_config: *const Config) -> Quot
             .ok_or(Quote3Error::InvalidParameter)?
             .cert_data_size;
         let buflen = cert_data_size as usize + mem::size_of::<Config>();
-        Box::from_raw(slice::from_raw_parts_mut(p_cert_config as *mut u8, buflen));
+        let _ = Box::from_raw(slice::from_raw_parts_mut(p_cert_config as *mut u8, buflen));
         Ok(())
     })() {
         Ok(()) => Quote3Error::Success,

intel-sgx/dcap-ql/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "dcap-ql"
-version = "0.4.0"
+version = "0.5.0"
 authors = ["Fortanix, Inc."]
 license = "MPL-2.0"
 description = """

intel-sgx/dcap-ql/src/quote.rs

@@ -18,7 +18,7 @@ use serde::{Deserialize, Serialize};
 #[cfg(feature = "verify")]
 use sgx_isa::Report;
 use std::borrow::Cow;
-use std::mem;
+use std::{cmp, mem};
 use anyhow::bail;
 
 // ====================================================
@@ -159,7 +159,9 @@ impl<'a> TakePrefix for Cow<'a, str> {
 }
 
 pub trait Quote3Signature<'a>: Sized {
-    fn parse(type_: Quote3AttestationKeyType, data: Cow<'a, [u8]>) -> Result<Self>;
+    /// Parse the signature encoded in data and return the parsed value along
+    /// with any trailing data that was not part of the signature.
+    fn parse(type_: Quote3AttestationKeyType, data: Cow<'a, [u8]>) -> Result<(Self, Cow<'a, [u8]>)>;
 }
 
 pub trait Qe3CertData<'a>: Sized {
@@ -231,19 +233,27 @@ fn get_ecdsa_sig_der(sig: &[u8]) -> Result<Vec<u8>> {
 }
 
 impl<'a> Quote3Signature<'a> for Quote3SignatureEcdsaP256<'a> {
-    fn parse(type_: Quote3AttestationKeyType, mut data: Cow<'a, [u8]>) -> Result<Self> {
+    fn parse(type_: Quote3AttestationKeyType, mut data: Cow<'a, [u8]>) -> Result<(Self, Cow<'a, [u8]>)> {
         if type_ != Quote3AttestationKeyType::EcdsaP256 {
             bail!("Invalid attestation key type: {:?}", type_)
         }
 
-        let sig_len = data.take_prefix(mem::size_of::<u32>()).map(|v| LE::read_u32(&v))?;
-        if sig_len as usize != data.len() {
+        let sig_len = data.take_prefix(mem::size_of::<u32>()).map(|v| LE::read_u32(&v))? as usize;
+        if sig_len > data.len() {
             bail!(
-                "Invalid signature length. Got {}, expected {}",
+                "Invalid signature length. Got {}, expected >= {}",
                 data.len(),
                 sig_len
             );
         }
+        // NOTE: data may contain trailing zeros due to `get_quote_size` and
+        // `get_quote` C APIs allowing larger than necessary buffer to be
+        // allocated to hold the quote.
+        let (mut data, trailing) = {
+            let prefix = data.take_prefix(cmp::min(data.len(), sig_len))?;
+            (prefix, data)
+        };
+
         let signature = data.take_prefix(ECDSA_P256_SIGNATURE_LEN)?;
         let attestation_public_key = data.take_prefix(ECDSA_P256_PUBLIC_KEY_LEN)?;
         let qe3_report = data.take_prefix(REPORT_BODY_LEN)?;
@@ -262,15 +272,15 @@ impl<'a> Quote3Signature<'a> for Quote3SignatureEcdsaP256<'a> {
             );
         }
 
-        Ok(Quote3SignatureEcdsaP256 {
+        Ok((Quote3SignatureEcdsaP256 {
             signature,
             attestation_public_key,
             qe3_report,
             qe3_signature,
             authentication_data,
             certification_data_type,
             certification_data: data,
-        })
+        }, trailing))
     }
 }
 
@@ -357,7 +367,9 @@ impl<'a> Quote<'a> {
         &self.report_body
     }
 
-    pub fn signature<T: Quote3Signature<'a>>(&self) -> Result<T> {
+    /// Parse the signature encoded in `self.signature` and return the parsed
+    /// value along with any trailing data that was not part of the signature.
+    pub fn signature<T: Quote3Signature<'a>>(&self) -> Result<(T, Cow<'a, [u8]>)> {
         let QuoteHeader::V3 {
             attestation_key_type,
             ..
@@ -538,11 +550,15 @@ impl<'a> Quote3SignatureVerify<'a> for Quote3SignatureEcdsaP256<'a> {
 
 impl<'a> Quote<'a> {
     #[cfg(feature = "verify")]
-    pub fn verify<T: Quote3SignatureVerify<'a>>(quote: &'a [u8], root_of_trust: T::TrustRoot) -> Result<Self> {
+    /// Parses the `Quote` encoded in `quote`, verifies the signature within the
+    /// quote, and returns the parsed quote along with any trailing data that
+    /// was not part of the signature. It's up to the caller to decide what to
+    /// do with the trailing bytes.
+    pub fn verify<T: Quote3SignatureVerify<'a>>(quote: &'a [u8], root_of_trust: T::TrustRoot) -> Result<(Self, Cow<'a, [u8]>)> {
         let parsed_quote = Self::parse(quote)?;
-        let sig = parsed_quote.signature::<T>()?;
+        let (sig, trailing) = parsed_quote.signature::<T>()?;
         Quote3SignatureVerify::verify(&sig, quote, root_of_trust)?;
-        Ok(parsed_quote)
+        Ok((parsed_quote, trailing))
     }
 }
 
@@ -641,8 +657,10 @@ mod tests {
         assert_eq!(user_data, &ud);
 
         assert_eq!(attestation_key_type, Quote3AttestationKeyType::EcdsaP256);
-        let sig = quote.signature::<Quote3SignatureEcdsaP256>().unwrap();
+        let (sig, trailing) = quote.signature::<Quote3SignatureEcdsaP256>().unwrap();
 
+        let expected_trailing: &[u8] = &[];
+        assert_eq!(&*trailing, expected_trailing);
         assert_eq!(
             sig.certification_data_type(),
             CertificationDataType::PpidEncryptedRsa3072
@@ -722,8 +740,11 @@ mod tests {
             // TODO: Update the example quote with a matching qe3_identity.json file
             qe3_identity: include_str!("../tests/corrupt_qe3_identity.json").to_string(),
         };
-        #[cfg(feature = "verify")]
-        assert!(Quote::verify::<Quote3SignatureEcdsaP256>(TEST_QUOTE, &mut verifier).is_ok())
+        #[cfg(feature = "verify")] {
+            let (_, trailing) = Quote::verify::<Quote3SignatureEcdsaP256>(TEST_QUOTE, &mut verifier).unwrap();
+            let expected_trailing: &[u8] = &[];
+            assert_eq!(&*trailing, expected_trailing);
+        }
     }
 
     #[test]
@@ -774,4 +795,64 @@ mod tests {
         #[cfg(feature = "verify")]
         assert!(Quote::verify::<Quote3SignatureEcdsaP256>(TEST_QUOTE, &mut verifier).is_err());
     }
+
+    #[test]
+    fn test_quote_with_trailing_zeros() {
+        let expected_trailing: &[u8] = &[42, 75, 92];
+        let with_trailing_data = {
+            const TEST_QUOTE: &[u8] = &*include_bytes!("../tests/quote_pck_cert_chain.bin");
+            let n = TEST_QUOTE.len();
+            let t = expected_trailing.len();
+            let mut with_trailing_data = vec![0u8; n + t];
+            with_trailing_data[..n].copy_from_slice(TEST_QUOTE);
+            with_trailing_data[n..].copy_from_slice(expected_trailing);
+            with_trailing_data
+        };
+
+        let quote = Quote::parse(&with_trailing_data).unwrap();
+        let &QuoteHeader::V3 {
+            attestation_key_type,
+            ..
+        } = quote.header();
+
+        assert_eq!(attestation_key_type, Quote3AttestationKeyType::EcdsaP256);
+        let (sig, trailing) = quote.signature::<Quote3SignatureEcdsaP256>().unwrap();
+
+        assert_eq!(&*trailing, expected_trailing);
+
+        #[cfg(feature = "verify")]
+        let mut verifier = MyVerifier {
+            // See the note in `fn test_quote_verification`.
+            // TODO: Update the example quote with a matching qe3_identity.json file
+            qe3_identity: include_str!("../tests/corrupt_qe3_identity.json").to_string(),
+        };
+        #[cfg(feature = "verify")] {
+            let (_, trailing) = Quote::verify::<Quote3SignatureEcdsaP256>(&with_trailing_data, &mut verifier).unwrap();
+            assert_eq!(&*trailing, expected_trailing);
+        }
+    }
+
+    #[test]
+    fn test_quote_too_short() {
+        let too_short = {
+            const TEST_QUOTE: &[u8] = &*include_bytes!("../tests/quote_pck_cert_chain.bin");
+            let mut too_short = vec![0u8; TEST_QUOTE.len() - 5];
+            let n = too_short.len();
+            too_short.copy_from_slice(&TEST_QUOTE[..n]);
+            too_short
+        };
+
+        // We won't detect the issue until we try to parse the signature
+        let quote = Quote::parse(&too_short).unwrap();
+        let &QuoteHeader::V3 {
+            attestation_key_type,
+            ..
+        } = quote.header();
+
+        assert_eq!(attestation_key_type, Quote3AttestationKeyType::EcdsaP256);
+        match quote.signature::<Quote3SignatureEcdsaP256>() {
+            Ok(_) => panic!("unexpected Ok result"),
+            Err(e) => assert_eq!(e.to_string(), "Invalid signature length. Got 4137, expected >= 4142"),
+        }
+    }
 }

intel-sgx/dcap-retrieve-pckid/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "dcap-retrieve-pckid"
-version = "0.2.0"
+version = "0.2.1"
 authors = ["Fortanix, Inc."]
 license = "MPL-2.0"
 edition = "2018"
@@ -15,7 +15,7 @@ categories = ["command-line-utilities"]
 [dependencies]
 # Project dependencies
 "aesm-client" = { version = "0.6.0", path = "../aesm-client", features = ["sgxs"] }
-"dcap-ql" = { version = "0.4.0", path = "../dcap-ql", default-features = false }
+"dcap-ql" = { version = "0.5.0", path = "../dcap-ql", default-features = false }
 "report-test" = { version = "0.4.0", path = "../report-test" }
 "sgx-isa" = { version = "0.4.0", path = "../sgx-isa" }
 "sgxs-loaders" = { version = "0.4.0", path = "../sgxs-loaders" }

intel-sgx/dcap-retrieve-pckid/src/lib.rs

@@ -86,9 +86,14 @@ pub fn retrieve_pckid_str() -> Result<PckId, anyhow::Error> {
 
     let quote = Quote::parse(res.quote()).map_err(|err| err.context("Error parsing quote"))?;
     let QuoteHeader::V3 { user_data, .. } = quote.header();
-    let sig = quote
+    let (sig, trailing) = quote
         .signature::<Quote3SignatureEcdsaP256>()
         .map_err(|err| err.context("Error parsing requested signature type"))?;
+
+    if !trailing.is_empty() {
+        return Err(anyhow!("trailing data after quote signature: {} bytes", trailing.len()));
+    }
+
     let cd_ppid = sig
         .certification_data::<Qe3CertDataPpid>()
         .map_err(|err| err.context("Error parsing requested signature type"))?;

intel-sgx/pcs/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "pcs"
-version = "0.3.0"
+version = "0.3.1"
 authors = ["Fortanix, Inc."]
 license = "MPL-2.0"
 edition = "2018"
@@ -19,7 +19,6 @@ categories = ["os", "hardware-support"]
 
 
 [dependencies]
-dcap-ql = { version = "0.4.0", path = "../dcap-ql", default-features = false }
 sgx-isa = { version = "0.4.1", path = "../sgx-isa", default-features = true }
 pkix = "0.2.0"
 yasna = { version = "0.3", features = ["num-bigint", "bit-vec"] }
@@ -37,6 +36,7 @@ num = "0.2"
 mbedtls = { version = "0.12.3", features = ["std", "time"], default-features = false, optional = true }
 
 [dev-dependencies]
+dcap-ql = { version = "0.5.0", path = "../dcap-ql", default-features = false }
 hex = "0.4.2"
 
 [features]

intel-sgx/pcs/src/pckcrt.rs

@@ -1008,7 +1008,7 @@ mod tests {
     fn pckcrt_from_quote() {
         let quote = include_bytes!("../tests/data/quote.bin");
         let quote = Quote::parse(Cow::from(&quote[..])).unwrap();
-        let sig = quote.signature::<Quote3SignatureEcdsaP256>().unwrap();
+        let (sig, _) = quote.signature::<Quote3SignatureEcdsaP256>().unwrap();
         let pck_chain: Qe3CertDataPckCertChain = sig.certification_data().unwrap();
         let pck = PckCert::from_pck_chain(pck_chain.certs.into()).unwrap();
         let sgx_extension = pck.sgx_extension().unwrap();

intel-sgx/sgxs-tools/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "sgxs-tools"
-version = "0.8.6"
+version = "0.8.7"
 authors = ["Fortanix, Inc."]
 license = "MPL-2.0"
 description = """
@@ -59,7 +59,7 @@ serde_derive = "1.0.84"                          # MIT/Apache-2.0
 serde_yaml = "0.8.8"                             # MIT/Apache-2.0
 
 [target.'cfg(unix)'.dependencies]
-"dcap-ql" = { version = "0.4.0", path = "../dcap-ql" }
+"dcap-ql" = { version = "0.5.0", path = "../dcap-ql" }
 
 [target.'cfg(windows)'.dependencies]
 winapi = { version = "0.3.7", features = ["winbase"] }