Skip to content

Commit

Permalink
Ensure session remove callback happens during SSL_CTX_free
Browse files Browse the repository at this point in the history
This is observable from openssl behaviour (even though it is
extremely unsound -- what happens if you `SSL_CTX_up_ref`
inside the callback? Instant UAF!)
  • Loading branch information
ctz committed Apr 26, 2024
1 parent 0589159 commit 40f4313
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 4 deletions.
24 changes: 24 additions & 0 deletions rustls-libssl/src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,11 @@ impl SessionCaches {
pub fn set_context(&mut self, context: &[u8]) {
self.server.set_context(context);
}

pub fn flush_all(&mut self) {
self.server.flush_all();
self.client.take();
}
}

impl Default for SessionCaches {
Expand Down Expand Up @@ -247,6 +252,25 @@ impl ServerSessionStorage {
None
}
}

fn flush_all(&self) {
if let Ok(mut items) = self.items.lock() {
let callbacks = self.callbacks();
if let Some(callback) = callbacks.remove_callback {
// if we have a callback to invoke, do it the slow way
while let Some(sess) = items.pop_first() {
callbacks::invoke_session_remove_callback(
Some(callback),
callbacks.ssl_ctx,
sess,
);
}
} else {
// otherwise, this is quicker.
items.clear();
}
}
}
}

#[derive(Debug)]
Expand Down
10 changes: 6 additions & 4 deletions rustls-libssl/src/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ use crate::error::{ffi_panic_boundary, Error, MysteriouslyOppositeReturnValue};
use crate::evp_pkey::EvpPkey;
use crate::ex_data::ExData;
use crate::ffi::{
clone_arc, free_arc, str_from_cstring, to_arc_mut_ptr, try_clone_arc, try_from,
try_mut_slice_int, try_ref_from_ptr, try_slice, try_slice_int, try_str, Castable, OwnershipArc,
OwnershipRef,
clone_arc, free_arc, free_arc_into_inner, str_from_cstring, to_arc_mut_ptr, try_clone_arc,
try_from, try_mut_slice_int, try_ref_from_ptr, try_slice, try_slice_int, try_str, Castable,
OwnershipArc, OwnershipRef,
};
use crate::not_thread_safe::NotThreadSafe;
use crate::x509::{load_certs, OwnedX509, OwnedX509Stack};
Expand Down Expand Up @@ -136,7 +136,9 @@ entry! {

entry! {
pub fn _SSL_CTX_free(ctx: *mut SSL_CTX) {
free_arc(ctx);
if let Some(inner) = free_arc_into_inner(ctx) {
inner.get_mut().flush_all_sessions();
}
}
}

Expand Down
15 changes: 15 additions & 0 deletions rustls-libssl/src/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,21 @@ where
drop(unsafe { Arc::from_raw(rs_typed) });
}

/// Similar to `free_arc`, but call `into_inner` on the Arc instead of just
/// dropping it.
///
/// This returns `Some` if this was the last reference.
pub(crate) fn free_arc_into_inner<C>(ptr: *const C) -> Option<C::RustType>
where
C: Castable<Ownership = OwnershipArc>,
{
if ptr.is_null() {
return None;
}
let rs_typed = cast_const_ptr(ptr);
Arc::into_inner(unsafe { Arc::from_raw(rs_typed) })
}

/// Convert a mutable pointer to a [`Castable`] to an optional `Box` over the underlying
/// [`Castable::RustType`], and immediately let it fall out of scope to be freed.
///
Expand Down
4 changes: 4 additions & 0 deletions rustls-libssl/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,10 @@ impl SslContext {
self.caches.set_timeout(timeout)
}

fn flush_all_sessions(&mut self) {
self.caches.flush_all();
}

fn set_max_early_data(&mut self, max: u32) {
self.max_early_data = max;
}
Expand Down

0 comments on commit 40f4313

Please sign in to comment.