smb: handles records with trailing nbss data

If a file (read/write) SMB record has padding/trailing data
after the buffer being read or written, and that Suricata falls
in one case where it skips the data, it should skip until
the very end of the NBSS record, meaning it should also skip the
padding/trailing data.

Otherwise, an attacker may smuggle some NBSS/SMB record in this
trailing data, that will be interpreted by Suricata, but not
by the SMB client/server, leading to evasions.

Ticket: #5786

diff --git a/rust/src/smb/smb.rs b/rust/src/smb/smb.rs
index aeb7102a6fab1e9b8c47231eca0f125fc3c2249c..6acc948991b8242104d16ccbd52115f3abd87492 100644 (file)
--- a/rust/src/smb/smb.rs
+++ b/rust/src/smb/smb.rs
@@ -1172,13 +1172,12 @@ impl SMBState {
         }
     }
 
-    pub fn set_skip(&mut self, direction: Direction, rec_size: u32, data_size: u32)
+    pub fn set_skip(&mut self, direction: Direction, nbss_remaining: u32)
     {
-        let skip = rec_size.saturating_sub(data_size);
         if direction == Direction::ToServer {
-            self.skip_ts = skip;
+            self.skip_ts = nbss_remaining;
         } else {
-            self.skip_tc = skip;
+            self.skip_tc = nbss_remaining;
         }
     }
 

diff --git a/rust/src/smb/smb1.rs b/rust/src/smb/smb1.rs
index bbe18921ae34ca5ea8172e1d8b1849316f810dc0..378ed856e541c65dae4c6cdcecbc286f849a70e7 100644 (file)
--- a/rust/src/smb/smb1.rs
+++ b/rust/src/smb/smb1.rs
@@ -950,7 +950,7 @@ pub fn smb1_write_request_record(state: &mut SMBState, r: &SmbRecord, andx_offse
                 // Record claims more bytes than are in NBSS record...
                 state.set_event(SMBEvent::WriteRequestTooLarge);
                 // Skip the remaining bytes of the record.
-                state.set_skip(Direction::ToServer, nbss_remaining, 0);
+                state.set_skip(Direction::ToServer, nbss_remaining);
                 return;
             }
             let mut file_fid = rd.fid.to_vec();
@@ -1037,7 +1037,7 @@ pub fn smb1_read_response_record(state: &mut SMBState, r: &SmbRecord, andx_offse
                     // Record claims more bytes than are in NBSS record...
                     state.set_event(SMBEvent::ReadResponseTooLarge);
                     // Skip the remaining bytes of the record.
-                    state.set_skip(Direction::ToClient, nbss_remaining, 0);
+                    state.set_skip(Direction::ToClient, nbss_remaining);
                     return;
                 }
                 let fid_key = SMBCommonHdr::from1(r, SMBHDR_TYPE_OFFSET);
@@ -1046,7 +1046,7 @@ pub fn smb1_read_response_record(state: &mut SMBState, r: &SmbRecord, andx_offse
                     None => {
                         SCLogDebug!("SMBv1 READ response: reply to unknown request: left {} {:?}",
                                 rd.len - rd.data.len() as u32, rd);
-                        state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32);
+                        state.set_skip(Direction::ToClient, nbss_remaining);
                         return;
                     },
                 };

diff --git a/rust/src/smb/smb2.rs b/rust/src/smb/smb2.rs
index 36c31c577b85ab5968447743b3d0b1c7deb74d13..a2fe021b0276f6b7a56d79385cd2c9963ea4c5be 100644 (file)
--- a/rust/src/smb/smb2.rs
+++ b/rust/src/smb/smb2.rs
@@ -126,7 +126,7 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record, nbss_rema
                 // Record claims more bytes than are in NBSS record...
                 state.set_event(SMBEvent::ReadResponseTooLarge);
                 // Skip the remaining bytes of the record.
-                state.set_skip(Direction::ToClient, nbss_remaining, 0);
+                state.set_skip(Direction::ToClient, nbss_remaining);
                 return;
             }
             if r.nt_status == SMB_NTSTATUS_BUFFER_OVERFLOW {
@@ -135,7 +135,7 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record, nbss_rema
 
             } else if r.nt_status != SMB_NTSTATUS_SUCCESS {
                 SCLogDebug!("SMBv2: read response error code received: skip record");
-                state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32);
+                state.set_skip(Direction::ToClient, nbss_remaining);
                 return;
             }
 
@@ -143,7 +143,7 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record, nbss_rema
                (unsafe { SMB_CFG_MAX_READ_SIZE != 0 && SMB_CFG_MAX_READ_SIZE < rd.len })
             {
                 state.set_event(SMBEvent::ReadResponseTooLarge);
-                state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32);
+                state.set_skip(Direction::ToClient, nbss_remaining);
                 return;
             }
 
@@ -156,7 +156,7 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record, nbss_rema
                 Some(o) => (o.offset, o.guid),
                 None => {
                     SCLogDebug!("SMBv2 READ response: reply to unknown request {:?}",rd);
-                    state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32);
+                    state.set_skip(Direction::ToClient, nbss_remaining);
                     return;
                 },
             };
@@ -173,10 +173,10 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record, nbss_rema
                         }
                         if max_queue_size != 0 && tdf.file_tracker.get_inflight_size() + rd.len as u64 > max_queue_size.into() {
                             state.set_event(SMBEvent::ReadQueueSizeExceeded);
-                            state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32);
+                            state.set_skip(Direction::ToClient, nbss_remaining);
                         } else if max_queue_cnt != 0 && tdf.file_tracker.get_inflight_cnt() >= max_queue_cnt as usize {
                             state.set_event(SMBEvent::ReadQueueCntExceeded);
-                            state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32);
+                            state.set_skip(Direction::ToClient, nbss_remaining);
                         } else {
                             filetracker_newchunk(&mut tdf.file_tracker,
                                     &tdf.file_name, rd.data, offset,
@@ -227,7 +227,7 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record, nbss_rema
                     smb_read_dcerpc_record(state, vercmd, hdr, &file_guid, rd.data);
                 } else if is_pipe {
                     SCLogDebug!("non-DCERPC pipe");
-                    state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32);
+                    state.set_skip(Direction::ToClient, nbss_remaining);
                 } else {
                     let file_name = match state.guid2name_map.get(&file_guid) {
                         Some(n) => { n.to_vec() }
@@ -246,10 +246,10 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record, nbss_rema
                         }
                         if max_queue_size != 0 && tdf.file_tracker.get_inflight_size() + rd.len as u64 > max_queue_size.into() {
                             state.set_event(SMBEvent::ReadQueueSizeExceeded);
-                            state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32);
+                            state.set_skip(Direction::ToClient, nbss_remaining);
                         } else if max_queue_cnt != 0 && tdf.file_tracker.get_inflight_cnt() >= max_queue_cnt as usize {
                             state.set_event(SMBEvent::ReadQueueCntExceeded);
-                            state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32);
+                            state.set_skip(Direction::ToClient, nbss_remaining);
                         } else {
                             filetracker_newchunk(&mut tdf.file_tracker,
                                     &file_name, rd.data, offset,
@@ -288,13 +288,13 @@ pub fn smb2_write_request_record(state: &mut SMBState, r: &Smb2Record, nbss_rema
                 // Record claims more bytes than are in NBSS record...
                 state.set_event(SMBEvent::WriteRequestTooLarge);
                 // Skip the remaining bytes of the record.
-                state.set_skip(Direction::ToServer, nbss_remaining, 0);
+                state.set_skip(Direction::ToServer, nbss_remaining);
                 return;
             }
             if (state.max_write_size != 0 && wr.wr_len > state.max_write_size) ||
                (unsafe { SMB_CFG_MAX_WRITE_SIZE != 0 && SMB_CFG_MAX_WRITE_SIZE < wr.wr_len }) {
                 state.set_event(SMBEvent::WriteRequestTooLarge);
-                state.set_skip(Direction::ToServer, wr.wr_len, wr.data.len() as u32);
+                state.set_skip(Direction::ToServer, nbss_remaining);
                 return;
             }
 
@@ -318,10 +318,10 @@ pub fn smb2_write_request_record(state: &mut SMBState, r: &Smb2Record, nbss_rema
                         }
                         if max_queue_size != 0 && tdf.file_tracker.get_inflight_size() + wr.wr_len as u64 > max_queue_size.into() {
                             state.set_event(SMBEvent::WriteQueueSizeExceeded);
-                            state.set_skip(Direction::ToServer, wr.wr_len, wr.data.len() as u32);
+                            state.set_skip(Direction::ToServer, nbss_remaining);
                         } else if max_queue_cnt != 0 && tdf.file_tracker.get_inflight_cnt() >= max_queue_cnt as usize {
                             state.set_event(SMBEvent::WriteQueueCntExceeded);
-                            state.set_skip(Direction::ToServer, wr.wr_len, wr.data.len() as u32);
+                            state.set_skip(Direction::ToServer, nbss_remaining);
                         } else {
                             filetracker_newchunk(&mut tdf.file_tracker,
                                     &file_name, wr.data, wr.wr_offset,
@@ -372,7 +372,7 @@ pub fn smb2_write_request_record(state: &mut SMBState, r: &Smb2Record, nbss_rema
                     smb_write_dcerpc_record(state, vercmd, hdr, wr.data);
                 } else if is_pipe {
                     SCLogDebug!("non-DCERPC pipe: skip rest of the record");
-                    state.set_skip(Direction::ToServer, wr.wr_len, wr.data.len() as u32);
+                    state.set_skip(Direction::ToServer, nbss_remaining);
                 } else {
                     let tx = state.new_file_tx(&file_guid, &file_name, Direction::ToServer);
                     tx.vercmd.set_smb2_cmd(SMB2_COMMAND_WRITE);
@@ -386,10 +386,10 @@ pub fn smb2_write_request_record(state: &mut SMBState, r: &Smb2Record, nbss_rema
 
                         if max_queue_size != 0 && tdf.file_tracker.get_inflight_size() + wr.wr_len as u64 > max_queue_size.into() {
                             state.set_event(SMBEvent::WriteQueueSizeExceeded);
-                            state.set_skip(Direction::ToServer, wr.wr_len, wr.data.len() as u32);
+                            state.set_skip(Direction::ToServer, nbss_remaining);
                         } else if max_queue_cnt != 0 && tdf.file_tracker.get_inflight_cnt() >= max_queue_cnt as usize {
                             state.set_event(SMBEvent::WriteQueueCntExceeded);
-                            state.set_skip(Direction::ToServer, wr.wr_len, wr.data.len() as u32);
+                            state.set_skip(Direction::ToServer, nbss_remaining);
                         } else {
                             filetracker_newchunk(&mut tdf.file_tracker,
                                     &file_name, wr.data, wr.wr_offset,