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

@@ -35,6 +35,9 @@ pub enum Role {
     Server,
 }
 
+// Number of stored remote keys.
+const STORED_REMOTE_KEYS: usize = 5;
+
 /// Holds all of the security information related to this session
 #[derive(Debug)]
 pub struct SecureChannel {
@@ -63,7 +66,15 @@ 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>)>,
+    ///
+    /// Store the last 5 used remote keys and choose which one to use based on the
+    /// token ID in the security header of the received message. This allows us to
+    /// properly decrypt messages that were encrypted with a previous key.
+    ///
+    /// See the "OpenSecureChannel" section in the spec for more info:
+    /// https://reference.opcfoundation.org/Core/Part4/v105/docs/5.5.2
+    #[allow(clippy::type_complexity)]
+    remote_keys: [Option<(Vec<u8>, AesKey, Vec<u8>)>; STORED_REMOTE_KEYS],
     /// 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 +99,7 @@ impl SecureChannel {
             private_key: None,
             remote_cert: None,
             local_keys: None,
-            remote_keys: None,
+            remote_keys: [const { None }; STORED_REMOTE_KEYS],
             decoding_options: DecodingOptions::default(),
         }
     }
@@ -121,7 +132,7 @@ impl SecureChannel {
             private_key,
             remote_cert: None,
             local_keys: None,
-            remote_keys: None,
+            remote_keys: [const { None }; STORED_REMOTE_KEYS],
             decoding_options,
         }
     }
@@ -226,10 +237,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 = (self.token_lifetime * 3) / 4;
             let renew_lifetime = TimeDelta::try_milliseconds(renew_lifetime as i64).unwrap();
             // Renew the token?
-            DateTime::now() - self.token_created_at() > renew_lifetime
+            DateTime::now() - self.token_created_at > renew_lifetime
         }
     }
 
@@ -356,7 +367,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,7 +377,10 @@ 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);
     }
 
@@ -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,18 @@ 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>)) {
+        self.remote_keys[self.token_id as usize % STORED_REMOTE_KEYS] = Some(keys);
+    }
+
+    fn get_remote_keys(&self, token_id: u32) -> Result<&(Vec<u8>, AesKey, Vec<u8>), StatusCode> {
+        match self.remote_keys[token_id as usize % STORED_REMOTE_KEYS] {
+            Some(ref keys) => Ok(keys),
+            None => {
+                error!("No remote keys found for channel token id: {}", token_id);
+                Err(StatusCode::BadUnexpectedError)
+            }
+        }
     }
 
     fn encryption_keys(&self) -> (&AesKey, &[u8]) {
@@ -1061,13 +1094,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 +1211,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 +1232,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 +1256,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 +1284,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);