From: Victor Julien <vjulien@oisf.net>
Date: Wed, 30 Nov 2022 05:44:40 +0000 (+0100)
Subject: smb: fix file reopening issue
X-Git-Tag: suricata-6.0.10~63
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d607c9295baacdb8122d9813043bf65f3dcea2d8;p=thirdparty%2Fsuricata.git

smb: fix file reopening issue

Fuzzing highlighted an issue where a command sequence on the same file
id triggered a logging issue:

file data for id N
close id N
file data for id N

If this happened in a single blob of data passed to the parser, the
existing file tx would be reused, the file "reopened", confusing the
file logging logic. This would trigger a debug assert.

This patch makes sure a new file tx is created for the file data
coming in after the first file tx is closed.

Bug: #5567.
(cherry picked from commit 45eb038e63604766de2828f6f25d145fea040424)
---

diff --git a/rust/src/smb/files.rs b/rust/src/smb/files.rs
index 0cfb61bff1..e2aa640338 100644
--- a/rust/src/smb/files.rs
+++ b/rust/src/smb/files.rs
@@ -117,7 +117,32 @@ impl SMBState {
         return (tx_ref.unwrap(), files, flags)
     }
 
-    pub fn get_file_tx_by_fuid(&mut self, fuid: &Vec<u8>, direction: u8)
+    /// get file tx for a open file. Returns None if a file for the fuid exists,
+    /// but has already been closed.
+    pub fn get_file_tx_by_fuid_with_open_file(&mut self, fuid: &[u8], direction: u8)
+        -> Option<(&mut SMBTransaction, &mut FileContainer, u16)>
+    {
+        let f = fuid.to_vec();
+        for tx in &mut self.transactions {
+            let found = match tx.type_data {
+                Some(SMBTransactionTypeData::FILE(ref mut d)) => {
+                    direction == d.direction && f == d.fuid && !d.file_tracker.is_done()
+                },
+                _ => { false },
+            };
+
+            if found {
+                SCLogDebug!("SMB: Found SMB file TX with ID {}", tx.id);
+                let (files, flags) = self.files.get(direction);
+                return Some((tx, files, flags));
+            }
+        }
+        SCLogDebug!("SMB: Failed to find SMB TX with FUID {:?}", fuid);
+        return None;
+    }
+
+    /// get file tx for a fuid. File may already have been closed.
+    pub fn get_file_tx_by_fuid(&mut self, fuid: &[u8], direction: u8)
         -> Option<(&mut SMBTransaction, &mut FileContainer, u16)>
     {
         let f = fuid.to_vec();
diff --git a/rust/src/smb/smb1.rs b/rust/src/smb/smb1.rs
index 2505c03707..f018a3858c 100644
--- a/rust/src/smb/smb1.rs
+++ b/rust/src/smb/smb1.rs
@@ -897,7 +897,7 @@ pub fn smb1_write_request_record<'b>(state: &mut SMBState, r: &SmbRecord<'b>)
                 None => b"<unknown>".to_vec(),
             };
             let mut set_event_fileoverlap = false;
-            let found = match state.get_file_tx_by_fuid(&file_fid, STREAM_TOSERVER) {
+            let found = match state.get_file_tx_by_fuid_with_open_file(&file_fid, STREAM_TOSERVER) {
                 Some((tx, files, flags)) => {
                     let file_id : u32 = tx.id as u32;
                     if let Some(SMBTransactionTypeData::FILE(ref mut tdf)) = tx.type_data {
@@ -990,7 +990,7 @@ pub fn smb1_read_response_record<'b>(state: &mut SMBState, r: &SmbRecord<'b>)
                         None => Vec::new(),
                     };
                     let mut set_event_fileoverlap = false;
-                    let found = match state.get_file_tx_by_fuid(&file_fid, STREAM_TOCLIENT) {
+                    let found = match state.get_file_tx_by_fuid_with_open_file(&file_fid, STREAM_TOCLIENT) {
                         Some((tx, files, flags)) => {
                             if let Some(SMBTransactionTypeData::FILE(ref mut tdf)) = tx.type_data {
                                 let file_id : u32 = tx.id as u32;
diff --git a/rust/src/smb/smb2.rs b/rust/src/smb/smb2.rs
index 82f24f0084..84d8978cd6 100644
--- a/rust/src/smb/smb2.rs
+++ b/rust/src/smb/smb2.rs
@@ -156,7 +156,7 @@ pub fn smb2_read_response_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>)
 
             let mut set_event_fileoverlap = false;
             // look up existing tracker and if we have it update it
-            let found = match state.get_file_tx_by_fuid(&file_guid, STREAM_TOCLIENT) {
+            let found = match state.get_file_tx_by_fuid_with_open_file(&file_guid, STREAM_TOCLIENT) {
                 Some((tx, files, flags)) => {
                     if let Some(SMBTransactionTypeData::FILE(ref mut tdf)) = tx.type_data {
                         let file_id : u32 = tx.id as u32;
@@ -297,7 +297,7 @@ pub fn smb2_write_request_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>)
             };
 
             let mut set_event_fileoverlap = false;
-            let found = match state.get_file_tx_by_fuid(&file_guid, STREAM_TOSERVER) {
+            let found = match state.get_file_tx_by_fuid_with_open_file(&file_guid, STREAM_TOSERVER) {
                 Some((tx, files, flags)) => {
                     if let Some(SMBTransactionTypeData::FILE(ref mut tdf)) = tx.type_data {
                         let file_id : u32 = tx.id as u32;