From f38f7c44afe3ff0fbdd0ccb0de529d13c0b31f98 Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Mon, 12 Feb 2024 20:12:09 +1100 Subject: [PATCH] Add a wrapper for slice::from_raw_parts (#1644) * Add a wrapper for slice::from_raw_parts And add a lint to prevent the method from being invoked directly. The documentation on the wrapper explains everything. * Add a newline to the file * Cleanup --------- Co-authored-by: Lars Eggert --- clippy.toml | 3 +++ neqo-crypto/src/agent.rs | 9 +++------ neqo-crypto/src/agentio.rs | 7 ++++--- neqo-crypto/src/cert.rs | 11 +++++------ neqo-crypto/src/ech.rs | 7 +++---- neqo-crypto/src/ext.rs | 3 ++- neqo-crypto/src/lib.rs | 25 +++++++++++++++++++++++++ neqo-crypto/src/p11.rs | 13 +++++++------ 8 files changed, 52 insertions(+), 26 deletions(-) create mode 100644 clippy.toml diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 0000000000..e928b4be64 --- /dev/null +++ b/clippy.toml @@ -0,0 +1,3 @@ +disallowed-methods = [ + { path = "std::slice::from_raw_parts", reason = "see null_safe_slice" } +] diff --git a/neqo-crypto/src/agent.rs b/neqo-crypto/src/agent.rs index cd0bb4cb12..85fc496841 100644 --- a/neqo-crypto/src/agent.rs +++ b/neqo-crypto/src/agent.rs @@ -33,6 +33,7 @@ use crate::{ ech, err::{is_blocked, secstatus_to_res, Error, PRErrorCode, Res}, ext::{ExtensionHandler, ExtensionTracker}, + null_safe_slice, p11::{self, PrivateKey, PublicKey}, prio, replay::AntiReplay, @@ -897,7 +898,7 @@ impl Client { let resumption = arg.cast::>().as_mut().unwrap(); let len = usize::try_from(len).unwrap(); let mut v = Vec::with_capacity(len); - v.extend_from_slice(std::slice::from_raw_parts(token, len)); + v.extend_from_slice(null_safe_slice(token, len)); qinfo!( [format!("{fd:p}")], "Got resumption token {}", @@ -1105,11 +1106,7 @@ impl Server { } let check_state = arg.cast::().as_mut().unwrap(); - let token = if client_token.is_null() { - &[] - } else { - std::slice::from_raw_parts(client_token, usize::try_from(client_token_len).unwrap()) - }; + let token = null_safe_slice(client_token, usize::try_from(client_token_len).unwrap()); match check_state.checker.check(token) { ZeroRttCheckResult::Accept => ssl::SSLHelloRetryRequestAction::ssl_hello_retry_accept, ZeroRttCheckResult::Fail => ssl::SSLHelloRetryRequestAction::ssl_hello_retry_fail, diff --git a/neqo-crypto/src/agentio.rs b/neqo-crypto/src/agentio.rs index 2bcc540530..1b0cf11ba7 100644 --- a/neqo-crypto/src/agentio.rs +++ b/neqo-crypto/src/agentio.rs @@ -20,7 +20,7 @@ use neqo_common::{hex, hex_with_len, qtrace}; use crate::{ constants::{ContentType, Epoch}, err::{nspr, Error, PR_SetError, Res}, - prio, ssl, + null_safe_slice, prio, ssl, }; // Alias common types. @@ -100,7 +100,7 @@ impl RecordList { ) -> ssl::SECStatus { let records = arg.cast::().as_mut().unwrap(); - let slice = std::slice::from_raw_parts(data, len as usize); + let slice = null_safe_slice(data, len); records.append(epoch, ContentType::try_from(ct).unwrap(), slice); ssl::SECSuccess } @@ -178,6 +178,7 @@ impl AgentIoInput { return Err(Error::NoDataAvailable); } + #[allow(clippy::disallowed_methods)] // We just checked if this was empty. let src = unsafe { std::slice::from_raw_parts(self.input, amount) }; qtrace!([self], "read {}", hex(src)); let dst = unsafe { std::slice::from_raw_parts_mut(buf, amount) }; @@ -232,7 +233,7 @@ impl AgentIo { // Stage output from TLS into the output buffer. fn save_output(&mut self, buf: *const u8, count: usize) { - let slice = unsafe { std::slice::from_raw_parts(buf, count) }; + let slice = unsafe { null_safe_slice(buf, count) }; qtrace!([self], "save output {}", hex(slice)); self.output.extend_from_slice(slice); } diff --git a/neqo-crypto/src/cert.rs b/neqo-crypto/src/cert.rs index 64e63ec71a..2c16380ee0 100644 --- a/neqo-crypto/src/cert.rs +++ b/neqo-crypto/src/cert.rs @@ -7,13 +7,13 @@ use std::{ convert::TryFrom, ptr::{addr_of, NonNull}, - slice, }; use neqo_common::qerror; use crate::{ err::secstatus_to_res, + null_safe_slice, p11::{CERTCertListNode, CERT_GetCertificateDer, CertList, Item, SECItem, SECItemArray}, ssl::{ PRFileDesc, SSL_PeerCertificateChain, SSL_PeerSignedCertTimestamps, @@ -52,7 +52,7 @@ fn stapled_ocsp_responses(fd: *mut PRFileDesc) -> Option>> { }; for idx in 0..len { let itemp: *const SECItem = unsafe { ocsp_ptr.as_ref().items.offset(idx).cast() }; - let item = unsafe { slice::from_raw_parts((*itemp).data, (*itemp).len as usize) }; + let item = unsafe { null_safe_slice((*itemp).data, (*itemp).len) }; ocsp_helper.push(item.to_owned()); } Some(ocsp_helper) @@ -68,9 +68,8 @@ fn signed_cert_timestamp(fd: *mut PRFileDesc) -> Option> { if unsafe { sct_ptr.as_ref().len == 0 || sct_ptr.as_ref().data.is_null() } { Some(Vec::new()) } else { - let sct_slice = unsafe { - slice::from_raw_parts(sct_ptr.as_ref().data, sct_ptr.as_ref().len as usize) - }; + let sct_slice = + unsafe { null_safe_slice(sct_ptr.as_ref().data, sct_ptr.as_ref().len) }; Some(sct_slice.to_owned()) } } @@ -105,7 +104,7 @@ impl<'a> Iterator for &'a mut CertificateInfo { let cert = unsafe { *self.cursor }.cert; secstatus_to_res(unsafe { CERT_GetCertificateDer(cert, &mut item) }) .expect("getting DER from certificate should work"); - Some(unsafe { std::slice::from_raw_parts(item.data, item.len as usize) }) + Some(unsafe { null_safe_slice(item.data, item.len) }) } } diff --git a/neqo-crypto/src/ech.rs b/neqo-crypto/src/ech.rs index 109d745520..6f9a3ba4ce 100644 --- a/neqo-crypto/src/ech.rs +++ b/neqo-crypto/src/ech.rs @@ -15,7 +15,7 @@ use neqo_common::qtrace; use crate::{ err::{ssl::SSL_ERROR_ECH_RETRY_WITH_ECH, Error, Res}, - experimental_api, + experimental_api, null_safe_slice, p11::{ self, Item, PrivateKey, PublicKey, SECITEM_FreeItem, SECItem, SECKEYPrivateKey, SECKEYPublicKey, Slot, @@ -76,7 +76,7 @@ pub fn convert_ech_error(fd: *mut PRFileDesc, err: Error) -> Error { return Error::InternalError; } let buf = unsafe { - let slc = std::slice::from_raw_parts(item.data, usize::try_from(item.len).unwrap()); + let slc = null_safe_slice(item.data, item.len); let buf = Vec::from(slc); SECITEM_FreeItem(&mut item, PRBool::from(false)); buf @@ -101,8 +101,7 @@ pub fn generate_keys() -> Res<(PrivateKey, PublicKey)> { let oid_data = unsafe { p11::SECOID_FindOIDByTag(p11::SECOidTag::SEC_OID_CURVE25519) }; let oid = unsafe { oid_data.as_ref() }.ok_or(Error::InternalError)?; - let oid_slc = - unsafe { std::slice::from_raw_parts(oid.oid.data, usize::try_from(oid.oid.len).unwrap()) }; + let oid_slc = unsafe { null_safe_slice(oid.oid.data, oid.oid.len) }; let mut params: Vec = Vec::with_capacity(oid_slc.len() + 2); params.push(u8::try_from(p11::SEC_ASN1_OBJECT_ID).unwrap()); params.push(u8::try_from(oid.oid.len).unwrap()); diff --git a/neqo-crypto/src/ext.rs b/neqo-crypto/src/ext.rs index 310e87a1b7..d9f3195051 100644 --- a/neqo-crypto/src/ext.rs +++ b/neqo-crypto/src/ext.rs @@ -16,6 +16,7 @@ use crate::{ agentio::as_c_void, constants::{Extension, HandshakeMessage, TLS_HS_CLIENT_HELLO, TLS_HS_ENCRYPTED_EXTENSIONS}, err::Res, + null_safe_slice, ssl::{ PRBool, PRFileDesc, SECFailure, SECStatus, SECSuccess, SSLAlertDescription, SSLExtensionHandler, SSLExtensionWriter, SSLHandshakeType, @@ -105,7 +106,7 @@ impl ExtensionTracker { alert: *mut SSLAlertDescription, arg: *mut c_void, ) -> SECStatus { - let d = std::slice::from_raw_parts(data, len as usize); + let d = null_safe_slice(data, len); #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] Self::wrap_handler_call(arg, |handler| { // Cast is safe here because the message type is always part of the enum diff --git a/neqo-crypto/src/lib.rs b/neqo-crypto/src/lib.rs index 4a23b5a7b1..3e9260813f 100644 --- a/neqo-crypto/src/lib.rs +++ b/neqo-crypto/src/lib.rs @@ -37,6 +37,7 @@ mod ssl; mod time; use std::{ + convert::TryFrom, ffi::CString, path::{Path, PathBuf}, ptr::null, @@ -200,3 +201,27 @@ pub fn assert_initialized() { .get() .expect("NSS not initialized with init or init_db"); } + +/// NSS tends to return empty "slices" with a null pointer, which will cause +/// `std::slice::from_raw_parts` to panic if passed directly. This wrapper avoids +/// that issue. It also performs conversion for lengths, as a convenience. +/// +/// # Panics +/// If the provided length doesn't fit into a `usize`. +/// +/// # Safety +/// The caller must adhere to the safety constraints of `std::slice::from_raw_parts`, +/// except that this will accept a null value for `data`. +unsafe fn null_safe_slice<'a, T>(data: *const u8, len: T) -> &'a [u8] +where + usize: TryFrom, +{ + if data.is_null() { + &[] + } else if let Ok(len) = usize::try_from(len) { + #[allow(clippy::disallowed_methods)] + std::slice::from_raw_parts(data, len) + } else { + panic!("null_safe_slice: size overflow"); + } +} diff --git a/neqo-crypto/src/p11.rs b/neqo-crypto/src/p11.rs index 2225d5b211..6ec9370360 100644 --- a/neqo-crypto/src/p11.rs +++ b/neqo-crypto/src/p11.rs @@ -20,7 +20,10 @@ use std::{ use neqo_common::hex_with_len; -use crate::err::{secstatus_to_res, Error, Res}; +use crate::{ + err::{secstatus_to_res, Error, Res}, + null_safe_slice, +}; #[allow(clippy::upper_case_acronyms)] #[allow(clippy::unreadable_literal)] @@ -148,9 +151,7 @@ impl PrivateKey { &mut key_item, ) })?; - let slc = unsafe { - std::slice::from_raw_parts(key_item.data, usize::try_from(key_item.len).unwrap()) - }; + let slc = unsafe { null_safe_slice(key_item.data, key_item.len) }; let key = Vec::from(slc); // The data that `key_item` refers to needs to be freed, but we can't // use the scoped `Item` implementation. This is OK as long as nothing @@ -206,7 +207,7 @@ impl SymKey { // This is accessing a value attached to the key, so we can treat this as a borrow. match unsafe { key_item.as_mut() } { None => Err(Error::InternalError), - Some(key) => Ok(unsafe { std::slice::from_raw_parts(key.data, key.len as usize) }), + Some(key) => Ok(unsafe { null_safe_slice(key.data, key.len) }), } } } @@ -285,7 +286,7 @@ impl Item { let b = self.ptr.as_ref().unwrap(); // Sanity check the type, as some types don't count bytes in `Item::len`. assert_eq!(b.type_, SECItemType::siBuffer); - let slc = std::slice::from_raw_parts(b.data, usize::try_from(b.len).unwrap()); + let slc = null_safe_slice(b.data, b.len); Vec::from(slc) } }