Security1 (#216)

Fixes for potential security issues #211 & #212 - excessive chunks & decoding recursion checks on variants / extension objects

Author: locka99

lib/src/client/builder.rs

@@ -273,6 +273,18 @@ impl ClientBuilder {
         self.config.session_name = session_name.into();
         self
     }
+
+    /// Set the maximum message size
+    pub fn max_message_size(mut self, max_message_size: usize) -> Self {
+        self.config.decoding_options.max_message_size = max_message_size;
+        self
+    }
+
+    /// Set the max chunk count
+    pub fn max_chunk_count(mut self, max_chunk_count: usize) -> Self {
+        self.config.decoding_options.max_chunk_count = max_chunk_count;
+        self
+    }
 }
 
 #[test]

lib/src/client/client.rs

@@ -427,10 +427,12 @@ impl Client {
         let decoding_options = &self.config.decoding_options;
         DecodingOptions {
             max_chunk_count: decoding_options.max_chunk_count,
+            max_message_size: decoding_options.max_message_size,
             max_string_length: decoding_options.max_string_length,
             max_byte_string_length: decoding_options.max_byte_string_length,
             max_array_length: decoding_options.max_array_length,
             client_offset: Duration::zero(),
+            ..Default::default()
         }
     }
 

lib/src/client/comms/tcp_transport.rs

@@ -132,6 +132,7 @@ impl ReadState {
                 let chunks_len = self.chunks.len();
                 if self.max_chunk_count > 0 && chunks_len > self.max_chunk_count {
                     error!("too many chunks {}> {}", chunks_len, self.max_chunk_count);
+                    // TODO this code should return an error to be safe
                     //remove first
                     let first_req_id = *self.chunks.iter().next().unwrap().0;
                     self.chunks.remove(&first_req_id);
@@ -312,7 +313,7 @@ impl TcpTransport {
     pub fn connect(&self, endpoint_url: &str) -> Result<(), StatusCode> {
         debug_assert!(!self.is_connected(), "Should not try to connect when already connected");
         let (host, port) =
-            hostname_port_from_url(endpoint_url, constants::DEFAULT_OPC_UA_SERVER_PORT)?;
+            hostname_port_from_url(endpoint_url, crate::core::constants::DEFAULT_OPC_UA_SERVER_PORT)?;
 
         // Resolve the host name into a socket address
         let addr = {

lib/src/client/config.rs

@@ -136,6 +136,8 @@ impl ClientEndpoint {
 #[derive(Debug, PartialEq, Serialize, Deserialize, Clone)]
 pub struct DecodingOptions {
     /// Maximum size of a message chunk in bytes. 0 means no limit
+    pub max_message_size: usize,
+    /// Maximum number of chunks in a message. 0 means no limit
     pub max_chunk_count: usize,
     /// Maximum length in bytes (not chars!) of a string. 0 actually means 0, i.e. no string permitted
     pub max_string_length: usize,
@@ -311,7 +313,6 @@ impl ClientConfig {
         pki_dir.push(Self::PKI_DIR);
 
         let decoding_options = crate::types::DecodingOptions::default();
-
         ClientConfig {
             application_name: application_name.into(),
             application_uri: application_uri.into(),
@@ -334,6 +335,7 @@ impl ClientConfig {
                 max_string_length: decoding_options.max_string_length,
                 max_byte_string_length: decoding_options.max_byte_string_length,
                 max_chunk_count: decoding_options.max_chunk_count,
+                max_message_size: decoding_options.max_message_size,
             },
             performance: Performance {
                 ignore_clock_skew: false,

lib/src/client/session/session_state.rs

@@ -157,7 +157,6 @@ impl SessionState {
     const SEND_BUFFER_SIZE: usize = 65535;
     const RECEIVE_BUFFER_SIZE: usize = 65535;
     const MAX_BUFFER_SIZE: usize = 65535;
-    const MAX_CHUNK_COUNT: usize = 0;
 
     pub fn new(
         ignore_clock_skew: bool,
@@ -176,7 +175,7 @@ impl SessionState {
             send_buffer_size: Self::SEND_BUFFER_SIZE,
             receive_buffer_size: Self::RECEIVE_BUFFER_SIZE,
             max_message_size: Self::MAX_BUFFER_SIZE,
-            max_chunk_count: Self::MAX_CHUNK_COUNT,
+            max_chunk_count: constants::MAX_CHUNK_COUNT,
             request_handle: Handle::new(Self::FIRST_REQUEST_HANDLE),
             session_id: NodeId::null(),
             authentication_token: NodeId::null(),

lib/src/core/comms/message_chunk.rs

@@ -158,7 +158,8 @@ impl BinaryEncoder<MessageChunk> for MessageChunk {
             })?;
 
         let message_size = chunk_header.message_size as usize;
-        if decoding_options.max_chunk_count > 0 && message_size > decoding_options.max_chunk_count {
+        if decoding_options.max_message_size > 0 && message_size > decoding_options.max_message_size
+        {
             // Message_size should be sanity checked and rejected if too large.
             Err(StatusCode::BadTcpMessageTooLarge)
         } else {

lib/src/core/comms/message_writer.rs

@@ -66,25 +66,37 @@ impl MessageWriter {
             &message,
         )?;
 
-        // Sequence number monotonically increases per chunk
-        self.last_sent_sequence_number += chunks.len() as u32;
+        if self.max_chunk_count > 0 && chunks.len() > self.max_chunk_count {
+            error!(
+                "Cannot write message since {} chunks exceeds {} chunk limit",
+                chunks.len(),
+                self.max_chunk_count
+            );
+            Err(StatusCode::BadCommunicationError)
+        } else {
+            // Sequence number monotonically increases per chunk
+            self.last_sent_sequence_number += chunks.len() as u32;
 
-        // Send chunks
+            // Send chunks
 
-        // This max chunk size allows the message to be encoded to a chunk with header + encoding
-        // which is just slightly larger in size (up to 1024 bytes).
-        let max_chunk_count = self.buffer.get_ref().len() + 1024;
-        let mut data = vec![0u8; max_chunk_count];
-        for chunk in chunks {
-            trace!("Sending chunk {:?}", chunk);
-            let size = secure_channel.apply_security(&chunk, &mut data)?;
-            self.buffer.write(&data[..size]).map_err(|error| {
-                error!("Error while writing bytes to stream, connection broken, check error {:?}", error);
-                StatusCode::BadCommunicationError
-            })?;
+            // This max chunk size allows the message to be encoded to a chunk with header + encoding
+            // which is just slightly larger in size (up to 1024 bytes).
+            let data_buffer_size = self.buffer.get_ref().len() + 1024;
+            let mut data = vec![0u8; data_buffer_size];
+            for chunk in chunks {
+                trace!("Sending chunk {:?}", chunk);
+                let size = secure_channel.apply_security(&chunk, &mut data)?;
+                self.buffer.write(&data[..size]).map_err(|error| {
+                    error!(
+                        "Error while writing bytes to stream, connection broken, check error {:?}",
+                        error
+                    );
+                    StatusCode::BadCommunicationError
+                })?;
+            }
+            trace!("Message written");
+            Ok(request_id)
         }
-        trace!("Message written");
-        Ok(request_id)
     }
 
     pub fn next_request_id(&mut self) -> u32 {

lib/src/core/comms/secure_channel.rs

@@ -70,12 +70,14 @@ pub struct SecureChannel {
     decoding_options: DecodingOptions,
 }
 
-impl From<(SecurityPolicy, MessageSecurityMode)> for SecureChannel {
-    fn from(v: (SecurityPolicy, MessageSecurityMode)) -> Self {
+impl SecureChannel {
+    /// For testing purposes only
+    #[cfg(test)]
+    pub fn new_no_certificate_store() -> SecureChannel {
         SecureChannel {
             role: Role::Unknown,
-            security_policy: v.0,
-            security_mode: v.1,
+            security_policy: SecurityPolicy::None,
+            security_mode: MessageSecurityMode::None,
             secure_channel_id: 0,
             token_id: 0,
             token_created_at: DateTime::now(),
@@ -90,14 +92,6 @@ impl From<(SecurityPolicy, MessageSecurityMode)> for SecureChannel {
             decoding_options: DecodingOptions::default(),
         }
     }
-}
-
-impl SecureChannel {
-    /// For testing purposes only
-    #[cfg(test)]
-    pub fn new_no_certificate_store() -> SecureChannel {
-        (SecurityPolicy::None, MessageSecurityMode::None).into()
-    }
 
     pub fn new(
         certificate_store: Arc<RwLock<CertificateStore>>,
@@ -222,7 +216,7 @@ impl SecureChannel {
     }
 
     pub fn decoding_options(&self) -> DecodingOptions {
-        self.decoding_options
+        self.decoding_options.clone()
     }
 
     /// Test if the secure channel token needs to be renewed. The algorithm determines it needs

lib/src/core/comms/security_header.rs

@@ -88,8 +88,7 @@ impl BinaryEncoder<AsymmetricSecurityHeader> for AsymmetricSecurityHeader {
 
         // validate sender_certificate_length < MaxCertificateSize
         if sender_certificate.value.is_some()
-            && sender_certificate.value.as_ref().unwrap().len()
-                >= constants::MAX_CERTIFICATE_LENGTH as usize
+            && sender_certificate.value.as_ref().unwrap().len() >= constants::MAX_CERTIFICATE_LENGTH
         {
             error!("Sender certificate exceeds max certificate size");
             Err(StatusCode::BadDecodingError)

lib/src/core/comms/url.rs

@@ -6,7 +6,7 @@
 
 use url::Url;
 
-use crate::types::{constants::DEFAULT_OPC_UA_SERVER_PORT, status_code::StatusCode};
+use crate::types::status_code::StatusCode;
 
 pub const OPC_TCP_SCHEME: &str = "opc.tcp";
 
@@ -16,7 +16,7 @@ fn opc_url_from_str(s: &str) -> Result<Url, ()> {
         .map(|mut url| {
             if url.port().is_none() {
                 // If no port is supplied, then treat it as the default port 4840
-                let _ = url.set_port(Some(DEFAULT_OPC_UA_SERVER_PORT));
+                let _ = url.set_port(Some(crate::core::constants::DEFAULT_OPC_UA_SERVER_PORT));
             }
             url
         })
@@ -57,7 +57,7 @@ pub fn server_url_from_endpoint_url(endpoint_url: &str) -> std::result::Result<S
         url.set_query(None);
         if let Some(port) = url.port() {
             // If the port is the default, strip it so the url string omits it.
-            if port == DEFAULT_OPC_UA_SERVER_PORT {
+            if port == crate::core::constants::DEFAULT_OPC_UA_SERVER_PORT {
                 let _ = url.set_port(None);
             }
         }

lib/src/core/mod.rs

@@ -93,6 +93,13 @@ pub mod debug {
 #[cfg(test)]
 pub mod tests;
 
+pub mod constants {
+    /// Default OPC UA port number. Used by a discovery server. Other servers would normally run
+    /// on a different port. So OPC UA for Rust does not use this nr by default but it is used
+    /// implicitly in opc.tcp:// urls and elsewhere.
+    pub const DEFAULT_OPC_UA_SERVER_PORT: u16 = 4840;
+}
+
 pub mod comms;
 pub mod config;
 pub mod handle;

lib/src/core/tests/chunk.rs

@@ -36,7 +36,7 @@ fn sample_secure_channel_request_data_security_none() -> MessageChunk {
 
     // Decode chunk from stream
     stream.set_position(0);
-    let decoding_options = DecodingOptions::default();
+    let decoding_options = DecodingOptions::test();
     let chunk = MessageChunk::decode(&mut stream, &decoding_options).unwrap();
 
     println!(
@@ -102,6 +102,7 @@ fn chunk_multi_encode_decode() {
         max_string_length: 65535,
         max_byte_string_length: 65535,
         max_array_length: 20000, // Need to bump this up because large response uses a large array
+        ..Default::default()
     });
 
     let response = make_large_read_response();
@@ -152,7 +153,7 @@ fn chunk_multi_chunk_intermediate_final() {
     .unwrap();
     assert!(chunks.len() > 1);
 
-    let decoding_options = DecodingOptions::default();
+    let decoding_options = DecodingOptions::test();
 
     // All chunks except the last should be intermediate, the last should be final
     for (i, chunk) in chunks.iter().enumerate() {

lib/src/core/tests/comms.rs

@@ -24,7 +24,7 @@ fn ack_data() -> Vec<u8> {
 #[test]
 pub fn hello() {
     let mut stream = Cursor::new(hello_data());
-    let decoding_options = DecodingOptions::default();
+    let decoding_options = DecodingOptions::test();
     let hello = HelloMessage::decode(&mut stream, &decoding_options).unwrap();
     println!("hello = {:?}", hello);
     assert_eq!(hello.message_header.message_type, MessageType::Hello);
@@ -43,7 +43,7 @@ pub fn hello() {
 #[test]
 pub fn acknowledge() {
     let mut stream = Cursor::new(ack_data());
-    let decoding_options = DecodingOptions::default();
+    let decoding_options = DecodingOptions::test();
     let ack = AcknowledgeMessage::decode(&mut stream, &decoding_options).unwrap();
     println!("ack = {:?}", ack);
     assert_eq!(ack.message_header.message_type, MessageType::Acknowledge);

lib/src/core/tests/mod.rs

@@ -36,7 +36,7 @@ where
     println!("encoded bytes = {:?}", actual);
     let mut stream = Cursor::new(actual);
 
-    let decoding_options = DecodingOptions::default();
+    let decoding_options = DecodingOptions::test();
     let new_value: T = T::decode(&mut stream, &decoding_options).unwrap();
     println!("new value = {:?}", new_value);
     assert_eq!(value, new_value);

lib/src/server/builder.rs

@@ -308,36 +308,60 @@ impl ServerBuilder {
     }
 
     /// Set the maximum number of subscriptions in a session
-    pub fn max_subscriptions(mut self, max_subscriptions: u32) -> Self {
+    pub fn max_subscriptions(mut self, max_subscriptions: usize) -> Self {
         self.config.limits.max_subscriptions = max_subscriptions;
         self
     }
 
     /// Set the maximum number of monitored items per subscription
-    pub fn max_monitored_items_per_sub(mut self, max_monitored_items_per_sub: u32) -> Self {
+    pub fn max_monitored_items_per_sub(mut self, max_monitored_items_per_sub: usize) -> Self {
         self.config.limits.max_monitored_items_per_sub = max_monitored_items_per_sub;
         self
     }
 
     /// Set the max array length in elements
-    pub fn max_array_length(mut self, max_array_length: u32) -> Self {
+    pub fn max_array_length(mut self, max_array_length: usize) -> Self {
         self.config.limits.max_array_length = max_array_length;
         self
     }
 
     /// Set the max string length in characters, i.e. if you set max to 1000 characters, then with
     /// UTF-8 encoding potentially that's 4000 bytes.
-    pub fn max_string_length(mut self, max_string_length: u32) -> Self {
+    pub fn max_string_length(mut self, max_string_length: usize) -> Self {
         self.config.limits.max_string_length = max_string_length;
         self
     }
 
     /// Set the max bytestring length in bytes
-    pub fn max_byte_string_length(mut self, max_byte_string_length: u32) -> Self {
+    pub fn max_byte_string_length(mut self, max_byte_string_length: usize) -> Self {
         self.config.limits.max_byte_string_length = max_byte_string_length;
         self
     }
 
+    /// Set the maximum message size
+    pub fn max_message_size(mut self, max_message_size: usize) -> Self {
+        self.config.limits.max_message_size = max_message_size;
+        self
+    }
+
+    /// Set the max chunk count
+    pub fn max_chunk_count(mut self, max_chunk_count: usize) -> Self {
+        self.config.limits.max_chunk_count = max_chunk_count;
+        self
+    }
+
+    // Set the send buffer size
+    pub fn send_buffer_size(mut self, send_buffer_size: usize) -> Self {
+        self.config.limits.send_buffer_size = send_buffer_size;
+        self
+    }
+
+    // Set the receive buffer size
+    pub fn receive_buffer_size(mut self, receive_buffer_size: usize) -> Self {
+        self.config.limits.receive_buffer_size = receive_buffer_size;
+        self
+    }
+
     /// Sets the server to automatically trust client certs. This subverts the
     /// authentication during handshake, so only do this if you understand the risks.
     pub fn trust_client_certs(mut self) -> Self {