From af5bb39a783c08f62e3ae01c1e296dbc66fbee69 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Tue, 1 Aug 2023 16:27:49 +0200 Subject: [PATCH] Clean up and remove some unsafe code from ffi callbacks --- boring/src/ssl/callbacks.rs | 309 +++++++++++++++++++++--------------- boring/src/ssl/mod.rs | 16 +- boring/src/ssl/test/mod.rs | 1 - 3 files changed, 190 insertions(+), 136 deletions(-) diff --git a/boring/src/ssl/callbacks.rs b/boring/src/ssl/callbacks.rs index d3293cb3..1611681f 100644 --- a/boring/src/ssl/callbacks.rs +++ b/boring/src/ssl/callbacks.rs @@ -1,3 +1,5 @@ +#![forbid(unsafe_op_in_unsafe_fn)] + use crate::ffi; use foreign_types::ForeignType; use foreign_types::ForeignTypeRef; @@ -19,31 +21,35 @@ use crate::ssl::{ }; use crate::x509::{X509StoreContext, X509StoreContextRef}; -pub extern "C" fn raw_verify(preverify_ok: c_int, x509_ctx: *mut ffi::X509_STORE_CTX) -> c_int +pub(super) unsafe extern "C" fn raw_verify( + preverify_ok: c_int, + x509_ctx: *mut ffi::X509_STORE_CTX, +) -> c_int where F: Fn(bool, &mut X509StoreContextRef) -> bool + 'static + Sync + Send, { - unsafe { - let ctx = X509StoreContextRef::from_ptr_mut(x509_ctx); - let ssl_idx = X509StoreContext::ssl_idx().expect("BUG: store context ssl index missing"); - let verify_idx = SslContext::cached_ex_index::(); + // SAFETY: boring provides valid inputs. + let ctx = unsafe { X509StoreContextRef::from_ptr_mut(x509_ctx) }; - // raw pointer shenanigans to break the borrow of ctx - // the callback can't mess with its own ex_data slot so this is safe - let verify = ctx - .ex_data(ssl_idx) - .expect("BUG: store context missing ssl") - .ssl_context() - .ex_data(verify_idx) - .expect("BUG: verify callback missing") as *const F; + let ssl_idx = X509StoreContext::ssl_idx().expect("BUG: store context ssl index missing"); + let verify_idx = SslContext::cached_ex_index::(); - (*verify)(preverify_ok != 0, ctx) as c_int - } + let verify = ctx + .ex_data(ssl_idx) + .expect("BUG: store context missing ssl") + .ssl_context() + .ex_data(verify_idx) + .expect("BUG: verify callback missing"); + + // SAFETY: The callback won't outlive the context it's associated with + // because there is no `X509StoreContextRef::ssl_mut(&mut self)` method. + let verify = unsafe { &*(verify as *const F) }; + + verify(preverify_ok != 0, ctx) as c_int } -#[cfg(not(osslconf = "OPENSSL_NO_PSK"))] -pub extern "C" fn raw_client_psk( - ssl: *mut ffi::SSL, +pub(super) unsafe extern "C" fn raw_client_psk( + ssl_ptr: *mut ffi::SSL, hint: *const c_char, identity: *mut c_char, max_identity_len: c_uint, @@ -56,34 +62,36 @@ where + Sync + Send, { - unsafe { - let ssl = SslRef::from_ptr_mut(ssl); - let callback_idx = SslContext::cached_ex_index::(); + // SAFETY: boring provides valid inputs. - let callback = ssl - .ssl_context() - .ex_data(callback_idx) - .expect("BUG: psk callback missing") as *const F; - let hint = if !hint.is_null() { - Some(CStr::from_ptr(hint).to_bytes()) - } else { - None - }; - // Give the callback mutable slices into which it can write the identity and psk. - let identity_sl = slice::from_raw_parts_mut(identity as *mut u8, max_identity_len as usize); - let psk_sl = slice::from_raw_parts_mut(psk as *mut u8, max_psk_len as usize); - match (*callback)(ssl, hint, identity_sl, psk_sl) { - Ok(psk_len) => psk_len as u32, - Err(e) => { - e.put(); - 0 - } + let ssl = unsafe { SslRef::from_ptr_mut(ssl_ptr) }; + + let hint = if !hint.is_null() { + Some(unsafe { CStr::from_ptr(hint) }.to_bytes()) + } else { + None + }; + + // Give the callback mutable slices into which it can write the identity and psk. + let identity_sl = + unsafe { slice::from_raw_parts_mut(identity as *mut u8, max_identity_len as usize) }; + let psk_sl = unsafe { slice::from_raw_parts_mut(psk as *mut u8, max_psk_len as usize) }; + + let ssl_context = ssl.ssl_context().to_owned(); + let callback = ssl_context + .ex_data(SslContext::cached_ex_index::()) + .expect("BUG: psk callback missing"); + + match callback(ssl, hint, identity_sl, psk_sl) { + Ok(psk_len) => psk_len as u32, + Err(e) => { + e.put(); + 0 } } } -#[cfg(not(osslconf = "OPENSSL_NO_PSK"))] -pub extern "C" fn raw_server_psk( +pub(super) unsafe extern "C" fn raw_server_psk( ssl: *mut ffi::SSL, identity: *const c_char, psk: *mut c_uchar, @@ -95,73 +103,90 @@ where + Sync + Send, { - unsafe { - let ssl = SslRef::from_ptr_mut(ssl); - let callback_idx = SslContext::cached_ex_index::(); + // SAFETY: boring provides valid inputs. - let callback = ssl - .ssl_context() - .ex_data(callback_idx) - .expect("BUG: psk callback missing") as *const F; - let identity = if identity.is_null() { - None - } else { - Some(CStr::from_ptr(identity).to_bytes()) - }; - // Give the callback mutable slices into which it can write the psk. - let psk_sl = slice::from_raw_parts_mut(psk as *mut u8, max_psk_len as usize); - match (*callback)(ssl, identity, psk_sl) { - Ok(psk_len) => psk_len as u32, - Err(e) => { - e.put(); - 0 - } + let ssl = unsafe { SslRef::from_ptr_mut(ssl) }; + + let identity = if !identity.is_null() { + Some(unsafe { CStr::from_ptr(identity) }.to_bytes()) + } else { + None + }; + + // Give the callback mutable slices into which it can write the psk. + let psk_sl = unsafe { slice::from_raw_parts_mut(psk as *mut u8, max_psk_len as usize) }; + + let ssl_context = ssl.ssl_context().to_owned(); + let callback = ssl_context + .ex_data(SslContext::cached_ex_index::()) + .expect("BUG: psk callback missing"); + + match callback(ssl, identity, psk_sl) { + Ok(psk_len) => psk_len as u32, + Err(e) => { + e.put(); + 0 } } } -pub extern "C" fn ssl_raw_verify( +pub(super) unsafe extern "C" fn ssl_raw_verify( preverify_ok: c_int, x509_ctx: *mut ffi::X509_STORE_CTX, ) -> c_int where F: Fn(bool, &mut X509StoreContextRef) -> bool + 'static + Sync + Send, { - unsafe { - let ctx = X509StoreContextRef::from_ptr_mut(x509_ctx); - let ssl_idx = X509StoreContext::ssl_idx().expect("BUG: store context ssl index missing"); - let callback_idx = Ssl::cached_ex_index::>(); + // SAFETY: boring provides valid inputs. + let ctx = unsafe { X509StoreContextRef::from_ptr_mut(x509_ctx) }; - let callback = ctx - .ex_data(ssl_idx) - .expect("BUG: store context missing ssl") - .ex_data(callback_idx) - .expect("BUG: ssl verify callback missing") - .clone(); + let ssl_idx = X509StoreContext::ssl_idx().expect("BUG: store context ssl index missing"); - callback(preverify_ok != 0, ctx) as c_int - } + // NOTE(nox): I'm pretty sure this Arc is unnecessary here as there is + // no way to get a `&mut SslRef` from a `&mut X509StoreContextRef`, and I + // don't understand how this callback is different from `raw_verify` above. + let callback = ctx + .ex_data(ssl_idx) + .expect("BUG: store context missing ssl") + .ex_data(Ssl::cached_ex_index::>()) + .expect("BUG: ssl verify callback missing") + .clone(); + + callback(preverify_ok != 0, ctx) as c_int } -pub extern "C" fn raw_sni(ssl: *mut ffi::SSL, al: *mut c_int, arg: *mut c_void) -> c_int +pub(super) unsafe extern "C" fn raw_sni( + ssl: *mut ffi::SSL, + al: *mut c_int, + arg: *mut c_void, +) -> c_int where F: Fn(&mut SslRef, &mut SslAlert) -> Result<(), SniError> + 'static + Sync + Send, { - unsafe { - let ssl = SslRef::from_ptr_mut(ssl); - let callback = arg as *const F; - let mut alert = SslAlert(*al); + // SAFETY: boring provides valid inputs. + let ssl = unsafe { SslRef::from_ptr_mut(ssl) }; + let al = unsafe { &mut *al }; - let r = (*callback)(ssl, &mut alert); - *al = alert.0; - match r { - Ok(()) => ffi::SSL_TLSEXT_ERR_OK, - Err(e) => e.0, - } + // SAFETY: We can make `callback` outlive `ssl` because it is a callback + // stored in the original context with which `ssl` was built. That + // original context is always stored as the session context in + // `Ssl::new` so it is always guaranteed to outlive the lifetime of + // this function's scope. + let callback = unsafe { &*(arg as *const F) }; + + let mut alert = SslAlert(*al); + + let r = callback(ssl, &mut alert); + + *al = alert.0; + + match r { + Ok(()) => ffi::SSL_TLSEXT_ERR_OK, + Err(e) => e.0, } } -pub extern "C" fn raw_alpn_select( +pub(super) unsafe extern "C" fn raw_alpn_select( ssl: *mut ffi::SSL, out: *mut *const c_uchar, outlen: *mut c_uchar, @@ -172,54 +197,62 @@ pub extern "C" fn raw_alpn_select( where F: for<'a> Fn(&mut SslRef, &'a [u8]) -> Result<&'a [u8], AlpnError> + 'static + Sync + Send, { - unsafe { - let ssl = SslRef::from_ptr_mut(ssl); - let callback = ssl - .ssl_context() - .ex_data(SslContext::cached_ex_index::()) - .expect("BUG: alpn callback missing") as *const F; - let protos = slice::from_raw_parts(inbuf as *const u8, inlen as usize); + // SAFETY: boring provides valid inputs. + let ssl = unsafe { SslRef::from_ptr_mut(ssl) }; + let protos = unsafe { slice::from_raw_parts(inbuf as *const u8, inlen as usize) }; + let out = unsafe { &mut *out }; + let outlen = unsafe { &mut *outlen }; - match (*callback)(ssl, protos) { - Ok(proto) => { - *out = proto.as_ptr() as *const c_uchar; - *outlen = proto.len() as c_uchar; - ffi::SSL_TLSEXT_ERR_OK - } - Err(e) => e.0, + let ssl_context = ssl.ssl_context().to_owned(); + let callback = ssl_context + .ex_data(SslContext::cached_ex_index::()) + .expect("BUG: alpn callback missing"); + + match callback(ssl, protos) { + Ok(proto) => { + *out = proto.as_ptr() as *const c_uchar; + *outlen = proto.len() as c_uchar; + + ffi::SSL_TLSEXT_ERR_OK } + Err(e) => e.0, } } -pub unsafe extern "C" fn raw_select_cert( +pub(super) unsafe extern "C" fn raw_select_cert( client_hello: *const ffi::SSL_CLIENT_HELLO, ) -> ffi::ssl_select_cert_result_t where F: Fn(&ClientHello) -> Result<(), SelectCertError> + Sync + Send + 'static, { - let ssl = SslRef::from_ptr_mut((*client_hello).ssl); - let client_hello = &*(client_hello as *const ClientHello); - let callback = ssl + // SAFETY: boring provides valid inputs. + let client_hello = unsafe { &*(client_hello as *const ClientHello) }; + + let callback = client_hello + .ssl() .ssl_context() .ex_data(SslContext::cached_ex_index::()) - .expect("BUG: select cert callback missing") as *const F; + .expect("BUG: select cert callback missing"); - match (*callback)(client_hello) { + match callback(client_hello) { Ok(()) => ffi::ssl_select_cert_result_t::ssl_select_cert_success, Err(e) => e.0, } } -pub unsafe extern "C" fn raw_tlsext_status(ssl: *mut ffi::SSL, _: *mut c_void) -> c_int +pub(super) unsafe extern "C" fn raw_tlsext_status(ssl: *mut ffi::SSL, _: *mut c_void) -> c_int where F: Fn(&mut SslRef) -> Result + 'static + Sync + Send, { - let ssl = SslRef::from_ptr_mut(ssl); - let callback = ssl - .ssl_context() + // SAFETY: boring provides valid inputs. + let ssl = unsafe { SslRef::from_ptr_mut(ssl) }; + + let ssl_context = ssl.ssl_context().to_owned(); + let callback = ssl_context .ex_data(SslContext::cached_ex_index::()) - .expect("BUG: ocsp callback missing") as *const F; - let ret = (*callback)(ssl); + .expect("BUG: ocsp callback missing"); + + let ret = callback(ssl); if ssl.is_server() { match ret { @@ -242,45 +275,54 @@ where } } -pub unsafe extern "C" fn raw_new_session( +pub(super) unsafe extern "C" fn raw_new_session( ssl: *mut ffi::SSL, session: *mut ffi::SSL_SESSION, ) -> c_int where F: Fn(&mut SslRef, SslSession) + 'static + Sync + Send, { - let ssl = SslRef::from_ptr_mut(ssl); + // SAFETY: boring provides valid inputs. + let ssl = unsafe { SslRef::from_ptr_mut(ssl) }; + let session = unsafe { SslSession::from_ptr(session) }; + let callback = ssl .ex_data(*SESSION_CTX_INDEX) .expect("BUG: session context missing") .ex_data(SslContext::cached_ex_index::()) - .expect("BUG: new session callback missing") as *const F; - let session = SslSession::from_ptr(session); + .expect("BUG: new session callback missing"); - (*callback)(ssl, session); + // SAFETY: We can make `callback` outlive `ssl` because it is a callback + // stored in the session context set in `Ssl::new` so it is always + // guaranteed to outlive the lifetime of this function's scope. + let callback = unsafe { &*(callback as *const F) }; + + callback(ssl, session); // the return code doesn't indicate error vs success, but whether or not we consumed the session 1 } -pub unsafe extern "C" fn raw_remove_session( +pub(super) unsafe extern "C" fn raw_remove_session( ctx: *mut ffi::SSL_CTX, session: *mut ffi::SSL_SESSION, ) where F: Fn(&SslContextRef, &SslSessionRef) + 'static + Sync + Send, { - let ctx = SslContextRef::from_ptr(ctx); + // SAFETY: boring provides valid inputs. + let ctx = unsafe { SslContextRef::from_ptr(ctx) }; + let session = unsafe { SslSessionRef::from_ptr(session) }; + let callback = ctx .ex_data(SslContext::cached_ex_index::()) .expect("BUG: remove session callback missing"); - let session = SslSessionRef::from_ptr(session); callback(ctx, session) } type DataPtr = *const c_uchar; -pub unsafe extern "C" fn raw_get_session( +pub(super) unsafe extern "C" fn raw_get_session( ssl: *mut ffi::SSL, data: DataPtr, len: c_int, @@ -289,15 +331,23 @@ pub unsafe extern "C" fn raw_get_session( where F: Fn(&mut SslRef, &[u8]) -> Option + 'static + Sync + Send, { - let ssl = SslRef::from_ptr_mut(ssl); + // SAFETY: boring provides valid inputs. + let ssl = unsafe { SslRef::from_ptr_mut(ssl) }; + let data = unsafe { slice::from_raw_parts(data as *const u8, len as usize) }; + let copy = unsafe { &mut *copy }; + let callback = ssl .ex_data(*SESSION_CTX_INDEX) .expect("BUG: session context missing") .ex_data(SslContext::cached_ex_index::()) - .expect("BUG: get session callback missing") as *const F; - let data = slice::from_raw_parts(data as *const u8, len as usize); + .expect("BUG: get session callback missing"); - match (*callback)(ssl, data) { + // SAFETY: We can make `callback` outlive `ssl` because it is a callback + // stored in the session context set in `Ssl::new` so it is always + // guaranteed to outlive the lifetime of this function's scope. + let callback = unsafe { &*(callback as *const F) }; + + match callback(ssl, data) { Some(session) => { let p = session.as_ptr(); mem::forget(session); @@ -308,17 +358,18 @@ where } } -pub unsafe extern "C" fn raw_keylog(ssl: *const ffi::SSL, line: *const c_char) +pub(super) unsafe extern "C" fn raw_keylog(ssl: *const ffi::SSL, line: *const c_char) where F: Fn(&SslRef, &str) + 'static + Sync + Send, { - let ssl = SslRef::from_ptr(ssl as *mut _); + // SAFETY: boring provides valid inputs. + let ssl = unsafe { SslRef::from_ptr(ssl as *mut _) }; + let line = unsafe { str::from_utf8_unchecked(CStr::from_ptr(line).to_bytes()) }; + let callback = ssl .ssl_context() .ex_data(SslContext::cached_ex_index::()) .expect("BUG: get session callback missing"); - let line = CStr::from_ptr(line).to_bytes(); - let line = str::from_utf8_unchecked(line); callback(ssl, line); } diff --git a/boring/src/ssl/mod.rs b/boring/src/ssl/mod.rs index 810d5cf0..1d8129d4 100644 --- a/boring/src/ssl/mod.rs +++ b/boring/src/ssl/mod.rs @@ -854,10 +854,9 @@ impl SslContextBuilder { // context's ex data. Instead, pass the pointer directly as the servername arg. It's // still stored in ex data to manage the lifetime. let arg = self.set_ex_data_inner(SslContext::cached_ex_index::(), callback); - ffi::SSL_CTX_set_tlsext_servername_arg(self.as_ptr(), arg); - let f: extern "C" fn(_, _, _) -> _ = raw_sni::; - ffi::SSL_CTX_set_tlsext_servername_callback(self.as_ptr(), Some(f)); + ffi::SSL_CTX_set_tlsext_servername_arg(self.as_ptr(), arg); + ffi::SSL_CTX_set_tlsext_servername_callback(self.as_ptr(), Some(raw_sni::)); } } @@ -1457,7 +1456,6 @@ impl SslContextBuilder { /// This corresponds to [`SSL_CTX_set_psk_client_callback`]. /// /// [`SSL_CTX_set_psk_client_callback`]: https://www.openssl.org/docs/man1.0.2/ssl/SSL_CTX_set_psk_client_callback.html - #[cfg(not(osslconf = "OPENSSL_NO_PSK"))] pub fn set_psk_client_callback(&mut self, callback: F) where F: Fn(&mut SslRef, Option<&[u8]>, &mut [u8], &mut [u8]) -> Result @@ -1472,7 +1470,6 @@ impl SslContextBuilder { } #[deprecated(since = "0.10.10", note = "renamed to `set_psk_client_callback`")] - #[cfg(not(osslconf = "OPENSSL_NO_PSK"))] pub fn set_psk_callback(&mut self, callback: F) where F: Fn(&mut SslRef, Option<&[u8]>, &mut [u8], &mut [u8]) -> Result @@ -1492,7 +1489,6 @@ impl SslContextBuilder { /// This corresponds to [`SSL_CTX_set_psk_server_callback`]. /// /// [`SSL_CTX_set_psk_server_callback`]: https://www.openssl.org/docs/man1.0.2/ssl/SSL_CTX_set_psk_server_callback.html - #[cfg(not(osslconf = "OPENSSL_NO_PSK"))] pub fn set_psk_server_callback(&mut self, callback: F) where F: Fn(&mut SslRef, Option<&[u8]>, &mut [u8]) -> Result @@ -1733,6 +1729,14 @@ foreign_type_and_impl_send_sync! { impl Clone for SslContext { fn clone(&self) -> Self { + (**self).to_owned() + } +} + +impl ToOwned for SslContextRef { + type Owned = SslContext; + + fn to_owned(&self) -> Self::Owned { unsafe { SSL_CTX_up_ref(self.as_ptr()); SslContext::from_ptr(self.as_ptr()) diff --git a/boring/src/ssl/test/mod.rs b/boring/src/ssl/test/mod.rs index 0b83f6cb..e72dc086 100644 --- a/boring/src/ssl/test/mod.rs +++ b/boring/src/ssl/test/mod.rs @@ -1048,7 +1048,6 @@ fn _check_kinds() { is_sync::>(); } -#[cfg(not(osslconf = "OPENSSL_NO_PSK"))] #[test] fn psk_ciphers() { const CIPHER: &str = "PSK-AES128-CBC-SHA";