Clean up and remove some unsafe code from ffi callbacks

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<F>(preverify_ok: c_int, x509_ctx: *mut ffi::X509_STORE_CTX) -> c_int
+pub(super) unsafe extern "C" fn raw_verify<F>(
+    preverify_ok: c_int,
+    x509_ctx: *mut ffi::X509_STORE_CTX,
+) -> c_int
 where
     F: Fn(bool, &mut X509StoreContextRef) -> bool + 'static + Sync + Send,
 {
-    unsafe {
+    // SAFETY: boring provides valid inputs.
-        let ctx = X509StoreContextRef::from_ptr_mut(x509_ctx);
+    let ctx = unsafe { 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::<F>();
 
-        // 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;
+        .expect("BUG: verify callback missing");
 
-        (*verify)(preverify_ok != 0, ctx) as c_int
+    // 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(super) unsafe extern "C" fn raw_client_psk<F>(
-pub extern "C" fn raw_client_psk<F>(
+    ssl_ptr: *mut ffi::SSL,
-    ssl: *mut ffi::SSL,
     hint: *const c_char,
     identity: *mut c_char,
     max_identity_len: c_uint,
@@ -56,34 +62,36 @@ where
         + Sync
         + Send,
 {
-    unsafe {
+    // SAFETY: boring provides valid inputs.
-        let ssl = SslRef::from_ptr_mut(ssl);
+
-        let callback_idx = SslContext::cached_ex_index::<F>();
+    let ssl = unsafe { SslRef::from_ptr_mut(ssl_ptr) };
 
-        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())
+        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 = slice::from_raw_parts_mut(identity as *mut u8, max_identity_len as usize);
+    let identity_sl =
-        let psk_sl = slice::from_raw_parts_mut(psk as *mut u8, max_psk_len as usize);
+        unsafe { slice::from_raw_parts_mut(identity as *mut u8, max_identity_len as usize) };
-        match (*callback)(ssl, hint, identity_sl, psk_sl) {
+    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::<F>())
+        .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(super) unsafe extern "C" fn raw_server_psk<F>(
-pub extern "C" fn raw_server_psk<F>(
     ssl: *mut ffi::SSL,
     identity: *const c_char,
     psk: *mut c_uchar,
@@ -95,73 +103,90 @@ where
         + Sync
         + Send,
 {
-    unsafe {
+    // SAFETY: boring provides valid inputs.
-        let ssl = SslRef::from_ptr_mut(ssl);
-        let callback_idx = SslContext::cached_ex_index::<F>();
 
-        let callback = ssl
+    let ssl = unsafe { SslRef::from_ptr_mut(ssl) };
-            .ssl_context()
+
-            .ex_data(callback_idx)
+    let identity = if !identity.is_null() {
-            .expect("BUG: psk callback missing") as *const F;
+        Some(unsafe { CStr::from_ptr(identity) }.to_bytes())
-        let identity = if identity.is_null() {
-            None
     } else {
-            Some(CStr::from_ptr(identity).to_bytes())
+        None
     };
+
     // 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);
+    let psk_sl = unsafe { slice::from_raw_parts_mut(psk as *mut u8, max_psk_len as usize) };
-        match (*callback)(ssl, identity, psk_sl) {
+
+    let ssl_context = ssl.ssl_context().to_owned();
+    let callback = ssl_context
+        .ex_data(SslContext::cached_ex_index::<F>())
+        .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<F>(
+pub(super) unsafe extern "C" fn ssl_raw_verify<F>(
     preverify_ok: c_int,
     x509_ctx: *mut ffi::X509_STORE_CTX,
 ) -> c_int
 where
     F: Fn(bool, &mut X509StoreContextRef) -> bool + 'static + Sync + Send,
 {
-    unsafe {
+    // SAFETY: boring provides valid inputs.
-        let ctx = X509StoreContextRef::from_ptr_mut(x509_ctx);
+    let ctx = unsafe { 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::<Arc<F>>();
 
+    let ssl_idx = X509StoreContext::ssl_idx().expect("BUG: store context ssl index missing");
+
+    // NOTE(nox): I'm pretty sure this Arc<F> 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(callback_idx)
+        .ex_data(Ssl::cached_ex_index::<Arc<F>>())
         .expect("BUG: ssl verify callback missing")
         .clone();
 
     callback(preverify_ok != 0, ctx) as c_int
-    }
 }
 
-pub extern "C" fn raw_sni<F>(ssl: *mut ffi::SSL, al: *mut c_int, arg: *mut c_void) -> c_int
+pub(super) unsafe extern "C" fn raw_sni<F>(
+    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 {
+    // SAFETY: boring provides valid inputs.
-        let ssl = SslRef::from_ptr_mut(ssl);
+    let ssl = unsafe { SslRef::from_ptr_mut(ssl) };
-        let callback = arg as *const F;
+    let al = unsafe { &mut *al };
+
+    // 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);
+    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<F>(
+pub(super) unsafe extern "C" fn raw_alpn_select<F>(
     ssl: *mut ffi::SSL,
     out: *mut *const c_uchar,
     outlen: *mut c_uchar,
@@ -172,54 +197,62 @@ pub extern "C" fn raw_alpn_select<F>(
 where
     F: for<'a> Fn(&mut SslRef, &'a [u8]) -> Result<&'a [u8], AlpnError> + 'static + Sync + Send,
 {
-    unsafe {
+    // SAFETY: boring provides valid inputs.
-        let ssl = SslRef::from_ptr_mut(ssl);
+    let ssl = unsafe { SslRef::from_ptr_mut(ssl) };
-        let callback = ssl
+    let protos = unsafe { slice::from_raw_parts(inbuf as *const u8, inlen as usize) };
-            .ssl_context()
+    let out = unsafe { &mut *out };
-            .ex_data(SslContext::cached_ex_index::<F>())
+    let outlen = unsafe { &mut *outlen };
-            .expect("BUG: alpn callback missing") as *const F;
-        let protos = slice::from_raw_parts(inbuf as *const u8, inlen as usize);
 
-        match (*callback)(ssl, protos) {
+    let ssl_context = ssl.ssl_context().to_owned();
+    let callback = ssl_context
+        .ex_data(SslContext::cached_ex_index::<F>())
+        .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<F>(
+pub(super) unsafe extern "C" fn raw_select_cert<F>(
     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);
+    // SAFETY: boring provides valid inputs.
-    let client_hello = &*(client_hello as *const ClientHello);
+    let client_hello = unsafe { &*(client_hello as *const ClientHello) };
-    let callback = ssl
+
+    let callback = client_hello
+        .ssl()
         .ssl_context()
         .ex_data(SslContext::cached_ex_index::<F>())
-        .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<F>(ssl: *mut ffi::SSL, _: *mut c_void) -> c_int
+pub(super) unsafe extern "C" fn raw_tlsext_status<F>(ssl: *mut ffi::SSL, _: *mut c_void) -> c_int
 where
     F: Fn(&mut SslRef) -> Result<bool, ErrorStack> + 'static + Sync + Send,
 {
-    let ssl = SslRef::from_ptr_mut(ssl);
+    // SAFETY: boring provides valid inputs.
-    let callback = ssl
+    let ssl = unsafe { SslRef::from_ptr_mut(ssl) };
-        .ssl_context()
+
+    let ssl_context = ssl.ssl_context().to_owned();
+    let callback = ssl_context
         .ex_data(SslContext::cached_ex_index::<F>())
-        .expect("BUG: ocsp callback missing") as *const F;
+        .expect("BUG: ocsp callback missing");
-    let ret = (*callback)(ssl);
+
+    let ret = callback(ssl);
 
     if ssl.is_server() {
         match ret {
@@ -242,45 +275,54 @@ where
     }
 }
 
-pub unsafe extern "C" fn raw_new_session<F>(
+pub(super) unsafe extern "C" fn raw_new_session<F>(
     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::<F>())
-        .expect("BUG: new session callback missing") as *const F;
+        .expect("BUG: new session callback missing");
-    let session = SslSession::from_ptr(session);
 
-    (*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<F>(
+pub(super) unsafe extern "C" fn raw_remove_session<F>(
     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::<F>())
         .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<F>(
+pub(super) unsafe extern "C" fn raw_get_session<F>(
     ssl: *mut ffi::SSL,
     data: DataPtr,
     len: c_int,
@@ -289,15 +331,23 @@ pub unsafe extern "C" fn raw_get_session<F>(
 where
     F: Fn(&mut SslRef, &[u8]) -> Option<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 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::<F>())
-        .expect("BUG: get session callback missing") as *const F;
+        .expect("BUG: get session callback missing");
-    let data = slice::from_raw_parts(data as *const u8, len as usize);
 
-    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<F>(ssl: *const ffi::SSL, line: *const c_char)
+pub(super) unsafe extern "C" fn raw_keylog<F>(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::<F>())
         .expect("BUG: get session callback missing");
-    let line = CStr::from_ptr(line).to_bytes();
-    let line = str::from_utf8_unchecked(line);
 
     callback(ssl, line);
 }

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::<F>(), callback);
-            ffi::SSL_CTX_set_tlsext_servername_arg(self.as_ptr(), arg);
 
-            let f: extern "C" fn(_, _, _) -> _ = raw_sni::<F>;
+            ffi::SSL_CTX_set_tlsext_servername_arg(self.as_ptr(), arg);
-            ffi::SSL_CTX_set_tlsext_servername_callback(self.as_ptr(), Some(f));
+            ffi::SSL_CTX_set_tlsext_servername_callback(self.as_ptr(), Some(raw_sni::<F>));
         }
     }
 
@@ -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<F>(&mut self, callback: F)
     where
         F: Fn(&mut SslRef, Option<&[u8]>, &mut [u8], &mut [u8]) -> Result<usize, ErrorStack>
@@ -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<F>(&mut self, callback: F)
     where
         F: Fn(&mut SslRef, Option<&[u8]>, &mut [u8], &mut [u8]) -> Result<usize, ErrorStack>
@@ -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<F>(&mut self, callback: F)
     where
         F: Fn(&mut SslRef, Option<&[u8]>, &mut [u8]) -> Result<usize, ErrorStack>
@@ -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())

boring/src/ssl/test/mod.rs

@@ -1048,7 +1048,6 @@ fn _check_kinds() {
     is_sync::<SslStream<TcpStream>>();
 }
 
-#[cfg(not(osslconf = "OPENSSL_NO_PSK"))]
 #[test]
 fn psk_ciphers() {
     const CIPHER: &str = "PSK-AES128-CBC-SHA";