From 5e0899b73dedacd3613c640bcd1adf55330f7470 Mon Sep 17 00:00:00 2001
From: Mohsen Zohrevandi <mohsen.zohrevandi@fortanix.com>
Date: Mon, 6 Jan 2025 12:06:11 -0800
Subject: [PATCH 1/2] dcap-ql: Fix length validation logic for quote signatures
---
intel-sgx/dcap-ql/src/quote.rs | 65 +++++++++++++++++++++++++++++++---
1 file changed, 61 insertions(+), 4 deletions(-)
diff --git a/intel-sgx/dcap-ql/src/quote.rs b/intel-sgx/dcap-ql/src/quote.rs
index 56f732820..4dd9a54a2 100644
--- a/intel-sgx/dcap-ql/src/quote.rs
+++ b/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;
// ====================================================
@@ -236,14 +236,19 @@ impl<'a> Quote3Signature<'a> for Quote3SignatureEcdsaP256<'a> {
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.
+ data = data.take_prefix(cmp::min(data.len(), sig_len))?;
+
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)?;
@@ -774,4 +779,56 @@ mod tests {
#[cfg(feature = "verify")]
assert!(Quote::verify::<Quote3SignatureEcdsaP256>(TEST_QUOTE, &mut verifier).is_err());
}
+
+ #[test]
+ fn test_quote_with_trailing_zeros() {
+ let with_trailing_zeros = {
+ const TEST_QUOTE: &[u8] = &*include_bytes!("../tests/quote_pck_cert_chain.bin");
+ let mut with_trailing_zeros = vec![0u8; TEST_QUOTE.len() + 3];
+ with_trailing_zeros[..TEST_QUOTE.len()].copy_from_slice(TEST_QUOTE);
+ with_trailing_zeros
+ };
+
+ let quote = Quote::parse(&with_trailing_zeros).unwrap();
+ let &QuoteHeader::V3 {
+ attestation_key_type,
+ ..
+ } = quote.header();
+
+ assert_eq!(attestation_key_type, Quote3AttestationKeyType::EcdsaP256);
+ let sig = quote.signature::<Quote3SignatureEcdsaP256>().unwrap();
+
+ #[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")]
+ assert!(Quote::verify::<Quote3SignatureEcdsaP256>(&with_trailing_zeros, &mut verifier).is_ok())
+ }
+
+ #[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"),
+ }
+ }
}
From a4d33f30cb5741eeb831b14afa746c3476d85f82 Mon Sep 17 00:00:00 2001
From: Mohsen Zohrevandi <mohsen.zohrevandi@fortanix.com>
Date: Fri, 17 Jan 2025 12:16:12 -0800
Subject: [PATCH 2/2] Don't ignore trailing bytes
---
Cargo.lock | 10 ++--
intel-sgx/dcap-provider/Cargo.toml | 4 +-
intel-sgx/dcap-provider/src/lib.rs | 9 +++-
intel-sgx/dcap-ql/Cargo.toml | 2 +-
intel-sgx/dcap-ql/src/quote.rs | 64 ++++++++++++++++--------
intel-sgx/dcap-retrieve-pckid/Cargo.toml | 4 +-
intel-sgx/dcap-retrieve-pckid/src/lib.rs | 7 ++-
intel-sgx/pcs/Cargo.toml | 4 +-
intel-sgx/pcs/src/pckcrt.rs | 2 +-
intel-sgx/sgxs-tools/Cargo.toml | 4 +-
10 files changed, 72 insertions(+), 38 deletions(-)
diff --git a/Cargo.lock b/Cargo.lock
index 279729f0d..2797a1a20 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -778,7 +778,7 @@ dependencies = [
[[package]]
name = "dcap-provider"
-version = "0.4.0"
+version = "0.4.1"
dependencies = [
"byteorder 1.3.4",
"dcap-ql",
@@ -792,7 +792,7 @@ dependencies = [
[[package]]
name = "dcap-ql"
-version = "0.4.0"
+version = "0.5.0"
dependencies = [
"anyhow",
"byteorder 1.3.4",
@@ -823,7 +823,7 @@ dependencies = [
[[package]]
name = "dcap-retrieve-pckid"
-version = "0.2.0"
+version = "0.2.1"
dependencies = [
"aesm-client",
"anyhow",
@@ -2728,7 +2728,7 @@ dependencies = [
[[package]]
name = "pcs"
-version = "0.3.0"
+version = "0.3.1"
dependencies = [
"anyhow",
"b64-ct",
@@ -3724,7 +3724,7 @@ dependencies = [
[[package]]
name = "sgxs-tools"
-version = "0.8.6"
+version = "0.8.7"
dependencies = [
"aesm-client",
"anyhow",
diff --git a/intel-sgx/dcap-provider/Cargo.toml b/intel-sgx/dcap-provider/Cargo.toml
index 1b3d2ceb2..f8217ddb3 100644
--- a/intel-sgx/dcap-provider/Cargo.toml
+++ b/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
diff --git a/intel-sgx/dcap-provider/src/lib.rs b/intel-sgx/dcap-provider/src/lib.rs
index 62521ea9e..5fc2a8d90 100644
--- a/intel-sgx/dcap-provider/src/lib.rs
+++ b/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,
diff --git a/intel-sgx/dcap-ql/Cargo.toml b/intel-sgx/dcap-ql/Cargo.toml
index 4f0a97251..36397b457 100644
--- a/intel-sgx/dcap-ql/Cargo.toml
+++ b/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 = """
diff --git a/intel-sgx/dcap-ql/src/quote.rs b/intel-sgx/dcap-ql/src/quote.rs
index 4dd9a54a2..08bd37031 100644
--- a/intel-sgx/dcap-ql/src/quote.rs
+++ b/intel-sgx/dcap-ql/src/quote.rs
@@ -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,7 +233,7 @@ 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_)
}
@@ -247,7 +249,10 @@ impl<'a> Quote3Signature<'a> for Quote3SignatureEcdsaP256<'a> {
// 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.
- data = data.take_prefix(cmp::min(data.len(), sig_len))?;
+ 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)?;
@@ -267,7 +272,7 @@ impl<'a> Quote3Signature<'a> for Quote3SignatureEcdsaP256<'a> {
);
}
- Ok(Quote3SignatureEcdsaP256 {
+ Ok((Quote3SignatureEcdsaP256 {
signature,
attestation_public_key,
qe3_report,
@@ -275,7 +280,7 @@ impl<'a> Quote3Signature<'a> for Quote3SignatureEcdsaP256<'a> {
authentication_data,
certification_data_type,
certification_data: data,
- })
+ }, trailing))
}
}
@@ -362,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,
..
@@ -543,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))
}
}
@@ -646,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
@@ -727,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]
@@ -782,21 +798,27 @@ mod tests {
#[test]
fn test_quote_with_trailing_zeros() {
- let 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 mut with_trailing_zeros = vec![0u8; TEST_QUOTE.len() + 3];
- with_trailing_zeros[..TEST_QUOTE.len()].copy_from_slice(TEST_QUOTE);
- with_trailing_zeros
+ 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_zeros).unwrap();
+ 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 = quote.signature::<Quote3SignatureEcdsaP256>().unwrap();
+ let (sig, trailing) = quote.signature::<Quote3SignatureEcdsaP256>().unwrap();
+
+ assert_eq!(&*trailing, expected_trailing);
#[cfg(feature = "verify")]
let mut verifier = MyVerifier {
@@ -804,8 +826,10 @@ 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>(&with_trailing_zeros, &mut verifier).is_ok())
+ #[cfg(feature = "verify")] {
+ let (_, trailing) = Quote::verify::<Quote3SignatureEcdsaP256>(&with_trailing_data, &mut verifier).unwrap();
+ assert_eq!(&*trailing, expected_trailing);
+ }
}
#[test]
diff --git a/intel-sgx/dcap-retrieve-pckid/Cargo.toml b/intel-sgx/dcap-retrieve-pckid/Cargo.toml
index bffddc56f..71b8d6306 100644
--- a/intel-sgx/dcap-retrieve-pckid/Cargo.toml
+++ b/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" }
diff --git a/intel-sgx/dcap-retrieve-pckid/src/lib.rs b/intel-sgx/dcap-retrieve-pckid/src/lib.rs
index 60ae55bc5..7061834b5 100644
--- a/intel-sgx/dcap-retrieve-pckid/src/lib.rs
+++ b/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"))?;
diff --git a/intel-sgx/pcs/Cargo.toml b/intel-sgx/pcs/Cargo.toml
index a33045e2b..960a3fb89 100644
--- a/intel-sgx/pcs/Cargo.toml
+++ b/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]
diff --git a/intel-sgx/pcs/src/pckcrt.rs b/intel-sgx/pcs/src/pckcrt.rs
index 4e03335ac..591a1b911 100644
--- a/intel-sgx/pcs/src/pckcrt.rs
+++ b/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("e[..])).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();
diff --git a/intel-sgx/sgxs-tools/Cargo.toml b/intel-sgx/sgxs-tools/Cargo.toml
index e967d4be2..3624c104f 100644
--- a/intel-sgx/sgxs-tools/Cargo.toml
+++ b/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"] }