From: Philippe Antoine Date: Tue, 27 Dec 2022 16:34:47 +0000 (+0100) Subject: smb: checks against nbss records length X-Git-Tag: suricata-7.0.0-rc2~582 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c1b7befb18a974eae05b25fad91d9ba84a65ab50;p=thirdparty%2Fsuricata.git 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 248d75e4b5..745c2ea2d2 100644 --- 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 b36b465db5..aeb7102a6f 100644 --- 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 c664461557..bbe18921ae 100644 --- 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 = 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 = 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 685aae487b..36c31c577b 100644 --- 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 {