Send illegal_parameter alert on invalid key share

For client::tls12, ensure the alert is sent prior to -- instead of -- the CCS. This means the peer can read it.
rustls · Oct 3, 2024 · d752eb2 · d752eb2
1 parent 2d3b7ab
commit d752eb2
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 10 deletions.
diff --git a/rustls/src/client/tls12.rs b/rustls/src/client/tls12.rs
@@ -844,8 +844,9 @@ impl State<ClientConnectionData> for ExpectServerDone<'_> {
         //    a) generate our kx pair
         //    b) emit a ClientKeyExchange containing it
         //    c) if doing client auth, emit a CertificateVerify
-        //    d) emit a CCS
-        //    e) derive the shared keys, and start encryption
+        //    d) derive the shared keys
+        //    e) emit a CCS
+        //    f) use the derived keys to start encryption
         // 6. emit a Finished, our first encrypted message under the new keys.
 
         // 1.
@@ -956,19 +957,26 @@ impl State<ClientConnectionData> for ExpectServerDone<'_> {
             emit_certverify(&mut transcript, signer.as_ref(), cx.common)?;
         }
 
-        // 5d.
-        emit_ccs(cx.common);
-
-        // 5e. Now commit secrets.
+        // 5d. Derive secrets.
+        // An alert at this point will be sent in plaintext.  That must happen
+        // prior to the CCS, or else the peer will try to decrypt it.
         let secrets = ConnectionSecrets::from_key_exchange(
             kx,
             kx_params.pub_key(),
             ems_seed,
             st.randoms,
             suite,
-        )?;
+        )
+        .map_err(|err| {
+            cx.common
+                .send_fatal_alert(AlertDescription::IllegalParameter, err)
+        })?;
         cx.common.kx_state.complete();
 
+        // 5e. CCS. We are definitely going to switch on encryption.
+        emit_ccs(cx.common);
+
+        // 5f. Now commit secrets.
         st.config.key_log.log(
             "CLIENT_RANDOM",
             &secrets.randoms.client,

diff --git a/rustls/src/client/tls13.rs b/rustls/src/client/tls13.rs
@@ -149,7 +149,12 @@ pub(super) fn handle_server_hello(
     };
 
     cx.common.kx_state.complete();
-    let shared_secret = our_key_share.complete(&their_key_share.payload.0)?;
+    let shared_secret = our_key_share
+        .complete(&their_key_share.payload.0)
+        .map_err(|err| {
+            cx.common
+                .send_fatal_alert(AlertDescription::IllegalParameter, err)
+        })?;
 
     let mut key_schedule = key_schedule_pre_handshake.into_handshake(shared_secret);
 

diff --git a/rustls/src/server/tls12.rs b/rustls/src/server/tls12.rs
@@ -583,7 +583,11 @@ impl State<ServerConnectionData> for ExpectClientKx<'_> {
             ems_seed,
             self.randoms,
             self.suite,
-        )?;
+        )
+        .map_err(|err| {
+            cx.common
+                .send_fatal_alert(AlertDescription::IllegalParameter, err)
+        })?;
         cx.common.kx_state.complete();
 
         self.config.key_log.log(

diff --git a/rustls/src/server/tls13.rs b/rustls/src/server/tls13.rs
@@ -490,7 +490,12 @@ mod client_hello {
         // Prepare key exchange; the caller already found the matching SupportedKxGroup
         let (share, kxgroup) = share_and_kxgroup;
         debug_assert_eq!(kxgroup.name(), share.group);
-        let ckx = kxgroup.start_and_complete(&share.payload.0)?;
+        let ckx = kxgroup
+            .start_and_complete(&share.payload.0)
+            .map_err(|err| {
+                cx.common
+                    .send_fatal_alert(AlertDescription::IllegalParameter, err)
+            })?;
         cx.common.kx_state.complete();
 
         extensions.push(ServerExtension::KeyShare(KeyShareEntry::new(