smb: checks against nbss records length

When Suricata handles files over SMB, it does not wait for the
NBSS record to be complete, and can stream the payload to the
file... But it did not check the consistency of the SMB record
length being read or written against the NBSS record length.

This could lead to an evasion where an attacker crafts a SMB
write with a too big Length field, and then sends its evil
payload, even if the server returned an error for the write request.

Ticket: #5770
(cherry picked from commit c1b7befb18a974eae05b25fad91d9ba84a65ab50)

diff --git a/rules/smb-events.rules b/rules/smb-events.rules
index 159033f898c0be9f0d589877308464d27fc4cd0a..502d5005073852ad3a9262b889273a4845a955a5 100644 (file)
--- a/rules/smb-events.rules
+++ b/rules/smb-events.rules
@@ -26,9 +26,9 @@ alert smb any any -> any any (msg:"SURICATA SMB max response READ size exceeded"
 # checks negotiated max-write-size and 'app-layer.protocols.smb.max-write-size`
 alert smb any any -> any any (msg:"SURICATA SMB max WRITE size exceeded"; flow:to_server; app-layer-event:smb.write_request_too_large; classtype:protocol-command-decode; sid:2225011; rev:1;)
 
-# checks 'app-layer.protocols.smb.max-read-size` against NEGOTIATE PROTOCOL response
+# checks 'app-layer.protocols.smb.max-read-size`, NEGOTIATE PROTOCOL response, and NBSS record length against SMB read data length
 alert smb any any -> any any (msg:"SURICATA SMB supported READ size exceeded"; flow:to_client; app-layer-event:smb.negotiate_max_read_size_too_large; classtype:protocol-command-decode; sid:2225012; rev:1;)
-# checks 'app-layer.protocols.smb.max-write-size` against NEGOTIATE PROTOCOL response
+# checks 'app-layer.protocols.smb.max-write-size`, NEGOTIATE PROTOCOL response, NBSS record length against SMB write data length
 alert smb any any -> any any (msg:"SURICATA SMB supported WRITE size exceeded"; flow:to_server; app-layer-event:smb.negotiate_max_write_size_too_large; classtype:protocol-command-decode; sid:2225013; rev:1;)
 
 # checks 'app-layer.protocols.smb.max-write-queue-size` against out of order chunks

diff --git a/rust/src/smb/smb.rs b/rust/src/smb/smb.rs
index cb605589a52d020b1f14bfa852604f8a968a45f9..2e194e7f339894bdec06b168782f14b9bcd7e3df 100644 (file)
--- a/rust/src/smb/smb.rs
+++ b/rust/src/smb/smb.rs
@@ -1352,7 +1352,11 @@ impl SMBState {
                                             if is_pipe {
                                                 return 0;
                                             }
-                                            smb1_write_request_record(self, r);
+                                            // how many more bytes are expected within this NBSS record
+                                            // So that we can check that further parsed offsets and lengths
+                                            // stay within the NBSS record.
+                                            let nbss_remaining = nbss_part_hdr.length - nbss_part_hdr.data.len() as u32;
+                                            smb1_write_request_record(self, r, nbss_remaining);
                                             let consumed = input.len() - output.len();
                                             return consumed;
                                         }
@@ -1368,7 +1372,11 @@ impl SMBState {
                                         SCLogDebug!("SMB2: partial record {}",
                                                 &smb2_command_string(smb_record.command));
                                         if smb_record.command == SMB2_COMMAND_WRITE {
-                                            smb2_write_request_record(self, smb_record);
+                                            // how many more bytes are expected within this NBSS record
+                                            // So that we can check that further parsed offsets and lengths
+                                            // stay within the NBSS record.
+                                            let nbss_remaining = nbss_part_hdr.length - nbss_part_hdr.data.len() as u32;
+                                            smb2_write_request_record(self, smb_record, nbss_remaining);
                                             let consumed = input.len() - output.len();
                                             SCLogDebug!("consumed {}", consumed);
                                             return consumed;
@@ -1605,7 +1613,11 @@ impl SMBState {
                                             if is_pipe {
                                                 return 0;
                                             }
-                                            smb1_read_response_record(self, r);
+                                            // how many more bytes are expected within this NBSS record
+                                            // So that we can check that further parsed offsets and lengths
+                                            // stay within the NBSS record.
+                                            let nbss_remaining = nbss_part_hdr.length - nbss_part_hdr.data.len() as u32;
+                                            smb1_read_response_record(self, r, nbss_remaining);
                                             let consumed = input.len() - output.len();
                                             return consumed;
                                         }
@@ -1619,7 +1631,11 @@ impl SMBState {
                                         SCLogDebug!("SMB2: partial record {}",
                                                 &smb2_command_string(smb_record.command));
                                         if smb_record.command == SMB2_COMMAND_READ {
-                                            smb2_read_response_record(self, smb_record);
+                                            // how many more bytes are expected within this NBSS record
+                                            // So that we can check that further parsed offsets and lengths
+                                            // stay within the NBSS record.
+                                            let nbss_remaining = nbss_part_hdr.length - nbss_part_hdr.data.len() as u32;
+                                            smb2_read_response_record(self, smb_record, nbss_remaining);
                                             let consumed = input.len() - output.len();
                                             return consumed;
                                         }

diff --git a/rust/src/smb/smb1.rs b/rust/src/smb/smb1.rs
index f018a3858cb3ebc1e2f87e97ff592453e1bcc1e5..0de13a5052569c35f0be2023fcc91b96d6c001da 100644 (file)
--- a/rust/src/smb/smb1.rs
+++ b/rust/src/smb/smb1.rs
@@ -396,7 +396,7 @@ pub fn smb1_request_record<'b>(state: &mut SMBState, r: &SmbRecord<'b>) -> u32 {
         SMB1_COMMAND_WRITE_ANDX |
         SMB1_COMMAND_WRITE |
         SMB1_COMMAND_WRITE_AND_CLOSE => {
-            smb1_write_request_record(state, r);
+            smb1_write_request_record(state, r, 0);
             true // tx handling in func
         },
         SMB1_COMMAND_TRANS => {
@@ -572,7 +572,7 @@ pub fn smb1_response_record<'b>(state: &mut SMBState, r: &SmbRecord<'b>) -> u32
 
     let have_tx = match r.command {
         SMB1_COMMAND_READ_ANDX => {
-            smb1_read_response_record(state, &r);
+            smb1_read_response_record(state, &r, 0);
             true // tx handling in func
         },
         SMB1_COMMAND_NEGOTIATE_PROTOCOL => {
@@ -872,7 +872,7 @@ pub fn smb1_trans_response_record<'b>(state: &mut SMBState, r: &SmbRecord<'b>)
 }
 
 /// Handle WRITE, WRITE_ANDX, WRITE_AND_CLOSE request records
-pub fn smb1_write_request_record<'b>(state: &mut SMBState, r: &SmbRecord<'b>)
+pub fn smb1_write_request_record<'b>(state: &mut SMBState, r: &SmbRecord<'b>, nbss_remaining: u32)
 {
     let mut events : Vec<SMBEvent> = Vec::new();
 
@@ -886,7 +886,13 @@ pub fn smb1_write_request_record<'b>(state: &mut SMBState, r: &SmbRecord<'b>)
     match result {
         Ok((_, rd)) => {
             SCLogDebug!("SMBv1: write andx => {:?}", rd);
-
+            if rd.len > rd.data.len() as u32 + nbss_remaining {
+                // Record claims more bytes than are in NBSS record...
+                state.set_event(SMBEvent::WriteRequestTooLarge);
+                // Skip the remaining bytes of the record.
+                state.set_skip(STREAM_TOSERVER, nbss_remaining, 0);
+                return;
+            }
             let mut file_fid = rd.fid.to_vec();
             file_fid.extend_from_slice(&u32_as_bytes(r.ssn_id));
             SCLogDebug!("SMBv1 WRITE: FID {:?} offset {}",
@@ -958,7 +964,7 @@ pub fn smb1_write_request_record<'b>(state: &mut SMBState, r: &SmbRecord<'b>)
     smb1_request_record_generic(state, r, events);
 }
 
-pub fn smb1_read_response_record<'b>(state: &mut SMBState, r: &SmbRecord<'b>)
+pub fn smb1_read_response_record<'b>(state: &mut SMBState, r: &SmbRecord<'b>, nbss_remaining: u32)
 {
     let mut events : Vec<SMBEvent> = Vec::new();
 
@@ -966,7 +972,13 @@ pub fn smb1_read_response_record<'b>(state: &mut SMBState, r: &SmbRecord<'b>)
         match parse_smb_read_andx_response_record(r.data) {
             Ok((_, rd)) => {
                 SCLogDebug!("SMBv1: read response => {:?}", rd);
-
+                if rd.len > nbss_remaining + rd.data.len() as u32 {
+                    // Record claims more bytes than are in NBSS record...
+                    state.set_event(SMBEvent::ReadResponseTooLarge);
+                    // Skip the remaining bytes of the record.
+                    state.set_skip(STREAM_TOCLIENT, nbss_remaining, 0);
+                    return;
+                }
                 let fid_key = SMBCommonHdr::from1(r, SMBHDR_TYPE_OFFSET);
                 let (offset, file_fid) = match state.ssn2vecoffset_map.remove(&fid_key) {
                     Some(o) => (o.offset, o.guid),

diff --git a/rust/src/smb/smb2.rs b/rust/src/smb/smb2.rs
index 84d8978cd6a945fa80b7ca74003eb316172cf071..af3bf174b2057e31be2cb27739aacc042728b2fc 100644 (file)
--- a/rust/src/smb/smb2.rs
+++ b/rust/src/smb/smb2.rs
@@ -112,7 +112,7 @@ fn smb2_read_response_record_generic<'b>(state: &mut SMBState, r: &Smb2Record<'b
     }
 }
 
-pub fn smb2_read_response_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>)
+pub fn smb2_read_response_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>, nbss_remaining: u32)
 {
     let max_queue_size = unsafe { SMB_CFG_MAX_READ_QUEUE_SIZE };
     let max_queue_cnt = unsafe { SMB_CFG_MAX_READ_QUEUE_CNT };
@@ -121,6 +121,13 @@ pub fn smb2_read_response_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>)
 
     match parse_smb2_response_read(r.data) {
         Ok((_, rd)) => {
+            if rd.len - rd.data.len() as u32 > nbss_remaining {
+                // Record claims more bytes than are in NBSS record...
+                state.set_event(SMBEvent::ReadResponseTooLarge);
+                // Skip the remaining bytes of the record.
+                state.set_skip(STREAM_TOCLIENT, nbss_remaining, 0);
+                return;
+            }
             if r.nt_status == SMB_NTSTATUS_BUFFER_OVERFLOW {
                 SCLogDebug!("SMBv2/READ: incomplete record, expecting a follow up");
                 // fall through
@@ -266,7 +273,7 @@ pub fn smb2_read_response_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>)
     }
 }
 
-pub fn smb2_write_request_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>)
+pub fn smb2_write_request_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>, nbss_remaining: u32)
 {
     let max_queue_size = unsafe { SMB_CFG_MAX_WRITE_QUEUE_SIZE };
     let max_queue_cnt = unsafe { SMB_CFG_MAX_WRITE_QUEUE_CNT };
@@ -279,6 +286,13 @@ pub fn smb2_write_request_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>)
     }
     match parse_smb2_request_write(r.data) {
         Ok((_, wr)) => {
+            if wr.wr_len - wr.data.len() as u32 > nbss_remaining {
+                // Record claims more bytes than are in NBSS record...
+                state.set_event(SMBEvent::WriteRequestTooLarge);
+                // Skip the remaining bytes of the record.
+                state.set_skip(STREAM_TOSERVER, nbss_remaining, 0);
+                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);
@@ -559,7 +573,7 @@ pub fn smb2_request_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>)
             }
         },
         SMB2_COMMAND_WRITE => {
-            smb2_write_request_record(state, &r);
+            smb2_write_request_record(state, &r, 0);
             true // write handling creates both file tx and generic tx
         },
         SMB2_COMMAND_CLOSE => {
@@ -665,7 +679,7 @@ pub fn smb2_response_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>)
         SMB2_COMMAND_READ => {
             if r.nt_status == SMB_NTSTATUS_SUCCESS ||
                r.nt_status == SMB_NTSTATUS_BUFFER_OVERFLOW {
-                smb2_read_response_record(state, &r);
+                smb2_read_response_record(state, &r, 0);
                 false
 
             } else if r.nt_status == SMB_NTSTATUS_END_OF_FILE {