From 45ed1c7c3c6d4581f40dd4a0355a85937fe13d66 Mon Sep 17 00:00:00 2001 From: Kornel Date: Wed, 4 Feb 2026 17:47:18 +0000 Subject: [PATCH] Safer slices --- boring/src/asn1.rs | 22 ++++-------- boring/src/bio.rs | 20 ++++++----- boring/src/lib.rs | 46 ++++++++++++++++++++++-- boring/src/pkey.rs | 8 ++--- boring/src/ssl/bio.rs | 27 ++++++++------ boring/src/ssl/callbacks.rs | 7 ++-- boring/src/ssl/mod.rs | 70 ++++++++++--------------------------- boring/src/util.rs | 4 +-- 8 files changed, 107 insertions(+), 97 deletions(-) diff --git a/boring/src/asn1.rs b/boring/src/asn1.rs index 27f375f29..81ab709aa 100644 --- a/boring/src/asn1.rs +++ b/boring/src/asn1.rs @@ -24,23 +24,23 @@ //! use boring::asn1::Asn1Time; //! let tomorrow = Asn1Time::days_from_now(1); //! ``` -use crate::ffi; use foreign_types::{ForeignType, ForeignTypeRef}; use libc::{c_int, c_long, time_t}; use std::cmp::Ordering; use std::ffi::CString; use std::fmt; use std::ptr; -use std::slice; use std::str; use crate::bio::MemBio; use crate::bn::{BigNum, BigNumRef}; use crate::error::ErrorStack; +use crate::ffi; use crate::nid::Nid; use crate::stack::Stackable; use crate::string::OpensslString; -use crate::{cvt, cvt_p}; +use crate::try_slice; +use crate::{cvt, cvt_n, cvt_p}; use openssl_macros::corresponds; foreign_type_and_impl_send_sync! { @@ -403,11 +403,7 @@ impl Asn1StringRef { pub fn as_utf8(&self) -> Result { unsafe { let mut ptr = ptr::null_mut(); - let len = ffi::ASN1_STRING_to_UTF8(&mut ptr, self.as_ptr()); - if len < 0 { - return Err(ErrorStack::get()); - } - + cvt_n(ffi::ASN1_STRING_to_UTF8(&mut ptr, self.as_ptr()))?; Ok(OpensslString::from_ptr(ptr.cast())) } } @@ -421,7 +417,7 @@ impl Asn1StringRef { #[corresponds(ASN1_STRING_get0_data)] #[must_use] pub fn as_slice(&self) -> &[u8] { - unsafe { slice::from_raw_parts(ASN1_STRING_get0_data(self.as_ptr()), self.len()) } + unsafe { try_slice(ASN1_STRING_get0_data(self.as_ptr()), self.len()).unwrap_or(&[]) } } /// Returns the number of bytes in the string. @@ -529,13 +525,7 @@ impl Asn1BitStringRef { #[corresponds(ASN1_STRING_get0_data)] #[must_use] pub fn as_slice(&self) -> &[u8] { - unsafe { - let ptr = ASN1_STRING_get0_data(self.as_ptr().cast()); - if ptr.is_null() { - return &[]; - } - slice::from_raw_parts(ptr, self.len()) - } + unsafe { try_slice(ASN1_STRING_get0_data(self.as_ptr().cast()), self.len()).unwrap_or(&[]) } } /// Returns the Asn1BitString as a str, if possible. diff --git a/boring/src/bio.rs b/boring/src/bio.rs index bb4aa1a54..5b4609a51 100644 --- a/boring/src/bio.rs +++ b/boring/src/bio.rs @@ -1,12 +1,11 @@ use std::marker::PhantomData; use std::ptr; -use std::slice; -use crate::cvt_p; use crate::error::ErrorStack; use crate::ffi; use crate::ffi::BIO_new_mem_buf; -use crate::try_int; +use crate::{cvt, cvt_p}; +use crate::{try_int, try_slice}; pub struct MemBioSlice<'a>(*mut ffi::BIO, PhantomData<&'a [u8]>); @@ -54,14 +53,17 @@ impl MemBio { self.0 } + /// An empty slice may indicate an error, use [`Self::try_get_buf`] instead. pub fn get_buf(&self) -> &[u8] { + self.try_get_buf().unwrap_or(&[]) + } + + pub fn try_get_buf(&self) -> Result<&[u8], ErrorStack> { unsafe { - let mut ptr = ptr::null_mut(); - let len = ffi::BIO_get_mem_data(self.0, &mut ptr); - if ptr.is_null() || len < 0 { - return &[]; - } - slice::from_raw_parts(ptr.cast_const().cast(), len as usize) + let mut ptr: *const u8 = ptr::null_mut(); + let mut len = 0; + cvt(ffi::BIO_mem_contents(self.0, &mut ptr, &mut len))?; + try_slice(ptr, len).ok_or_else(|| ErrorStack::internal_error_str("invalid slice")) } } } diff --git a/boring/src/lib.rs b/boring/src/lib.rs index d1d87e595..0de3be972 100644 --- a/boring/src/lib.rs +++ b/boring/src/lib.rs @@ -170,11 +170,11 @@ fn cvt_0(r: usize) -> Result<(), ErrorStack> { } } -fn cvt_0i(r: c_int) -> Result { +fn cvt_0i(r: c_int) -> Result<(), ErrorStack> { if r == 0 { Err(ErrorStack::get()) } else { - Ok(r) + Ok(()) } } @@ -201,6 +201,48 @@ fn cvt_n(r: c_int) -> Result { } } +unsafe fn try_slice<'a, T: Sized + 'static, L: TryInto + Copy + 'static>( + ptr: *const T, + len_in_items: L, +) -> Option<&'a [T]> { + safe_slice_size::(len_in_items) + .filter(|_| !ptr.is_null()) + .map(|len| { + // C/C++ may assume the pointer can be anything if it's not dereferenced, which isn't true in Rust + if len > 0 { + std::slice::from_raw_parts(ptr, len) + } else { + &[] + } + }) +} + +unsafe fn try_slice_mut<'a, T: Sized + 'static, L: TryInto + Copy + 'static>( + ptr: *mut T, + len_in_items: L, +) -> Result<&'a mut [T], ErrorStack> { + safe_slice_size::(len_in_items) + .filter(|_| !ptr.is_null()) + .map(|len| { + if len > 0 { + std::slice::from_raw_parts_mut(ptr, len) + } else { + &mut [] + } + }) + .ok_or_else(|| ErrorStack::internal_error_str("invalid slice")) +} + +fn safe_slice_size>(len_in_items: L) -> Option { + let len = len_in_items.try_into().ok()?; + // it's UB to have larger allocation size in Rust + if len.checked_mul(size_of::())? < isize::MAX as usize { + Some(len) + } else { + None + } +} + fn try_int(from: F) -> Result where F: TryInto + Send + Sync + Copy + 'static, diff --git a/boring/src/pkey.rs b/boring/src/pkey.rs index 0ab9c5288..a162d1623 100644 --- a/boring/src/pkey.rs +++ b/boring/src/pkey.rs @@ -235,7 +235,7 @@ where pub fn raw_public_key_len(&self) -> Result { unsafe { let mut size = 0; - _ = cvt_0i(ffi::EVP_PKEY_get_raw_public_key( + cvt_0i(ffi::EVP_PKEY_get_raw_public_key( self.as_ptr(), std::ptr::null_mut(), &mut size, @@ -251,7 +251,7 @@ where pub fn raw_public_key<'a>(&self, out: &'a mut [u8]) -> Result<&'a [u8], ErrorStack> { unsafe { let mut size = out.len(); - _ = cvt_0i(ffi::EVP_PKEY_get_raw_public_key( + cvt_0i(ffi::EVP_PKEY_get_raw_public_key( self.as_ptr(), out.as_mut_ptr(), &mut size, @@ -303,7 +303,7 @@ where pub fn raw_private_key_len(&self) -> Result { unsafe { let mut size = 0; - _ = cvt_0i(ffi::EVP_PKEY_get_raw_private_key( + cvt_0i(ffi::EVP_PKEY_get_raw_private_key( self.as_ptr(), std::ptr::null_mut(), &mut size, @@ -319,7 +319,7 @@ where pub fn raw_private_key<'a>(&self, out: &'a mut [u8]) -> Result<&'a [u8], ErrorStack> { unsafe { let mut size = out.len(); - _ = cvt_0i(ffi::EVP_PKEY_get_raw_private_key( + cvt_0i(ffi::EVP_PKEY_get_raw_private_key( self.as_ptr(), out.as_mut_ptr(), &mut size, diff --git a/boring/src/ssl/bio.rs b/boring/src/ssl/bio.rs index 3bae8a79b..d49387a8e 100644 --- a/boring/src/ssl/bio.rs +++ b/boring/src/ssl/bio.rs @@ -8,10 +8,10 @@ use std::io; use std::io::prelude::*; use std::panic::{catch_unwind, AssertUnwindSafe}; use std::ptr; -use std::slice; -use crate::cvt_p; use crate::error::ErrorStack; +use crate::{cvt_p, try_int}; +use crate::{try_slice, try_slice_mut}; pub struct StreamState { pub stream: S, @@ -106,7 +106,9 @@ unsafe extern "C" fn bwrite(bio: *mut BIO, buf: *const c_char, len: c_ }; let state = state::(bio); - let buf = slice::from_raw_parts(buf.cast(), len); + let Some(buf) = try_slice(buf.cast(), len) else { + return -1; + }; match catch_unwind(AssertUnwindSafe(|| state.stream.write(buf))) { Ok(Ok(len)) => len as c_int, @@ -127,15 +129,20 @@ unsafe extern "C" fn bwrite(bio: *mut BIO, buf: *const c_char, len: c_ unsafe extern "C" fn bread(bio: *mut BIO, buf: *mut c_char, len: c_int) -> c_int { BIO_clear_retry_flags(bio); - let Ok(len) = usize::try_from(len) else { - return -1; - }; - let state = state::(bio); - let buf = slice::from_raw_parts_mut(buf.cast(), len); - match catch_unwind(AssertUnwindSafe(|| state.stream.read(buf))) { - Ok(Ok(len)) => len as c_int, + let buf = try_int(len) + .and_then(|len: usize| unsafe { try_slice_mut(buf.cast(), len) }) + .map_err(io::Error::other); + + let res = catch_unwind(AssertUnwindSafe(|| { + state + .stream + .read(buf?) + .and_then(|len| c_int::try_from(len).map_err(io::Error::other)) + })); + match res { + Ok(Ok(len)) => len, Ok(Err(err)) => { if retriable_error(&err) { BIO_set_retry_read(bio); diff --git a/boring/src/ssl/callbacks.rs b/boring/src/ssl/callbacks.rs index 41c08c6b9..73112bbcf 100644 --- a/boring/src/ssl/callbacks.rs +++ b/boring/src/ssl/callbacks.rs @@ -12,6 +12,7 @@ use crate::hmac::HmacCtxRef; use crate::ssl::TicketKeyCallbackResult; use crate::symm::CipherCtxRef; use crate::x509::{X509StoreContext, X509StoreContextRef}; +use crate::{try_slice, try_slice_mut}; use foreign_types::ForeignType; use foreign_types::ForeignTypeRef; use libc::{c_char, c_int, c_uchar, c_uint, c_void}; @@ -666,7 +667,9 @@ where .ex_data(SslContext::cached_ex_index::()) .expect("BUG: certificate compression missed"); - let input_slice = unsafe { std::slice::from_raw_parts(input, input_len) }; + let Some(input_slice) = (unsafe { try_slice(input, input_len) }) else { + return 0; + }; let mut writer = CryptoByteBuilder::from_ptr(out); if compressor.compress(input_slice, &mut writer).is_err() { return 0; @@ -755,7 +758,7 @@ impl<'a> CryptoBufferBuilder<'a> { let buffer = unsafe { crate::cvt_p(ffi::CRYPTO_BUFFER_alloc(&mut data, capacity))? }; Ok(CryptoBufferBuilder { buffer, - cursor: std::io::Cursor::new(unsafe { std::slice::from_raw_parts_mut(data, capacity) }), + cursor: std::io::Cursor::new(unsafe { try_slice_mut(data, capacity)? }), }) } diff --git a/boring/src/ssl/mod.rs b/boring/src/ssl/mod.rs index 802e09390..71c3a8850 100644 --- a/boring/src/ssl/mod.rs +++ b/boring/src/ssl/mod.rs @@ -92,7 +92,6 @@ use crate::ssl::callbacks::*; use crate::ssl::error::InnerError; use crate::stack::{Stack, StackRef, Stackable}; use crate::symm::CipherCtxRef; -use crate::try_int; use crate::x509::store::{X509Store, X509StoreBuilder, X509StoreBuilderRef, X509StoreRef}; use crate::x509::verify::X509VerifyParamRef; use crate::x509::{ @@ -100,6 +99,7 @@ use crate::x509::{ }; use crate::{cvt, cvt_0i, cvt_n, cvt_p, init}; use crate::{ffi, free_data_box}; +use crate::{try_int, try_slice}; pub use self::async_callbacks::{ AsyncPrivateKeyMethod, AsyncPrivateKeyMethodError, AsyncSelectCertError, BoxCustomVerifyFinish, @@ -790,7 +790,7 @@ pub fn select_next_proto<'a>(server: &'a [u8], client: &'a [u8]) -> Option<&'a [ ); if r == ffi::OPENSSL_NPN_NEGOTIATED { - Some(slice::from_raw_parts(out.cast_const(), outlen as usize)) + try_slice(out.cast_const(), outlen) } else { None } @@ -1967,9 +1967,8 @@ impl SslContextBuilder { cvt_0i(ffi::SSL_CTX_set_verify_algorithm_prefs( self.as_ptr(), prefs.as_ptr().cast(), - prefs.len(), + try_int(prefs.len())?, )) - .map(|_| ()) } } @@ -1994,7 +1993,6 @@ impl SslContextBuilder { self.as_ptr(), curves.as_ptr(), )) - .map(|_| ()) } } @@ -2003,7 +2001,7 @@ impl SslContextBuilder { /// This feature isn't available in the certified version of BoringSSL. #[corresponds(SSL_CTX_set_compliance_policy)] pub fn set_compliance_policy(&mut self, policy: CompliancePolicy) -> Result<(), ErrorStack> { - unsafe { cvt_0i(ffi::SSL_CTX_set_compliance_policy(self.as_ptr(), policy.0)).map(|_| ()) } + unsafe { cvt_0i(ffi::SSL_CTX_set_compliance_policy(self.as_ptr(), policy.0)) } } /// Sets the context's info callback. @@ -2035,7 +2033,6 @@ impl SslContextBuilder { self.as_ptr(), credential.as_ptr(), )) - .map(|_| ()) } } @@ -2053,7 +2050,6 @@ impl SslContextBuilder { types.as_ptr() as *const u8, types.len(), )) - .map(|_| ()) } } @@ -2350,11 +2346,7 @@ impl SslContextRef { if types_len == 0 { return None; } - - Some(slice::from_raw_parts( - types as *const CertificateType, - types_len, - )) + try_slice(types.cast::(), types_len) } } } @@ -2391,7 +2383,7 @@ impl ClientHello<'_> { if result == 0 { return None; } - Some(slice::from_raw_parts(ptr, len)) + try_slice(ptr, len) } } @@ -2425,19 +2417,19 @@ impl ClientHello<'_> { /// Returns the raw data of the client hello message #[must_use] pub fn as_bytes(&self) -> &[u8] { - unsafe { slice::from_raw_parts(self.0.client_hello, self.0.client_hello_len) } + unsafe { try_slice(self.0.client_hello, self.0.client_hello_len).unwrap_or(&[]) } } /// Returns the client random data #[must_use] pub fn random(&self) -> &[u8] { - unsafe { slice::from_raw_parts(self.0.random, self.0.random_len) } + unsafe { try_slice(self.0.random, self.0.random_len).unwrap_or(&[]) } } /// Returns the raw list of ciphers supported by the client in its Client Hello record. #[must_use] pub fn ciphers(&self) -> &[u8] { - unsafe { slice::from_raw_parts(self.0.cipher_suites, self.0.cipher_suites_len) } + unsafe { try_slice(self.0.cipher_suites, self.0.cipher_suites_len).unwrap_or(&[]) } } } @@ -2648,7 +2640,7 @@ impl SslSessionRef { unsafe { let mut len = 0; let p = ffi::SSL_SESSION_get_id(self.as_ptr(), &mut len); - slice::from_raw_parts(p, len as usize) + try_slice(p, len).unwrap_or(&[]) } } @@ -2896,7 +2888,7 @@ impl SslRef { #[corresponds(SSL_set1_curves_list)] pub fn set_curves_list(&mut self, curves: &str) -> Result<(), ErrorStack> { let curves = CString::new(curves).map_err(ErrorStack::internal_error)?; - unsafe { cvt_0i(ffi::SSL_set1_curves_list(self.as_ptr(), curves.as_ptr())).map(|_| ()) } + unsafe { cvt_0i(ffi::SSL_set1_curves_list(self.as_ptr(), curves.as_ptr())) } } /// Returns the curve ID (aka group ID) used for this `SslRef`. @@ -3302,12 +3294,7 @@ impl SslRef { // Get the negotiated protocol from the SSL instance. // `data` will point at a `c_uchar` array; `len` will contain the length of this array. ffi::SSL_get0_alpn_selected(self.as_ptr(), &mut data, &mut len); - - if data.is_null() { - None - } else { - Some(slice::from_raw_parts(data, len as usize)) - } + try_slice(data, len) } } @@ -3553,12 +3540,7 @@ impl SslRef { unsafe { let mut p = ptr::null(); let len = ffi::SSL_get_tlsext_status_ocsp_resp(self.as_ptr(), &mut p); - - if len == 0 { - None - } else { - Some(slice::from_raw_parts(p, len)) - } + try_slice(p, len) } } @@ -3743,7 +3725,6 @@ impl SslRef { ech_config_list.as_ptr(), ech_config_list.len(), )) - .map(|_| ()) } } @@ -3760,12 +3741,7 @@ impl SslRef { let mut data = ptr::null(); let mut len: usize = 0; ffi::SSL_get0_ech_retry_configs(self.as_ptr(), &mut data, &mut len); - - if data.is_null() { - None - } else { - Some(slice::from_raw_parts(data, len)) - } + try_slice(data, len) } } @@ -3782,12 +3758,7 @@ impl SslRef { let mut data: *const c_char = ptr::null(); let mut len: usize = 0; ffi::SSL_get0_ech_name_override(self.as_ptr(), &mut data, &mut len); - - if data.is_null() { - None - } else { - Some(slice::from_raw_parts(data.cast::(), len)) - } + try_slice(data.cast(), len) } } @@ -3811,13 +3782,13 @@ impl SslRef { /// Sets the compliance policy on `SSL`. #[corresponds(SSL_set_compliance_policy)] pub fn set_compliance_policy(&mut self, policy: CompliancePolicy) -> Result<(), ErrorStack> { - unsafe { cvt_0i(ffi::SSL_set_compliance_policy(self.as_ptr(), policy.0)).map(|_| ()) } + unsafe { cvt_0i(ffi::SSL_set_compliance_policy(self.as_ptr(), policy.0)) } } /// Adds a credential. #[corresponds(SSL_add1_credential)] pub fn add_credential(&mut self, credential: &SslCredentialRef) -> Result<(), ErrorStack> { - unsafe { cvt_0i(ffi::SSL_add1_credential(self.as_ptr(), credential.as_ptr())).map(|_| ()) } + unsafe { cvt_0i(ffi::SSL_add1_credential(self.as_ptr(), credential.as_ptr())) } } /// Returns the public key sent by the other peer, `None` if there is no ongoing handshake. @@ -3849,7 +3820,6 @@ impl SslRef { types.as_ptr() as *const u8, types.len(), )) - .map(|_| ()) } } @@ -3866,11 +3836,7 @@ impl SslRef { if types_len == 0 { return None; } - - Some(slice::from_raw_parts( - types as *const CertificateType, - types_len, - )) + try_slice(types.cast::(), types_len) } } diff --git a/boring/src/util.rs b/boring/src/util.rs index 6583f0aaf..74f6b182f 100644 --- a/boring/src/util.rs +++ b/boring/src/util.rs @@ -1,9 +1,9 @@ use crate::error::ErrorStack; +use crate::try_slice_mut; use foreign_types::{ForeignType, ForeignTypeRef}; use libc::{c_char, c_int, c_void}; use std::any::Any; use std::panic::{self, AssertUnwindSafe}; -use std::slice; /// Wraps a user-supplied callback and a slot for panics thrown inside the callback (while FFI /// frames are on the stack). @@ -49,7 +49,7 @@ where let callback = &mut *cb_state.cast::>(); let result = panic::catch_unwind(AssertUnwindSafe(|| { - let pass_slice = slice::from_raw_parts_mut(buf.cast::(), size as usize); + let pass_slice = try_slice_mut(buf.cast::(), size)?; callback.cb.take().unwrap()(pass_slice) }));