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

diff --git a/rules/smb-events.rules b/rules/smb-events.rules
index 248d75e4b575a4c2802fa4eed84f43dceecd3405..745c2ea2d2f70d35dc69b1931137b3cc23f08ca2 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 b36b465db51baaee9b1bd67ec9f746d2f42bd9f5..aeb7102a6fab1e9b8c47231eca0f125fc3c2249c 100644 (file)
--- a/rust/src/smb/smb.rs
+++ b/rust/src/smb/smb.rs
@@ -1306,8 +1306,12 @@ impl SMBState {
                                 if is_pipe {
                                     return 0;
                                 }
-                                smb1_write_request_record(self, r, SMB1_HEADER_SIZE, SMB1_COMMAND_WRITE_ANDX);
-                                
+                                // 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, SMB1_HEADER_SIZE, SMB1_COMMAND_WRITE_ANDX, nbss_remaining);
+
                                 self.add_nbss_ts_frames(flow, stream_slice, input, nbss_part_hdr.length as i64);
                                 self.add_smb1_ts_pdu_frame(flow, stream_slice, nbss_part_hdr.data, nbss_part_hdr.length as i64);
                                 self.add_smb1_ts_hdr_data_frames(flow, stream_slice, nbss_part_hdr.data, nbss_part_hdr.length as i64);
@@ -1322,8 +1326,12 @@ 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);
+
                                 self.add_nbss_ts_frames(flow, stream_slice, input, nbss_part_hdr.length as i64);
                                 self.add_smb2_ts_pdu_frame(flow, stream_slice, nbss_part_hdr.data, nbss_part_hdr.length as i64);
                                 self.add_smb2_ts_hdr_data_frames(flow, stream_slice, nbss_part_hdr.data, nbss_part_hdr.length as i64, smb_record.header_len as i64);
@@ -1641,7 +1649,11 @@ impl SMBState {
                                 self.add_smb1_tc_pdu_frame(flow, stream_slice, nbss_part_hdr.data, nbss_part_hdr.length as i64);
                                 self.add_smb1_tc_hdr_data_frames(flow, stream_slice, nbss_part_hdr.data, nbss_part_hdr.length as i64);
 
-                                smb1_read_response_record(self, r, SMB1_HEADER_SIZE);
+                                // 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, SMB1_HEADER_SIZE, nbss_remaining);
                                 let consumed = input.len() - output.len();
                                 return consumed;
                             }
@@ -1658,7 +1670,11 @@ impl SMBState {
                                 self.add_smb2_tc_pdu_frame(flow, stream_slice, nbss_part_hdr.data, nbss_part_hdr.length as i64);
                                 self.add_smb2_tc_hdr_data_frames(flow, stream_slice, nbss_part_hdr.data, nbss_part_hdr.length as i64, smb_record.header_len as i64);
 
-                                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 c6644615572edee8af004cb7927acf51b3d12a8f..bbe18921ae34ca5ea8172e1d8b1849316f810dc0 100644 (file)
--- a/rust/src/smb/smb1.rs
+++ b/rust/src/smb/smb1.rs
@@ -417,7 +417,7 @@ fn smb1_request_record_one(state: &mut SMBState, r: &SmbRecord, command: u8, and
         SMB1_COMMAND_WRITE_ANDX |
         SMB1_COMMAND_WRITE |
         SMB1_COMMAND_WRITE_AND_CLOSE => {
-            smb1_write_request_record(state, r, *andx_offset, command);
+            smb1_write_request_record(state, r, *andx_offset, command, 0);
             true // tx handling in func
         },
         SMB1_COMMAND_TRANS => {
@@ -617,7 +617,7 @@ fn smb1_response_record_one(state: &mut SMBState, r: &SmbRecord, command: u8, an
 
     let have_tx = match command {
         SMB1_COMMAND_READ_ANDX => {
-            smb1_read_response_record(state, r, *andx_offset);
+            smb1_read_response_record(state, r, *andx_offset, 0);
             true // tx handling in func
         },
         SMB1_COMMAND_NEGOTIATE_PROTOCOL => {
@@ -932,7 +932,7 @@ pub fn smb1_trans_response_record(state: &mut SMBState, r: &SmbRecord)
 }
 
 /// Handle WRITE, WRITE_ANDX, WRITE_AND_CLOSE request records
-pub fn smb1_write_request_record(state: &mut SMBState, r: &SmbRecord, andx_offset: usize, command: u8)
+pub fn smb1_write_request_record(state: &mut SMBState, r: &SmbRecord, andx_offset: usize, command: u8, nbss_remaining: u32)
 {
     let mut events : Vec<SMBEvent> = Vec::new();
 
@@ -946,7 +946,13 @@ pub fn smb1_write_request_record(state: &mut SMBState, r: &SmbRecord, andx_offse
     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(Direction::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 {}",
@@ -1019,7 +1025,7 @@ pub fn smb1_write_request_record(state: &mut SMBState, r: &SmbRecord, andx_offse
     smb1_request_record_generic(state, r, events);
 }
 
-pub fn smb1_read_response_record(state: &mut SMBState, r: &SmbRecord, andx_offset: usize)
+pub fn smb1_read_response_record(state: &mut SMBState, r: &SmbRecord, andx_offset: usize, nbss_remaining: u32)
 {
     let mut events : Vec<SMBEvent> = Vec::new();
 
@@ -1027,7 +1033,13 @@ pub fn smb1_read_response_record(state: &mut SMBState, r: &SmbRecord, andx_offse
         match parse_smb_read_andx_response_record(&r.data[andx_offset-SMB1_HEADER_SIZE..]) {
             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(Direction::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 685aae487bf5ae897d8e1c1be1f33b05da222961..36c31c577b85ab5968447743b3d0b1c7deb74d13 100644 (file)
--- a/rust/src/smb/smb2.rs
+++ b/rust/src/smb/smb2.rs
@@ -113,7 +113,7 @@ fn smb2_read_response_record_generic(state: &mut SMBState, r: &Smb2Record)
     }
 }
 
-pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record)
+pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record, 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 };
@@ -122,6 +122,13 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record)
 
     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(Direction::ToClient, nbss_remaining, 0);
+                return;
+            }
             if r.nt_status == SMB_NTSTATUS_BUFFER_OVERFLOW {
                 SCLogDebug!("SMBv2/READ: incomplete record, expecting a follow up");
                 // fall through
@@ -264,7 +271,7 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record)
     }
 }
 
-pub fn smb2_write_request_record(state: &mut SMBState, r: &Smb2Record)
+pub fn smb2_write_request_record(state: &mut SMBState, r: &Smb2Record, 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 };
@@ -277,6 +284,13 @@ pub fn smb2_write_request_record(state: &mut SMBState, r: &Smb2Record)
     }
     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(Direction::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);
@@ -580,7 +594,7 @@ pub fn smb2_request_record(state: &mut SMBState, r: &Smb2Record)
             }
         },
         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 => {
@@ -684,7 +698,7 @@ pub fn smb2_response_record(state: &mut SMBState, r: &Smb2Record)
         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 {