From 2fa3d96966952375d238d869fde6d74abb172cd6 Mon Sep 17 00:00:00 2001 From: Christopher Patton Date: Tue, 8 Aug 2023 16:43:36 -0700 Subject: [PATCH] Use features to set key exchange preferences Overwrite boringSSL's default key exchange preferences with safe defaults using feature flags: * "kx-pq-supported" enables support for PQ key exchange algorithms. Classical key exchange is still preferred, but will be upgraded to PQ if requested. * "kx-pq-preferred" enables preference for PQ key exchange, with fallback to classical key exchange if requested. * "kx-nist-required" disables non-NIST key exchange. Each feature implies "kx-safe-default". When this feature is enabled, don't compile bindings for `SSL_CTX_set1_curves()` and `SslCurve`. This is to prevent the feature flags from silently overriding curve preferences chosen by the user. Ideally we'd allow both: that is, use "kx-*" to set defaults, but still allow the user to manually override them. However, this doesn't work because by the time the `SSL_CTX` is constructed, we don't yet know whether we're the client or server. (The "kx-*" features set different preferences for each.) If "kx-sfe-default" is set, then the curve preferences are set just before initiating a TLS handshake (`SslStreamBuilder::connect()`) or waiting for a TLS handshake (`SslStreamBuilder::accept()`). --- .github/workflows/ci.yml | 4 ++- boring-sys/Cargo.toml | 6 +++- boring/Cargo.toml | 38 +++++++++++++++++++++---- boring/src/ssl/mod.rs | 57 ++++++++++++++++++++++++++++++++++++++ boring/src/ssl/test/mod.rs | 20 +++++++++++++ 5 files changed, 118 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2f0b3e73..2b40aae5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -237,4 +237,6 @@ jobs: - run: cargo test --features pq-experimental name: Run `pq-experimental` tests - run: cargo test --features pq-experimental,rpk - name: Run `pq-experimental,rpk` tests \ No newline at end of file + name: Run `pq-experimental,rpk` tests + - run: cargo test --features kx-safe-default,pq-experimental + name: Run `kx-safe-default` tests diff --git a/boring-sys/Cargo.toml b/boring-sys/Cargo.toml index b37a91dc..5a94948a 100644 --- a/boring-sys/Cargo.toml +++ b/boring-sys/Cargo.toml @@ -63,7 +63,11 @@ fips-link-precompiled = [] # Enables Raw public key API (https://datatracker.ietf.org/doc/html/rfc7250) rpk = [] -# Enables experimental post-quantum crypto (https://blog.cloudflare.com/post-quantum-for-all/) +# Applies a patch (`patches/boring-pq.patch`) to the boringSSL source code that +# enables support for PQ key exchange. This feature is necessary in order to +# compile the bindings for the default branch of boringSSL (`deps/boringssl`). +# Alternatively, a version of boringSSL that implements the same feature set +# can be provided by setting `BORING_BSSL_SOURCE_PATH`. pq-experimental = [] # Disables git patching of the BoringSSL sources for features like `rpk` and `pq-experimental`, but diff --git a/boring/Cargo.toml b/boring/Cargo.toml index 7cf00d0e..8d983fe8 100644 --- a/boring/Cargo.toml +++ b/boring/Cargo.toml @@ -16,6 +16,8 @@ features = ["rpk", "pq-experimental"] rustdoc-args = ["--cfg", "docsrs"] [features] +# Controlling the build + # Use a FIPS-validated version of boringssl. fips = ["boring-sys/fips"] @@ -25,17 +27,43 @@ fips-link-precompiled = ["boring-sys/fips-link-precompiled"] # Enables Raw public key API (https://datatracker.ietf.org/doc/html/rfc7250) rpk = ["boring-sys/rpk"] -# Enables experimental post-quantum crypto (https://blog.cloudflare.com/post-quantum-for-all/) +# Applies a patch to the boringSSL source code that enables support for PQ key +# exchange. This feature is necessary in order to compile the bindings for the +# default branch of boringSSL. Alternatively, a version of boringSSL that +# implements the same feature set can be provided by setting +# `BORING_BSSL_SOURCE_PATH`. pq-experimental = ["boring-sys/pq-experimental"] # Disables git patching of the BoringSSL sources for features like `rpk` and `pq-experimental`, but -# keeps the related Rust API. -# -# Supposed to be used with either pre-compiled BoringSSL (via `BORING_BSSL_PATH` env variable) or -# with custom BoringSSL sources (via `BORING_BSSL_SOURCE_PATH` env variable) already containing +# keeps the related Rust API. +# +# Supposed to be used with either pre-compiled BoringSSL (via `BORING_BSSL_PATH` env variable) or +# with custom BoringSSL sources (via `BORING_BSSL_SOURCE_PATH` env variable) already containing # required patches. no-patches = ["boring-sys/no-patches"] +# Controlling key exchange preferences at compile time + +# Choose key exchange preferences at compile time. This prevents the user from +# choosing their own preferences. +kx-safe-default = [] + +# Support PQ key exchange. The client will prefer classical key exchange, but +# will upgrade to PQ key exchange if requested by the server. This is the +# safest option if you don't know if the peer supports PQ key exchange. This +# feature implies "kx-safe-default". +kx-client-pq-supported = ["kx-safe-default"] + +# Prefer PQ key exchange. The client will prefer PQ exchange, but fallback to +# classical key exchange if requested by the server. This is the best option if +# you know the peer supports PQ key exchange. This feature implies +# "kx-safe-default" and "kx-client-pq-supported". +kx-client-pq-preferred = ["kx-safe-default", "kx-client-pq-supported"] + +# Disable key exchange involving non-NIST key exchange on the client side. +# Implies "kx-safe-default". +kx-client-nist-required = ["kx-safe-default"] + [dependencies] bitflags = { workspace = true } foreign-types = { workspace = true } diff --git a/boring/src/ssl/mod.rs b/boring/src/ssl/mod.rs index aec7508b..bd3dde77 100644 --- a/boring/src/ssl/mod.rs +++ b/boring/src/ssl/mod.rs @@ -619,10 +619,12 @@ impl SslSignatureAlgorithm { } /// A TLS Curve. +#[cfg(not(feature = "kx-safe-default"))] #[repr(transparent)] #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub struct SslCurve(c_int); +#[cfg(not(feature = "kx-safe-default"))] impl SslCurve { pub const SECP224R1: SslCurve = SslCurve(ffi::NID_secp224r1); @@ -1703,6 +1705,11 @@ impl SslContextBuilder { /// This corresponds to [`SSL_CTX_set1_curves`] /// /// [`SSL_CTX_set1_curves`]: https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#SSL_CTX_set1_curves + // + // If the "kx-*" flags are used to set key exchange preference, then don't allow the user to + // set them here. This ensures we don't override the user's preference without telling them: + // when the flags are used, the preferences are set just before connecting or accepting. + #[cfg(not(feature = "kx-safe-default"))] pub fn set_curves(&mut self, curves: &[SslCurve]) -> Result<(), ErrorStack> { unsafe { cvt_0i(ffi::SSL_CTX_set1_curves( @@ -2400,6 +2407,48 @@ impl SslRef { unsafe { ErrorCode::from_raw(ffi::SSL_get_error(self.as_ptr(), ret)) } } + #[cfg(feature = "kx-safe-default")] + fn set_curves_list(&mut self, curves: &str) -> Result<(), ErrorStack> { + let curves = CString::new(curves).unwrap(); + unsafe { + cvt(ffi::SSL_set1_curves_list( + self.as_ptr(), + curves.as_ptr() as *const _, + )) + .map(|_| ()) + } + } + + #[cfg(feature = "kx-safe-default")] + fn client_set_default_curves_list(&mut self) -> Result<(), ErrorStack> { + let curves = if cfg!(feature = "kx-client-pq-preferred") { + if cfg!(feature = "kx-client-nist-required") { + "P256Kyber768Draft00:P-256:P-384" + } else { + "X25519Kyber768Draft00:X25519:P256Kyber768Draft00:P-256:P-384" + } + } else if cfg!(feature = "kx-client-pq-supported") { + if cfg!(feature = "kx-client-nist-required") { + "P-256:P-384:P256Kyber768Draft00" + } else { + "X25519:P-256:P-384:X25519Kyber768Draft00:P256Kyber768Draft00" + } + } else { + if cfg!(feature = "kx-client-nist-required") { + "P-256:P-384" + } else { + "X25519:P-256:P-384" + } + }; + + self.set_curves_list(curves) + } + + #[cfg(feature = "kx-safe-default")] + fn server_set_default_curves_list(&mut self) -> Result<(), ErrorStack> { + self.set_curves_list("X25519Kyber768Draft00:P256Kyber768Draft00:X25519:P-256:P-384") + } + /// Like [`SslContextBuilder::set_verify`]. /// /// This corresponds to [`SSL_set_verify`]. @@ -3480,6 +3529,10 @@ where /// See `Ssl::connect` pub fn connect(self) -> Result, HandshakeError> { let mut stream = self.inner; + + #[cfg(feature = "kx-safe-default")] + stream.ssl.client_set_default_curves_list()?; + let ret = unsafe { ffi::SSL_connect(stream.ssl.as_ptr()) }; if ret > 0 { Ok(stream) @@ -3503,6 +3556,10 @@ where /// See `Ssl::accept` pub fn accept(self) -> Result, HandshakeError> { let mut stream = self.inner; + + #[cfg(feature = "kx-safe-default")] + stream.ssl.server_set_default_curves_list()?; + let ret = unsafe { ffi::SSL_accept(stream.ssl.as_ptr()) }; if ret > 0 { Ok(stream) diff --git a/boring/src/ssl/test/mod.rs b/boring/src/ssl/test/mod.rs index e72dc086..367b8f4d 100644 --- a/boring/src/ssl/test/mod.rs +++ b/boring/src/ssl/test/mod.rs @@ -1115,3 +1115,23 @@ fn session_cache_size() { let ctx = ctx.build(); assert_eq!(ctx.session_cache_size(), 1234); } + +#[cfg(feature = "kx-safe-default")] +#[test] +fn client_set_default_curves_list() { + let ssl_ctx = SslContextBuilder::new(SslMethod::tls()).unwrap().build(); + let mut ssl = Ssl::new(&ssl_ctx).unwrap(); + + ssl.client_set_default_curves_list() + .expect("Failed to set curves list. Is Kyber768 missing in boringSSL?") +} + +#[cfg(feature = "kx-safe-default")] +#[test] +fn server_set_default_curves_list() { + let ssl_ctx = SslContextBuilder::new(SslMethod::tls()).unwrap().build(); + let mut ssl = Ssl::new(&ssl_ctx).unwrap(); + + ssl.server_set_default_curves_list() + .expect("Failed to set curves list. Is Kyber768 missing in boringSSL?") +}