From e4e691a790dc1dfe5b45261a40674a927085c206 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 25 Jan 2022 13:05:37 -0500 Subject: [PATCH] More comments on the limitations of tor-rtcompat's TLS API Also, more comments on why these limitations are safe within the context of Tor, but you wouldn't want to use them elsewhere. --- crates/tor-rtcompat/src/impls/rustls.rs | 3 +++ crates/tor-rtcompat/src/traits.rs | 29 ++++++++++++++++++++++--- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/crates/tor-rtcompat/src/impls/rustls.rs b/crates/tor-rtcompat/src/impls/rustls.rs index e3ae5b271..1cc482c65 100644 --- a/crates/tor-rtcompat/src/impls/rustls.rs +++ b/crates/tor-rtcompat/src/impls/rustls.rs @@ -233,6 +233,9 @@ fn convert_scheme( use rustls::internal::msgs::enums::SignatureScheme as R; use x509_signature::SignatureScheme as X; + // Yes, we do allow PKCS1 here. That's fine in practice when PKCS1 is only + // used (as in TLS 1.2) for signatures; the attacks against correctly + // implemented PKCS1 make sense only when it's used for encryption. Ok(match scheme { R::RSA_PKCS1_SHA256 => X::RSA_PKCS1_SHA256, R::ECDSA_NISTP256_SHA256 => X::ECDSA_NISTP256_SHA256, diff --git a/crates/tor-rtcompat/src/traits.rs b/crates/tor-rtcompat/src/traits.rs index 75470fdf5..443e0a436 100644 --- a/crates/tor-rtcompat/src/traits.rs +++ b/crates/tor-rtcompat/src/traits.rs @@ -172,10 +172,29 @@ pub trait CertifiedConn { /// An object that knows how to wrap a TCP connection (where the type of said TCP /// connection is `S`) with TLS. /// -/// (Note that because of Tor's peculiarities, this is not a +/// # Usage notes +/// +/// Note that because of Tor's peculiarities, this is not a /// general-purpose TLS type. Unlike typical users, Tor does not want -/// its TLS library to check whether the certificates are signed -/// within the web PKI hierarchy, or what their hostnames are. +/// its TLS library to check whether the certificates used in TLS are signed +/// within the web PKI hierarchy, or what their hostnames are, or even whether +/// they are valid. It *does*, however, check that the subject public key in the +/// certificate is indeed correctly used to authenticate the TLS handshake. +/// +/// If you are implementing something other than Tor, this is **not** the +/// functionality you want. +/// +/// How can this behavior be remotely safe, even in Tor? It only works for Tor +/// because the certificate that a Tor relay uses in TLS is not actually being +/// used to certify that relay's public key. Instead, the certificate only used +/// as a container for the relay's public key. The real certification happens +/// later, inside the TLS session, when the relay presents a CERTS cell. +/// +/// Such sneakiness was especially necessary before TLS 1.3, which encrypts more +/// of the handshake, and before pluggable transports, which make +/// "innocuous-looking TLS handshakes" less important than they once were. Once +/// TLS 1.3 is completely ubiquitous, we might be able to specify a simpler link +/// handshake than Tor uses now. #[async_trait] pub trait TlsConnector { /// The type of connection returned by this connector @@ -197,6 +216,10 @@ pub trait TlsConnector { /// This is separate from [`TlsConnector`] because eventually we may /// eventually want to support multiple `TlsConnector` implementations /// that use a single [`Runtime`]. +/// +/// See the [`TlsConnector`] documentation for a discussion of the Tor-specific +/// limitations of this trait: If you are implementing something other than Tor, +/// this is **not** the functionality you want. pub trait TlsProvider { /// The Connector object that this provider can return. type Connector: TlsConnector + Send + Sync + Unpin;