From c5ef5a62fdfa7f33ada70414e0ea6b81ff053e5c Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Mon, 13 May 2024 16:13:45 +0100 Subject: [PATCH] Use `X509_STORE` for root certificate storage In {client,server}.c, see that the effects of `SSL_CTX_load_verify_file` can be observed via `SSL_CTX_get_cert_store` --- rustls-libssl/src/lib.rs | 35 ++++++++---------- rustls-libssl/src/miri.rs | 10 ++++++ rustls-libssl/src/verifier.rs | 17 +++++---- rustls-libssl/src/x509.rs | 67 ++++++++++++++++++++++++++++++++++- rustls-libssl/tests/client.c | 4 +++ rustls-libssl/tests/server.c | 4 +++ 6 files changed, 108 insertions(+), 29 deletions(-) diff --git a/rustls-libssl/src/lib.rs b/rustls-libssl/src/lib.rs index 041e573..a92a56d 100644 --- a/rustls-libssl/src/lib.rs +++ b/rustls-libssl/src/lib.rs @@ -17,7 +17,7 @@ use rustls::pki_types::{CertificateDer, ServerName}; use rustls::server::{Accepted, Acceptor}; use rustls::{ CipherSuite, ClientConfig, ClientConnection, Connection, HandshakeKind, ProtocolVersion, - RootCertStore, ServerConfig, SignatureScheme, SupportedProtocolVersion, + ServerConfig, SignatureScheme, SupportedProtocolVersion, }; use not_thread_safe::NotThreadSafe; @@ -413,7 +413,6 @@ pub struct SslContext { raw_options: u64, verify_mode: VerifyMode, verify_depth: c_int, - verify_roots: RootCertStore, verify_x509_store: x509::OwnedX509Store, alpn: Vec>, default_cert_file: Option, @@ -435,7 +434,6 @@ impl SslContext { raw_options: 0, verify_mode: VerifyMode::default(), verify_depth: -1, - verify_roots: RootCertStore::empty(), verify_x509_store: OwnedX509Store::default(), alpn: vec![], default_cert_file: None, @@ -604,12 +602,7 @@ impl SslContext { &mut self, certs: Vec>, ) -> Result<(), error::Error> { - for c in certs { - self.verify_roots - .add(c) - .map_err(error::Error::from_rustls)?; - } - Ok(()) + self.verify_x509_store.add(certs) } fn get_x509_store(&self) -> *mut X509_STORE { @@ -714,8 +707,8 @@ struct Ssl { mode: ConnMode, verify_mode: VerifyMode, verify_depth: c_int, - verify_roots: RootCertStore, verify_server_name: Option>, + verify_x509_store: x509::OwnedX509Store, alpn: Vec>, alpn_callback: callbacks::AlpnCallbackConfig, cert_callback: callbacks::CertCallbackConfig, @@ -754,8 +747,8 @@ impl Ssl { mode: inner.method.mode(), verify_mode: inner.verify_mode, verify_depth: inner.verify_depth, - verify_roots: Self::load_verify_certs(inner)?, verify_server_name: None, + verify_x509_store: Self::load_verify_certs(inner)?, alpn: inner.alpn.clone(), alpn_callback: inner.alpn_callback.clone(), cert_callback: inner.cert_callback.clone(), @@ -989,7 +982,7 @@ impl Ssl { let provider = Arc::new(provider::default_provider()); let verifier = Arc::new(verifier::ServerVerifier::new( - self.verify_roots.clone().into(), + self.verify_x509_store.clone(), provider.clone(), self.verify_mode, &self.verify_server_name, @@ -1074,7 +1067,7 @@ impl Ssl { let provider = Arc::new(provider::default_provider()); let verifier = Arc::new( verifier::ClientVerifier::new( - self.verify_roots.clone().into(), + self.verify_x509_store.clone(), provider.clone(), self.verify_mode, ) @@ -1377,20 +1370,20 @@ impl Ssl { } } - fn load_verify_certs(ctx: &SslContext) -> Result { - let mut verify_roots = ctx.verify_roots.clone(); - + fn load_verify_certs(ctx: &SslContext) -> Result { // If verify_roots isn't empty then it was configured with `SSL_CTX_load_verify_file` // or `SSL_CTX_load_verify_dir` and we should use it as-is. - if !ctx.verify_roots.is_empty() { - return Ok(verify_roots); + if !ctx.verify_x509_store.is_empty() { + return Ok(ctx.verify_x509_store.clone()); } // Otherwise, try to load the default cert file or cert dir. + let mut verify_roots = x509::OwnedX509Store::default(); + if let Some(default_cert_file) = &ctx.default_cert_file { - verify_roots.add_parsable_certificates(x509::load_certs( + verify_roots.add(x509::load_certs( vec![default_cert_file.to_path_buf()].into_iter(), - )?); + )?)?; } else if let Some(default_cert_dir) = &ctx.default_cert_dir { let entries = match fs::read_dir(default_cert_dir) { Ok(iter) => iter, @@ -1399,7 +1392,7 @@ impl Ssl { .filter_map(|entry| entry.ok()) .map(|dir_entry| dir_entry.path()); - verify_roots.add_parsable_certificates(x509::load_certs(entries)?); + verify_roots.add(x509::load_certs(entries)?)?; } Ok(verify_roots) diff --git a/rustls-libssl/src/miri.rs b/rustls-libssl/src/miri.rs index b6dcece..c827c9d 100644 --- a/rustls-libssl/src/miri.rs +++ b/rustls-libssl/src/miri.rs @@ -9,6 +9,16 @@ pub extern "C" fn X509_STORE_new() -> *mut X509_STORE { Box::into_raw(Box::new(X509_STORE(()))) } +#[no_mangle] +pub extern "C" fn X509_STORE_get0_objects(s: *mut X509_STORE) -> *mut c_void { + ptr::null_mut() +} + +#[no_mangle] +pub extern "C" fn OPENSSL_sk_num(sk: *mut c_void) -> c_int { + 0 +} + #[no_mangle] pub extern "C" fn X509_STORE_free(ptr: *mut X509_STORE) { if ptr.is_null() { diff --git a/rustls-libssl/src/verifier.rs b/rustls-libssl/src/verifier.rs index 641d583..45a9249 100644 --- a/rustls-libssl/src/verifier.rs +++ b/rustls-libssl/src/verifier.rs @@ -16,10 +16,10 @@ use rustls::{ pki_types::{CertificateDer, ServerName, UnixTime}, server::danger::{ClientCertVerified, ClientCertVerifier}, server::{ParsedCertificate, WebPkiClientVerifier}, - CertificateError, DigitallySignedStruct, DistinguishedName, Error, RootCertStore, - SignatureScheme, + CertificateError, DigitallySignedStruct, DistinguishedName, Error, SignatureScheme, }; +use crate::x509::OwnedX509Store; use crate::VerifyMode; /// This is a verifier that implements the selection of bad ideas from OpenSSL: @@ -29,7 +29,7 @@ use crate::VerifyMode; /// - that the behaviour defaults to verifying nothing #[derive(Debug)] pub struct ServerVerifier { - root_store: Arc, + x509_store: OwnedX509Store, provider: Arc, @@ -47,13 +47,13 @@ pub struct ServerVerifier { impl ServerVerifier { pub fn new( - root_store: Arc, + x509_store: OwnedX509Store, provider: Arc, mode: VerifyMode, hostname: &Option>, ) -> Self { Self { - root_store, + x509_store, provider, verify_hostname: hostname.clone(), mode, @@ -77,10 +77,11 @@ impl ServerVerifier { now: UnixTime, ) -> Result<(), Error> { let end_entity = ParsedCertificate::try_from(end_entity)?; + let root_store = self.x509_store.root_store()?; verify_server_cert_signed_by_trust_anchor( &end_entity, - &self.root_store, + &root_store, intermediates, now, self.provider.signature_verification_algorithms.all, @@ -169,10 +170,12 @@ pub struct ClientVerifier { impl ClientVerifier { pub fn new( - root_store: Arc, + x509_store: OwnedX509Store, provider: Arc, mode: VerifyMode, ) -> Result { + let root_store = x509_store.root_store()?; + let (parent, initial_result) = if !mode.server_must_attempt_client_auth() { (Ok(WebPkiClientVerifier::no_client_auth()), X509_V_OK) } else { diff --git a/rustls-libssl/src/x509.rs b/rustls-libssl/src/x509.rs index 3e48aa2..c832b44 100644 --- a/rustls-libssl/src/x509.rs +++ b/rustls-libssl/src/x509.rs @@ -1,14 +1,17 @@ use core::ffi::{c_int, c_long, c_void}; use core::{ptr, slice}; use std::path::PathBuf; +use std::sync::Arc; use std::{fs, io}; use openssl_sys::{ d2i_X509, i2d_X509, stack_st_X509, OPENSSL_free, OPENSSL_sk_new_null, OPENSSL_sk_num, - OPENSSL_sk_push, OPENSSL_sk_value, X509_STORE_free, X509_STORE_new, X509_free, OPENSSL_STACK, + OPENSSL_sk_push, OPENSSL_sk_value, X509_STORE_add_cert, X509_STORE_free, + X509_STORE_get0_objects, X509_STORE_get1_all_certs, X509_STORE_new, X509_free, OPENSSL_STACK, X509, X509_STORE, }; use rustls::pki_types::CertificateDer; +use rustls::RootCertStore; use crate::error::Error; @@ -232,6 +235,7 @@ impl Drop for OwnedX509 { } } +#[derive(Debug)] pub struct OwnedX509Store { raw: *mut X509_STORE, } @@ -245,6 +249,54 @@ impl OwnedX509Store { pub fn pointer(&self) -> *mut X509_STORE { self.raw } + + pub fn len(&self) -> usize { + match unsafe { OPENSSL_sk_num(X509_STORE_get0_objects(self.raw) as *const OPENSSL_STACK) } { + -1 | 0 => 0, + i => i as usize, + } + } + + pub fn is_empty(&self) -> bool { + self.len() == 0 + } + + pub fn add( + &mut self, + cert_ders: impl IntoIterator>, + ) -> Result<(), Error> { + for cert_der in cert_ders { + let item = OwnedX509::parse_der(cert_der.as_ref()) + .ok_or_else(|| Error::bad_data("cannot parse certificate"))?; + match unsafe { X509_STORE_add_cert(self.raw, item.borrow_ref()) } { + 1 => {} + _ => { + return Err(Error::bad_data("cannot X509_STORE_add_cert")); + } + } + } + + Ok(()) + } + + pub fn root_store(&self) -> Result, rustls::Error> { + let ptr = unsafe { X509_STORE_get1_all_certs(self.raw) }; + + if ptr.is_null() { + return Err(rustls::Error::General( + "cannot retrieve trusted certs".to_string(), + )); + } + + let certs = OwnedX509Stack::new(ptr); + let mut ret = RootCertStore::empty(); + + for i in 0..certs.len() { + ret.add(certs.item(i).der_bytes().into())?; + } + + Ok(ret.into()) + } } impl Default for OwnedX509Store { @@ -255,6 +307,15 @@ impl Default for OwnedX509Store { } } +impl Clone for OwnedX509Store { + fn clone(&self) -> Self { + unsafe { + X509_STORE_up_ref(self.raw); + } + Self { raw: self.raw } + } +} + impl Drop for OwnedX509Store { fn drop(&mut self) { unsafe { @@ -263,6 +324,9 @@ impl Drop for OwnedX509Store { } } +unsafe impl Send for OwnedX509Store {} +unsafe impl Sync for OwnedX509Store {} + pub(crate) fn load_certs<'a>( file_names: impl Iterator, ) -> Result>, Error> { @@ -294,4 +358,5 @@ extern "C" { ); fn OPENSSL_sk_dup(st: *const OPENSSL_STACK) -> *mut OPENSSL_STACK; fn X509_up_ref(x: *mut X509) -> c_int; + fn X509_STORE_up_ref(xs: *mut X509_STORE) -> c_int; } diff --git a/rustls-libssl/tests/client.c b/rustls-libssl/tests/client.c index 4026904..c48529f 100644 --- a/rustls-libssl/tests/client.c +++ b/rustls-libssl/tests/client.c @@ -56,8 +56,12 @@ int main(int argc, char **argv) { dump_openssl_error_stack(); assert(SSL_CTX_get_verify_mode(ctx) == SSL_VERIFY_PEER); assert(SSL_CTX_get_verify_callback(ctx) == NULL); + TRACE(sk_X509_OBJECT_num( + X509_STORE_get0_objects(SSL_CTX_get_cert_store(ctx)))); TRACE(SSL_CTX_load_verify_file(ctx, cacert)); dump_openssl_error_stack(); + TRACE(sk_X509_OBJECT_num( + X509_STORE_get0_objects(SSL_CTX_get_cert_store(ctx)))); } printf("SSL_CTX_get_verify_depth default %d\n", SSL_CTX_get_verify_depth(ctx)); diff --git a/rustls-libssl/tests/server.c b/rustls-libssl/tests/server.c index e3d7b5e..40db94a 100644 --- a/rustls-libssl/tests/server.c +++ b/rustls-libssl/tests/server.c @@ -128,8 +128,12 @@ int main(int argc, char **argv) { if (strcmp(cacert, "unauth") != 0) { SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, NULL); dump_openssl_error_stack(); + TRACE(sk_X509_OBJECT_num( + X509_STORE_get0_objects(SSL_CTX_get_cert_store(ctx)))); TRACE(SSL_CTX_load_verify_file(ctx, cacert)); dump_openssl_error_stack(); + TRACE(sk_X509_OBJECT_num( + X509_STORE_get0_objects(SSL_CTX_get_cert_store(ctx)))); } else { printf("client auth disabled\n"); }