Clients should accept messages secured by an expired SecurityToken

I encountered a bug where an incoming message would fail with an invalid
signature error. After quite some debugging I noticed this only occurred
when the message was received just a few milliseconds after a security
channel renewal.

After careful inspection it turned out that the message was still
encrypted (which is according to spec) with the remote keys of the old
security channel, but the client tried to validate the message with the
renewed keys.

According to the specs clients should accept messages secured by an
expired SecurityToken for up to 25 % of the token lifetime (see
https://reference.opcfoundation.org/Core/Part4/v105/docs/5.5.2 for more
details), so I added a bit of logic to store the last 5 used remote keys
and choose which one to use based on the token ID in the security
header.

After making these changes and testing again against the same OPC UA
server the issue did not occur again (while it occurred multiple times
per hour before this fix was implemented) so this seems like a nice
little improvement making the package a little bit more complient.

Author: svanharmelen

lib/src/core/comms/secure_channel.rs

@@ -3,12 +3,13 @@
 // Copyright (C) 2017-2024 Adam Lock
 
 use std::{
+    collections::HashMap,
     io::{Cursor, Write},
     ops::Range,
     sync::Arc,
 };
 
-use chrono::{Duration, TimeDelta};
+use chrono::Duration;
 
 use crate::crypto::{
     aeskey::AesKey,
@@ -35,6 +36,12 @@ pub enum Role {
     Server,
 }
 
+#[derive(Debug)]
+struct RemoteKeys {
+    keys: (Vec<u8>, AesKey, Vec<u8>),
+    expires_at: DateTime,
+}
+
 /// Holds all of the security information related to this session
 #[derive(Debug)]
 pub struct SecureChannel {
@@ -63,7 +70,14 @@ pub struct SecureChannel {
     /// Our nonce generated while handling open secure channel
     local_nonce: Vec<u8>,
     /// Client (i.e. other end's set of keys) Symmetric Signing Key, Encrypt Key, IV
-    remote_keys: Option<(Vec<u8>, AesKey, Vec<u8>)>,
+    ///
+    /// This is a map of channel token ids and their respective keys. We need to keep
+    /// the old keys around as the client should accept messages secured by an expired
+    /// SecurityToken for up to 25 % of the token lifetime.
+    ///
+    /// See the "OpenSecureChannel" section in the spec for more info:
+    /// https://reference.opcfoundation.org/Core/Part4/v105/docs/5.5.2
+    remote_keys: HashMap<u32, RemoteKeys>,
     /// Server (i.e. our end's set of keys) Symmetric Signing Key, Decrypt Key, IV
     local_keys: Option<(Vec<u8>, AesKey, Vec<u8>)>,
     /// Decoding options
@@ -88,7 +102,7 @@ impl SecureChannel {
             private_key: None,
             remote_cert: None,
             local_keys: None,
-            remote_keys: None,
+            remote_keys: HashMap::new(),
             decoding_options: DecodingOptions::default(),
         }
     }
@@ -121,7 +135,7 @@ impl SecureChannel {
             private_key,
             remote_cert: None,
             local_keys: None,
-            remote_keys: None,
+            remote_keys: HashMap::new(),
             decoding_options,
         }
     }
@@ -226,10 +240,10 @@ impl SecureChannel {
             false
         } else {
             // Check if secure channel 75% close to expiration in which case send a renew
-            let renew_lifetime = (self.token_lifetime() * 3) / 4;
-            let renew_lifetime = TimeDelta::try_milliseconds(renew_lifetime as i64).unwrap();
+            let renew_lifetime = (self.token_lifetime * 3) / 4;
+            let renew_lifetime = Duration::milliseconds(renew_lifetime as i64);
             // Renew the token?
-            DateTime::now() - self.token_created_at() > renew_lifetime
+            DateTime::now() - self.token_created_at > renew_lifetime
         }
     }
 
@@ -356,7 +370,7 @@ impl SecureChannel {
     /// are used to secure Messages sent by the Server.
     ///
     pub fn derive_keys(&mut self) {
-        self.remote_keys = Some(
+        self.insert_remote_keys(
             self.security_policy
                 .make_secure_channel_keys(&self.local_nonce, &self.remote_nonce),
         );
@@ -366,16 +380,16 @@ impl SecureChannel {
         );
         trace!("Remote nonce = {:?}", self.remote_nonce);
         trace!("Local nonce = {:?}", self.local_nonce);
-        trace!("Derived remote keys = {:?}", self.remote_keys);
+        trace!(
+            "Derived remote keys = {:?}",
+            self.get_remote_keys(self.token_id)
+        );
         trace!("Derived local keys = {:?}", self.local_keys);
     }
 
     /// Test if the token has expired yet
     pub fn token_has_expired(&self) -> bool {
-        let token_created_at = self.token_created_at;
-        let token_expires =
-            token_created_at + TimeDelta::try_seconds(self.token_lifetime as i64).unwrap();
-        DateTime::now().ge(&token_expires)
+        DateTime::now() >= self.token_created_at + Duration::seconds(self.token_lifetime as i64)
     }
 
     /// Calculates the signature size for a message depending on the supplied security header
@@ -739,11 +753,20 @@ impl SecureChannel {
                 encrypted_range
             );
 
+            let SecurityHeader::Symmetric(security_header) = security_header else {
+                error!(
+                    "Expected symmetric security header, got {:?}",
+                    security_header
+                );
+                return Err(StatusCode::BadUnexpectedError);
+            };
+
             let mut decrypted_data = vec![0u8; message_size];
             let decrypted_size = self.symmetric_decrypt_and_verify(
                 src,
                 signed_range,
                 encrypted_range,
+                security_header.token_id,
                 &mut decrypted_data,
             )?;
 
@@ -1048,8 +1071,32 @@ impl SecureChannel {
         self.local_keys.as_ref().unwrap()
     }
 
-    fn remote_keys(&self) -> &(Vec<u8>, AesKey, Vec<u8>) {
-        self.remote_keys.as_ref().unwrap()
+    fn insert_remote_keys(&mut self, keys: (Vec<u8>, AesKey, Vec<u8>)) {
+        let expires_at = (self.token_lifetime as f32 * 1.25).ceil();
+        let expires_at = Duration::milliseconds(expires_at as i64);
+
+        // Insert the remote keys.
+        self.remote_keys.insert(
+            self.token_id,
+            RemoteKeys {
+                keys,
+                expires_at: self.token_created_at + expires_at,
+            },
+        );
+
+        // Remove any expired keys.
+        self.remote_keys
+            .retain(|_, v| DateTime::now() < v.expires_at);
+    }
+
+    fn get_remote_keys(&self, token_id: u32) -> Result<&(Vec<u8>, AesKey, Vec<u8>), StatusCode> {
+        match self.remote_keys.get(&token_id) {
+            Some(remote_keys) => Ok(&remote_keys.keys),
+            None => {
+                error!("No remote keys found for token: {}", token_id);
+                Err(StatusCode::BadUnexpectedError)
+            }
+        }
     }
 
     fn encryption_keys(&self) -> (&AesKey, &[u8]) {
@@ -1061,13 +1108,13 @@ impl SecureChannel {
         &(self.local_keys()).0
     }
 
-    fn decryption_keys(&self) -> (&AesKey, &[u8]) {
-        let keys = self.remote_keys();
-        (&keys.1, &keys.2)
+    fn decryption_keys(&self, token_id: u32) -> Result<(&AesKey, &[u8]), StatusCode> {
+        let keys = self.get_remote_keys(token_id)?;
+        Ok((&keys.1, &keys.2))
     }
 
-    fn verification_key(&self) -> &[u8] {
-        &(self.remote_keys()).0
+    fn verification_key(&self, token_id: u32) -> Result<&[u8], StatusCode> {
+        Ok(&(self.get_remote_keys(token_id))?.0)
     }
 
     /// Encode data using security. Destination buffer is expected to be same size as src and expected
@@ -1178,6 +1225,7 @@ impl SecureChannel {
         src: &[u8],
         signed_range: Range<usize>,
         encrypted_range: Range<usize>,
+        token_id: u32,
         dst: &mut [u8],
     ) -> Result<usize, StatusCode> {
         match self.security_mode {
@@ -1198,7 +1246,7 @@ impl SecureChannel {
                     signed_range,
                     signed_range.end
                 );
-                let verification_key = self.verification_key();
+                let verification_key = self.verification_key(token_id)?;
                 self.security_policy.symmetric_verify_signature(
                     verification_key,
                     &dst[signed_range.clone()],
@@ -1222,7 +1270,7 @@ impl SecureChannel {
 
                 // Decrypt encrypted portion
                 let mut decrypted_tmp = vec![0u8; ciphertext_size + 16]; // tmp includes +16 for blocksize
-                let (key, iv) = self.decryption_keys();
+                let (key, iv) = self.decryption_keys(token_id)?;
 
                 trace!(
                     "Secure decrypt called with encrypted range {:?}",
@@ -1250,7 +1298,7 @@ impl SecureChannel {
                     signed_range,
                     signature_range
                 );
-                let verification_key = self.verification_key();
+                let verification_key = self.verification_key(token_id)?;
                 self.security_policy.symmetric_verify_signature(
                     verification_key,
                     &dst[signed_range],

lib/src/core/tests/chunk.rs

@@ -53,7 +53,7 @@ fn set_chunk_sequence_number(
     sequence_number: u32,
 ) -> u32 {
     // Read the sequence header
-    let mut chunk_info = chunk.chunk_info(&secure_channel).unwrap();
+    let mut chunk_info = chunk.chunk_info(secure_channel).unwrap();
     let old_sequence_number = chunk_info.sequence_header.sequence_number;
     chunk_info.sequence_header.sequence_number = sequence_number;
     // Write the sequence header out again with new value
@@ -69,7 +69,7 @@ fn set_chunk_request_id(
     request_id: u32,
 ) -> u32 {
     // Read the sequence header
-    let mut chunk_info = chunk.chunk_info(&secure_channel).unwrap();
+    let mut chunk_info = chunk.chunk_info(secure_channel).unwrap();
     let old_request_id = chunk_info.sequence_header.request_id;
     chunk_info.sequence_header.request_id = request_id;
     // Write the sequence header out again with new value
@@ -341,7 +341,7 @@ fn chunk_open_secure_channel() {
         assert_eq!(request_header.timestamp.ticks(), 131284521470690000);
         assert_eq!(request_header.request_handle, 1);
         assert!(request_header.return_diagnostics.is_empty());
-        assert_eq!(request_header.audit_entry_id.is_null(), true);
+        assert!(request_header.audit_entry_id.is_null());
         assert_eq!(request_header.timeout_hint, 0);
     }
 
@@ -408,7 +408,7 @@ fn open_secure_channel_response() {
     };
     assert_eq!(response.response_header.request_handle, 0);
     assert_eq!(response.response_header.service_result, StatusCode::Good);
-    assert_eq!(response.response_header.string_table.is_none(), true);
+    assert!(response.response_header.string_table.is_none());
     assert_eq!(response.server_nonce, ByteString::null());
 }
 
@@ -464,7 +464,7 @@ fn security_policy_symmetric_encrypt_decrypt() {
 
     let mut src2 = vec![0u8; 200];
     let decrypted_len = secure_channel2
-        .symmetric_decrypt_and_verify(&dst, 0..80, 20..100, &mut src2)
+        .symmetric_decrypt_and_verify(&dst, 0..80, 20..100, 0, &mut src2)
         .unwrap();
     assert_eq!(decrypted_len, 100);
 

lib/src/core/tests/secure_channel.rs

@@ -24,7 +24,7 @@ fn test_symmetric_encrypt_decrypt(
 
         let mut encrypted_data = vec![0u8; chunk.data.len() + 4096];
         let encrypted_size = secure_channel1
-            .apply_security(&chunk, &mut encrypted_data[..])
+            .apply_security(chunk, &mut encrypted_data[..])
             .unwrap();
         trace!("Result of applying security = {}", encrypted_size);
 
@@ -81,7 +81,7 @@ fn test_asymmetric_encrypt_decrypt(
 
         let mut encrypted_data = vec![0u8; chunk.data.len() + 4096];
         let encrypted_size = secure_channel
-            .apply_security(&chunk, &mut encrypted_data[..])
+            .apply_security(chunk, &mut encrypted_data[..])
             .unwrap();
         trace!("Result of applying security = {}", encrypted_size);