From: Philippe Antoine Date: Tue, 27 Dec 2022 16:34:47 +0000 (+0100) Subject: smb: checks against nbss records length X-Git-Tag: suricata-6.0.11~46 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0bf3ab9e6defee477e4f881454603fed1aa7c477;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 (cherry picked from commit c1b7befb18a974eae05b25fad91d9ba84a65ab50) --- diff --git a/rules/smb-events.rules b/rules/smb-events.rules index 159033f898..502d500507 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 cb605589a5..2e194e7f33 100644 --- 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 f018a3858c..0de13a5052 100644 --- 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 = 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 = 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 84d8978cd6..af3bf174b2 100644 --- 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 {