Skip to content

fix(migtd): bound untrusted input sizes before allocation #1012

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
MichalTarnacki wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(migtd): bound untrusted input sizes before allocation #1012

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/crypto/src/rustls_impl/tls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ use super::ecdsa::EcdsaPk;

pub type TlsLibError = rustls::Error;
const TLS_CUSTOM_CALLBACK_ERROR: &str = "TlsCustomCallbackError";
pub const TLS_BUFFER_SIZE: usize = 16 * 0x1000;

pub struct SecureChannel<T: AsyncRead + AsyncWrite + Unpin> {
conn: TlsConnection<T>,
Expand Down Expand Up @@ -381,6 +382,7 @@ impl ClientCertVerifier for Verifier {
}

pub(crate) mod connection {
use super::TLS_BUFFER_SIZE;
use alloc::{collections::VecDeque, sync::Arc, vec::Vec};
use async_io::{AsyncRead, AsyncWrite};
use rust_std_stub::io;
Expand All @@ -396,7 +398,6 @@ pub(crate) mod connection {
use zeroize::Zeroize;

pub const PAGE_SIZE: usize = 0x1000;
pub const TLS_BUFFER_SIZE: usize = 16 * PAGE_SIZE;
pub const APP_DATA_BUFFER_LIMIT: usize = PAGE_SIZE;

#[derive(Debug)]
Expand Down
9 changes: 9 additions & 0 deletions src/migtd/src/migration/pre_session_data.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,15 @@ pub(super) async fn receive_pre_session_data_packet<T: AsyncRead + AsyncWrite +
}

let pre_session_data_payload_size = header.length as usize;
const MAX_PRE_SESSION_PAYLOAD: usize = 64 * 1024; // 64 KiB
if pre_session_data_payload_size > MAX_PRE_SESSION_PAYLOAD {
log::error!(
"receive_pre_session_data_packet: payload size {} exceeds max {}\n",
pre_session_data_payload_size,
MAX_PRE_SESSION_PAYLOAD
);
return Err(MigrationResult::InvalidParameter);
}
let mut pre_session_data_payload = vec![0u8; pre_session_data_payload_size];
receive_pre_session_data(transport, &mut pre_session_data_payload)
.await
Expand Down
43 changes: 25 additions & 18 deletions src/migtd/src/ratls/server_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use async_io::{AsyncRead, AsyncWrite};
use crypto::{
ecdsa::EcdsaPk,
hash::digest_sha384,
tls::{SecureChannel, TlsConfig},
tls::{SecureChannel, TlsConfig, TLS_BUFFER_SIZE},
x509::{
AlgorithmIdentifier, AnyRef, BitStringRef, Certificate, CertificateBuilder, Decode, Encode,
ExtendedKeyUsage, Extension, Extensions, Tag,
Expand Down Expand Up @@ -762,6 +762,25 @@ fn verify_client_cert(cert: &[u8], quote: &[u8]) -> core::result::Result<(), Cry
verify_peer_cert(false, cert, quote)
}

// A certificate larger than the whole TLS input buffer can never be received.
const MAX_PEER_CERTIFICATE_SIZE: usize = TLS_BUFFER_SIZE;

fn parse_peer_certificate(cert: &[u8]) -> core::result::Result<Certificate, CryptoError> {
if cert.len() > MAX_PEER_CERTIFICATE_SIZE {
log::error!(
"Certificate too large: {} bytes (max {})\n",
cert.len(),
MAX_PEER_CERTIFICATE_SIZE
);
return Err(CryptoError::ParseCertificate);
}

Certificate::from_der(cert).map_err(|e| {
log::error!("Failed to parse certificate from DER. Error: {:?}\n", e);
CryptoError::ParseCertificate
})
}

#[cfg(not(feature = "test_disable_ra_and_accept_all"))]
mod verify {
use super::*;
Expand All @@ -782,10 +801,7 @@ mod verify {
log::error!("Mutual attestation error {:?}.\n", e);
CryptoError::TlsVerifyPeerCert(MUTUAL_ATTESTATION_ERROR.to_string())
})?;
let cert = Certificate::from_der(cert).map_err(|e| {
log::error!("Failed to parse certificate from DER. Error: {:?}\n", e);
CryptoError::ParseCertificate
})?;
let cert = parse_peer_certificate(cert)?;
let extensions = cert.tbs_certificate.extensions.as_ref().ok_or_else(|| {
log::error!("Failed to get certificate extensions.\n");
CryptoError::ParseCertificate
Expand Down Expand Up @@ -850,10 +866,7 @@ mod verify {
cert: &[u8],
peer_data: &[u8],
) -> core::result::Result<(), CryptoError> {
let cert = Certificate::from_der(cert).map_err(|_| {
log::error!("Failed to parse certificate from DER.\n");
CryptoError::ParseCertificate
})?;
let cert = parse_peer_certificate(cert)?;

let extensions = cert.tbs_certificate.extensions.as_ref().ok_or_else(|| {
log::error!("Failed to get certificate extensions.\n");
Expand Down Expand Up @@ -915,10 +928,7 @@ mod verify {
cert: &[u8],
peer_data: &[u8],
) -> core::result::Result<(), CryptoError> {
let cert = Certificate::from_der(cert).map_err(|_| {
log::error!("Failed to parse certificate from DER.\n");
CryptoError::ParseCertificate
})?;
let cert = parse_peer_certificate(cert)?;

let extensions = cert.tbs_certificate.extensions.as_ref().ok_or_else(|| {
log::error!("Failed to get certificate extensions.\n");
Expand Down Expand Up @@ -994,10 +1004,7 @@ mod verify {
cert: &[u8],
peer_data: &[u8],
) -> core::result::Result<(), CryptoError> {
let cert = Certificate::from_der(cert).map_err(|_| {
log::error!("Failed to parse certificate from DER.\n");
CryptoError::ParseCertificate
})?;
let cert = parse_peer_certificate(cert)?;

let extensions = cert.tbs_certificate.extensions.as_ref().ok_or_else(|| {
log::error!("Failed to get certificate extensions.\n");
Expand Down Expand Up @@ -1182,7 +1189,7 @@ mod verify {
cert: &[u8],
_quote_local: &[u8],
) -> core::result::Result<(), CryptoError> {
let cert = Certificate::from_der(cert).map_err(|_| CryptoError::ParseCertificate)?;
let cert = parse_peer_certificate(cert)?;

let extensions = cert
.tbs_certificate
Expand Down
Loading