rust: bssl-tls: Disallow downcasting in-handshake to normal handle A big downside in doing so is that it accidentally allows users to trigger progression in TLS state machine during the handshake. This is possible because we supply various callbacks with the in-handshake handle and through this existing downcasting the callbacks can call read and write again, which is a re-entry into our handshake code. Update-Note: a breaking change is introduced to further restrict the `in_handshake()` handle to the connection, so that one cannot possibly advance the TLS state machine while a read or write into the TLS socket is in progress. This was possible through callbacks that can be invoked while reading or writing on the socket. The necessary changes can be found in cl/934247882. Signed-off-by: Xiangfei Ding <xfding@google.com> Change-Id: I3212b0184a3ebda606812217a4e350396a6a6964 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/97667 Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/rust/bssl-tls-tokio/src/lib.rs b/rust/bssl-tls-tokio/src/lib.rs index a906309..87c345e 100644 --- a/rust/bssl-tls-tokio/src/lib.rs +++ b/rust/bssl-tls-tokio/src/lib.rs
@@ -77,7 +77,7 @@ //! # let (stream, _) = listener.accept().await.unwrap(); //! # let mut conn = server_ctx.new_server_connection(None).unwrap().build(); //! # conn.set_io(TokioIo(stream)).unwrap(); -//! # conn.in_handshake().unwrap().async_handshake().await.unwrap(); +//! # conn.async_handshake().await.unwrap(); //! # //! # let mut tls_stream = TokioTlsConnection::new(conn); //! # let mut buf = [0u8; 1024];
diff --git a/rust/bssl-tls-tokio/tests/datagram.rs b/rust/bssl-tls-tokio/tests/datagram.rs index f8dec3d..19a6c95 100644 --- a/rust/bssl-tls-tokio/tests/datagram.rs +++ b/rust/bssl-tls-tokio/tests/datagram.rs
@@ -88,11 +88,7 @@ use std::time::Duration; let task = tokio::spawn(async move { - server_conn - .in_handshake() - .unwrap() - .async_handshake() - .await?; + server_conn.async_handshake().await?; let mut message = [0; 21]; let mut read_bytes = 0; @@ -117,11 +113,7 @@ Ok::<_, Error>(()) }); - client_conn - .in_handshake() - .unwrap() - .async_handshake() - .await?; + client_conn.async_handshake().await?; client_conn .as_pin_mut() .async_write(b"BoringSSL is awesome!")
diff --git a/rust/bssl-tls/src/connection.rs b/rust/bssl-tls/src/connection.rs index 03347b2..54ae965 100644 --- a/rust/bssl-tls/src/connection.rs +++ b/rust/bssl-tls/src/connection.rs
@@ -147,8 +147,8 @@ /// TLS Connection /// -/// `Role` is expected to be either [`Server`] or [`Client`] and -/// `Mode` is expected to be either [`TlsMode`] or [`QuicMode`]. +/// `Role` is expected to be either [`crate::context::Server`] or [`crate::context::Client`] and +/// `Mode` is expected to be either [`crate::context::TlsMode`] or [`crate::context::QuicMode`]. /// These generics will govern the capabilities that respective TLS connection role can access, // NOTE: any method that involves I/O must require exclusive access, enforced by requiring `&mut`. #[repr(transparent)]
diff --git a/rust/bssl-tls/src/connection/credentials.rs b/rust/bssl-tls/src/connection/credentials.rs index 11b16e5..0d10cfa 100644 --- a/rust/bssl-tls/src/connection/credentials.rs +++ b/rust/bssl-tls/src/connection/credentials.rs
@@ -86,7 +86,7 @@ &mut self, key_method: Option<Box<dyn PrivateKeyDelegate>>, ) -> &mut Self { - let ctx = self.ptr(); + let ctx = self.0.ptr(); if key_method.is_some() { unsafe { // Safety: we only install our own vtable. @@ -98,7 +98,7 @@ bssl_sys::SSL_set_private_key_method(ctx, core::ptr::null()); } } - self.get_connection_methods().private_key_delegate = key_method; + self.0.get_connection_methods().private_key_delegate = key_method; self } } @@ -185,7 +185,7 @@ &mut self, mode: CertificateVerificationMode, ) -> &mut Self { - let ctx = self.ptr(); + let ctx = self.0.ptr(); unsafe { // Safety: this method only updates the mode value. bssl_sys::SSL_set_verify(ctx, mode as _, None); @@ -202,7 +202,7 @@ where V: VerifyCertificate + 'static, { - let ctx = self.ptr(); + let ctx = self.0.ptr(); unsafe { // Safety: we only install our own vtable. bssl_sys::SSL_set_custom_verify( @@ -211,18 +211,18 @@ Some(cert_cb::<super::methods::RustConnectionMethods<M>>), ); } - self.get_connection_methods().verify_certificate_methods = Some(Box::new(verifier) as _); + self.0.get_connection_methods().verify_certificate_methods = Some(Box::new(verifier) as _); self } /// Remove custom certificate verifier. pub fn remove_certificate_verifier(&mut self, mode: CertificateVerificationMode) -> &mut Self { - let ctx = self.ptr(); + let ctx = self.0.ptr(); unsafe { // Safety: we only uninstall the vtable. bssl_sys::SSL_set_custom_verify(ctx, mode as _, None); } - self.get_connection_methods().verify_certificate_methods = None; + self.0.get_connection_methods().verify_certificate_methods = None; self } @@ -230,7 +230,7 @@ pub fn get_certificate_verification_mode(&self) -> Option<CertificateVerificationMode> { unsafe { // Safety: the validity of the handle `self.0` is witnessed by `self`. - bssl_sys::SSL_get_verify_mode(self.ptr()) + bssl_sys::SSL_get_verify_mode(self.0.ptr()) } .try_into() .ok() @@ -249,7 +249,7 @@ pub fn add_credential(&mut self, credential: &TlsCredential) -> Result<&mut Self, Error> { check_lib_error!(unsafe { // Safety: `credential` is still valid. - bssl_sys::SSL_add1_credential(self.ptr(), credential.ptr()) + bssl_sys::SSL_add1_credential(self.0.ptr(), credential.ptr()) }); Ok(self) } @@ -258,7 +258,7 @@ pub fn clear_credentials(&mut self) -> &mut Self { unsafe { // Safety: `credential` is still valid. - bssl_sys::SSL_certs_clear(self.ptr()); + bssl_sys::SSL_certs_clear(self.0.ptr()); } self } @@ -275,7 +275,7 @@ pub fn enable_signed_certificate_timestamps(&mut self) -> &mut Self { unsafe { // Safety: the validity of the handle `self.0` is witnessed by `self`. - bssl_sys::SSL_enable_signed_cert_timestamps(self.ptr()); + bssl_sys::SSL_enable_signed_cert_timestamps(self.0.ptr()); } self } @@ -290,7 +290,7 @@ // - the validity of the handle `self.0` is witnessed by `self`. // - when pending handshake, the assignment is always successful. // - `SSL_set1_verify_cert_store` bumps the ref-count on the store. - bssl_sys::SSL_set1_verify_cert_store(self.ptr(), store.as_mut_ptr()); + bssl_sys::SSL_set1_verify_cert_store(self.0.ptr(), store.as_mut_ptr()); } self } @@ -314,7 +314,7 @@ let (prefs, prefs_len) = slice_into_ffi_raw_parts(algs); check_lib_error!(unsafe { // Safety: the validity of the handle `self.0` is witnessed by `self`. - bssl_sys::SSL_set_verify_algorithm_prefs(self.ptr(), prefs, prefs_len) + bssl_sys::SSL_set_verify_algorithm_prefs(self.0.ptr(), prefs, prefs_len) }); Ok(self) } @@ -330,7 +330,7 @@ // Safety: // - the validity of the handle `self.0` is witnessed by `self`. // - the host name string has been sanitised for internal NUL-bytes and NUL-terminated. - bssl_sys::SSL_set1_host(self.ptr(), host_name.as_ptr()) + bssl_sys::SSL_set1_host(self.0.ptr(), host_name.as_ptr()) }); Ok(self) } @@ -345,7 +345,7 @@ .map_err(|_| Error::Configuration(ConfigurationError::ValueOutOfRange))?; unsafe { // Safety: the validity of the handle `self.0` is witnessed by `self`. - bssl_sys::SSL_set_verify_depth(self.ptr(), depth); + bssl_sys::SSL_set_verify_depth(self.0.ptr(), depth); } Ok(self) } @@ -356,7 +356,7 @@ pub fn get_verify_depth(&self) -> Option<u16> { unsafe { // Safety: the validity and state of the handle `self.0` is witnessed by `self`. - bssl_sys::SSL_get_verify_depth(self.ptr()).try_into().ok() + bssl_sys::SSL_get_verify_depth(self.0.ptr()).try_into().ok() } } @@ -369,7 +369,7 @@ // Safety: // - the validity of the handle `self.0` is witnessed by `self`. // - `SSL_set1_param` claims shared ownership of `params` by bumping ref-count. - bssl_sys::SSL_set1_param(self.ptr(), params.as_ptr()) + bssl_sys::SSL_set1_param(self.0.ptr(), params.as_ptr()) }); Ok(self) } @@ -379,7 +379,7 @@ impl<R, M> TlsConnectionInHandshake<'_, R, M> { /// Disable session creation. pub fn disable_session(&mut self) -> &mut Self { - let ptr = self.ptr(); + let ptr = self.0.ptr(); unsafe { // Safety: the validity of the handle `ptr` is witnessed by `self`. bssl_sys::SSL_set_mode(ptr, super::ConnectionMode::MODE_NO_SESSION_CREATION.bits()); @@ -391,7 +391,7 @@ pub fn set_session(&mut self, session: &crate::sessions::TlsSession) -> &mut Self { unsafe { // Safety: self.ptr and session.0 are valid. - bssl_sys::SSL_set_session(self.ptr.as_ptr(), session.0.as_ptr()); + bssl_sys::SSL_set_session(self.0.ptr.as_ptr(), session.0.as_ptr()); } self } @@ -409,7 +409,7 @@ // Safety: // - `self.ptr()` is a valid handle. // - `CertificateType` is a `u8` with acceptable values by construction. - bssl_sys::SSL_set1_accepted_peer_cert_types(self.ptr(), types as *const _, types_len) + bssl_sys::SSL_set1_accepted_peer_cert_types(self.0.ptr(), types as *const _, types_len) }); Ok(self) }
diff --git a/rust/bssl-tls/src/connection/lifecycle.rs b/rust/bssl-tls/src/connection/lifecycle.rs index 032cec2..42aef37 100644 --- a/rust/bssl-tls/src/connection/lifecycle.rs +++ b/rust/bssl-tls/src/connection/lifecycle.rs
@@ -170,24 +170,13 @@ } /// A handle to the connection that is valid only during handshake. +// NOTE(@xfding): this type is strictly for configuration of the connection during the handshake, +// and no methods should be allowed to drive the TLS state machine. #[repr(transparent)] pub struct TlsConnectionInHandshake<'a, R, M>(pub(crate) &'a mut TlsConnection<R, M>); -impl<R, M> Deref for TlsConnectionInHandshake<'_, R, M> { - type Target = TlsConnection<R, M>; - fn deref(&self) -> &Self::Target { - &*self.0 - } -} - -impl<R, M> DerefMut for TlsConnectionInHandshake<'_, R, M> { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut *self.0 - } -} - /// # Handshake -impl<R, M> TlsConnectionInHandshake<'_, R, M> +impl<R, M> TlsConnection<R, M> where M: HasTlsConnectionMethod, { @@ -205,7 +194,7 @@ } } -impl<M> TlsConnectionInHandshake<'_, Server, M> +impl<M> TlsConnection<Server, M> where M: HasTlsConnectionMethod, { @@ -219,7 +208,7 @@ } } -impl<M> TlsConnectionInHandshake<'_, Client, M> +impl<M> TlsConnection<Client, M> where M: HasTlsConnectionMethod, { @@ -317,14 +306,14 @@ } } -impl<R, M> TlsConnectionInHandshake<'_, R, M> +impl<R, M> TlsConnection<R, M> where M: SupportedMode, { /// Perform asynchronous handshake, until completion or until pending on non-I/O operations. /// /// The caller needs to ensure that any pending operations during the handshake are resolved, - /// before polling [`async_handshake`] again. + /// before polling [`Self::async_handshake`] again. pub fn async_handshake( &mut self, ) -> impl Send + Future<Output = Result<Option<TlsRetryReason>, Error>> + '_ {
diff --git a/rust/bssl-tls/src/context.rs b/rust/bssl-tls/src/context.rs index fe9e475..29088c6 100644 --- a/rust/bssl-tls/src/context.rs +++ b/rust/bssl-tls/src/context.rs
@@ -80,7 +80,7 @@ /// General TLS configuration /// /// The `Mode` generic can be either [`TlsMode`] or [`QuicMode`]. -/// This generic governs the kind of [`TlsConnection`] that can be constructed. +/// This generic governs the kind of [`crate::connection::TlsConnection`] that can be constructed. pub struct TlsContextBuilder<Mode = TlsMode> { ptr: NonNull<bssl_sys::SSL_CTX>, _p: PhantomData<fn() -> Mode>,
diff --git a/rust/bssl-tls/src/credentials/tests.rs b/rust/bssl-tls/src/credentials/tests.rs index 02a70e8..6ae988b 100644 --- a/rust/bssl-tls/src/credentials/tests.rs +++ b/rust/bssl-tls/src/credentials/tests.rs
@@ -167,14 +167,12 @@ let test_future = async { let client_handshake = async { - let mut in_handshake = client_conn.in_handshake().unwrap(); - in_handshake.async_handshake().await?; + assert!(client_conn.async_handshake().await?.is_none()); Ok::<(), crate::errors::Error>(()) }; let server_handshake = async { - let mut in_handshake = server_conn.in_handshake().unwrap(); - in_handshake.async_handshake().await?; + assert!(server_conn.async_handshake().await?.is_none()); Ok::<(), crate::errors::Error>(()) }; @@ -222,13 +220,11 @@ server_conn.set_split_io(server_reader, server_writer)?; let server_thread = std::thread::spawn(move || { - let mut in_handshake = server_conn.in_handshake().unwrap(); - in_handshake.do_handshake().unwrap(); + server_conn.do_handshake().unwrap(); server_conn }); - let mut in_handshake = client_conn.in_handshake().unwrap(); - in_handshake.do_handshake().unwrap(); + client_conn.do_handshake().unwrap(); let _server_conn = server_thread.join().unwrap(); @@ -473,19 +469,9 @@ client_conn.set_io(sock_client)?; let test_future = async { - let client_handshake = async { - if let Some(mut in_handshake) = client_conn.in_handshake() { - in_handshake.async_handshake().await?; - } - Ok::<(), crate::errors::Error>(()) - }; + let client_handshake = client_conn.async_handshake(); - let server_handshake = async { - if let Some(mut in_handshake) = server_conn.in_handshake() { - in_handshake.async_handshake().await?; - } - Ok::<(), crate::errors::Error>(()) - }; + let server_handshake = server_conn.async_handshake(); let handshake_result = futures::future::try_join(client_handshake, server_handshake).await;
diff --git a/rust/bssl-tls/src/sessions.rs b/rust/bssl-tls/src/sessions.rs index 05e6fe6..a1696b6 100644 --- a/rust/bssl-tls/src/sessions.rs +++ b/rust/bssl-tls/src/sessions.rs
@@ -390,8 +390,8 @@ executor .run(try_join( - conn_client.in_handshake().unwrap().async_handshake(), - conn_server.in_handshake().unwrap().async_handshake(), + conn_client.async_handshake(), + conn_server.async_handshake(), )) .unwrap(); @@ -441,8 +441,8 @@ executor .run(try_join( - conn_client.in_handshake().unwrap().async_handshake(), - conn_server.in_handshake().unwrap().async_handshake(), + conn_client.async_handshake(), + conn_server.async_handshake(), )) .unwrap();
diff --git a/rust/bssl-tls/src/tests.rs b/rust/bssl-tls/src/tests.rs index 83c7f5f..ed9b45f 100644 --- a/rust/bssl-tls/src/tests.rs +++ b/rust/bssl-tls/src/tests.rs
@@ -109,7 +109,7 @@ mut client_conn: TlsConnection<Client, M>, ) -> Result<(), Error> { let thread = std::thread::spawn(move || { - server_conn.in_handshake().unwrap().accept()?; + server_conn.accept()?; assert!(!server_conn.is_in_handshake()); // TODO: switch to `From` impls when Rust compiler is bumped to 1.95.0. let mut message = [MaybeUninit::uninit(); 21]; @@ -126,7 +126,7 @@ Ok::<_, Error>(()) }); - client_conn.in_handshake().unwrap().connect()?; + client_conn.connect()?; assert!(!client_conn.is_in_handshake()); client_conn.sync_write(b"BoringSSL is awesome!")?; let mut message = [MaybeUninit::uninit(); 19]; @@ -425,14 +425,7 @@ } pub async fn drive_handshake<R>(conn: &mut TlsConnection<R>) { - assert!( - conn.in_handshake() - .unwrap() - .async_handshake() - .await - .unwrap() - .is_none() - ); + assert!(conn.async_handshake().await.unwrap().is_none()); } #[test]
diff --git a/rust/bssl-tls/src/tests/credentials.rs b/rust/bssl-tls/src/tests/credentials.rs index 0dfe046..6c026de 100644 --- a/rust/bssl-tls/src/tests/credentials.rs +++ b/rust/bssl-tls/src/tests/credentials.rs
@@ -140,10 +140,8 @@ server_conn.set_io(server_socket)?; let mut server_task = async move || -> Result<(), crate::errors::Error> { - let mut in_handshake = server_conn.in_handshake().unwrap(); - loop { - match in_handshake.async_handshake().await { + match server_conn.async_handshake().await { Ok(None) => break, Ok(Some(TlsRetryReason::PendingPrivateKeyOperation)) => { struct Yield(bool); @@ -175,7 +173,7 @@ }; let client_task = async move || -> Result<(), crate::errors::Error> { - let res = client_conn.in_handshake().unwrap().do_handshake(); + let res = client_conn.do_handshake(); assert!( matches!(res, Ok(Some(TlsRetryReason::WantRead))), "Expected WantRead, got {:?}",
diff --git a/rust/bssl-tls/src/tests/datagram.rs b/rust/bssl-tls/src/tests/datagram.rs index 7f8775e..78a9649 100644 --- a/rust/bssl-tls/src/tests/datagram.rs +++ b/rust/bssl-tls/src/tests/datagram.rs
@@ -87,12 +87,8 @@ client_conn.set_io(client_socket)?; let test_future = async { - let server_handshake = - async { server_conn.in_handshake().unwrap().async_handshake().await }; - let client_handshake = - async { client_conn.in_handshake().unwrap().async_handshake().await }; - - futures::future::try_join(server_handshake, client_handshake).await?; + futures::future::try_join(server_conn.async_handshake(), client_conn.async_handshake()) + .await?; let server_data = async { let mut buf = [0u8; TEST_DATA.len()];