From 233ab1114873e632014b58d91c351505417fe8ed Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Wed, 18 Jan 2023 16:47:56 +0100 Subject: [PATCH] smb: handles records with trailing nbss data If a file (read/write) SMB record has padding/trailing data after the buffer being read or written, and that Suricata falls in one case where it skips the data, it should skip until the very end of the NBSS record, meaning it should also skip the padding/trailing data. Otherwise, an attacker may smuggle some NBSS/SMB record in this trailing data, that will be interpreted by Suricata, but not by the SMB client/server, leading to evasions. Ticket: #5786 --- rust/src/smb/smb.rs | 7 +++---- rust/src/smb/smb1.rs | 6 +++--- rust/src/smb/smb2.rs | 32 ++++++++++++++++---------------- 3 files changed, 22 insertions(+), 23 deletions(-) diff --git a/rust/src/smb/smb.rs b/rust/src/smb/smb.rs index aeb7102a6f..6acc948991 100644 --- a/rust/src/smb/smb.rs +++ b/rust/src/smb/smb.rs @@ -1172,13 +1172,12 @@ impl SMBState { } } - pub fn set_skip(&mut self, direction: Direction, rec_size: u32, data_size: u32) + pub fn set_skip(&mut self, direction: Direction, nbss_remaining: u32) { - let skip = rec_size.saturating_sub(data_size); if direction == Direction::ToServer { - self.skip_ts = skip; + self.skip_ts = nbss_remaining; } else { - self.skip_tc = skip; + self.skip_tc = nbss_remaining; } } diff --git a/rust/src/smb/smb1.rs b/rust/src/smb/smb1.rs index bbe18921ae..378ed856e5 100644 --- a/rust/src/smb/smb1.rs +++ b/rust/src/smb/smb1.rs @@ -950,7 +950,7 @@ pub fn smb1_write_request_record(state: &mut SMBState, r: &SmbRecord, andx_offse // 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); + state.set_skip(Direction::ToServer, nbss_remaining); return; } let mut file_fid = rd.fid.to_vec(); @@ -1037,7 +1037,7 @@ pub fn smb1_read_response_record(state: &mut SMBState, r: &SmbRecord, andx_offse // 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); + state.set_skip(Direction::ToClient, nbss_remaining); return; } let fid_key = SMBCommonHdr::from1(r, SMBHDR_TYPE_OFFSET); @@ -1046,7 +1046,7 @@ pub fn smb1_read_response_record(state: &mut SMBState, r: &SmbRecord, andx_offse None => { SCLogDebug!("SMBv1 READ response: reply to unknown request: left {} {:?}", rd.len - rd.data.len() as u32, rd); - state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32); + state.set_skip(Direction::ToClient, nbss_remaining); return; }, }; diff --git a/rust/src/smb/smb2.rs b/rust/src/smb/smb2.rs index 36c31c577b..a2fe021b02 100644 --- a/rust/src/smb/smb2.rs +++ b/rust/src/smb/smb2.rs @@ -126,7 +126,7 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record, nbss_rema // 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); + state.set_skip(Direction::ToClient, nbss_remaining); return; } if r.nt_status == SMB_NTSTATUS_BUFFER_OVERFLOW { @@ -135,7 +135,7 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record, nbss_rema } else if r.nt_status != SMB_NTSTATUS_SUCCESS { SCLogDebug!("SMBv2: read response error code received: skip record"); - state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32); + state.set_skip(Direction::ToClient, nbss_remaining); return; } @@ -143,7 +143,7 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record, nbss_rema (unsafe { SMB_CFG_MAX_READ_SIZE != 0 && SMB_CFG_MAX_READ_SIZE < rd.len }) { state.set_event(SMBEvent::ReadResponseTooLarge); - state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32); + state.set_skip(Direction::ToClient, nbss_remaining); return; } @@ -156,7 +156,7 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record, nbss_rema Some(o) => (o.offset, o.guid), None => { SCLogDebug!("SMBv2 READ response: reply to unknown request {:?}",rd); - state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32); + state.set_skip(Direction::ToClient, nbss_remaining); return; }, }; @@ -173,10 +173,10 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record, nbss_rema } if max_queue_size != 0 && tdf.file_tracker.get_inflight_size() + rd.len as u64 > max_queue_size.into() { state.set_event(SMBEvent::ReadQueueSizeExceeded); - state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32); + state.set_skip(Direction::ToClient, nbss_remaining); } else if max_queue_cnt != 0 && tdf.file_tracker.get_inflight_cnt() >= max_queue_cnt as usize { state.set_event(SMBEvent::ReadQueueCntExceeded); - state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32); + state.set_skip(Direction::ToClient, nbss_remaining); } else { filetracker_newchunk(&mut tdf.file_tracker, &tdf.file_name, rd.data, offset, @@ -227,7 +227,7 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record, nbss_rema smb_read_dcerpc_record(state, vercmd, hdr, &file_guid, rd.data); } else if is_pipe { SCLogDebug!("non-DCERPC pipe"); - state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32); + state.set_skip(Direction::ToClient, nbss_remaining); } else { let file_name = match state.guid2name_map.get(&file_guid) { Some(n) => { n.to_vec() } @@ -246,10 +246,10 @@ pub fn smb2_read_response_record(state: &mut SMBState, r: &Smb2Record, nbss_rema } if max_queue_size != 0 && tdf.file_tracker.get_inflight_size() + rd.len as u64 > max_queue_size.into() { state.set_event(SMBEvent::ReadQueueSizeExceeded); - state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32); + state.set_skip(Direction::ToClient, nbss_remaining); } else if max_queue_cnt != 0 && tdf.file_tracker.get_inflight_cnt() >= max_queue_cnt as usize { state.set_event(SMBEvent::ReadQueueCntExceeded); - state.set_skip(Direction::ToClient, rd.len, rd.data.len() as u32); + state.set_skip(Direction::ToClient, nbss_remaining); } else { filetracker_newchunk(&mut tdf.file_tracker, &file_name, rd.data, offset, @@ -288,13 +288,13 @@ pub fn smb2_write_request_record(state: &mut SMBState, r: &Smb2Record, nbss_rema // 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); + state.set_skip(Direction::ToServer, nbss_remaining); 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); - state.set_skip(Direction::ToServer, wr.wr_len, wr.data.len() as u32); + state.set_skip(Direction::ToServer, nbss_remaining); return; } @@ -318,10 +318,10 @@ pub fn smb2_write_request_record(state: &mut SMBState, r: &Smb2Record, nbss_rema } if max_queue_size != 0 && tdf.file_tracker.get_inflight_size() + wr.wr_len as u64 > max_queue_size.into() { state.set_event(SMBEvent::WriteQueueSizeExceeded); - state.set_skip(Direction::ToServer, wr.wr_len, wr.data.len() as u32); + state.set_skip(Direction::ToServer, nbss_remaining); } else if max_queue_cnt != 0 && tdf.file_tracker.get_inflight_cnt() >= max_queue_cnt as usize { state.set_event(SMBEvent::WriteQueueCntExceeded); - state.set_skip(Direction::ToServer, wr.wr_len, wr.data.len() as u32); + state.set_skip(Direction::ToServer, nbss_remaining); } else { filetracker_newchunk(&mut tdf.file_tracker, &file_name, wr.data, wr.wr_offset, @@ -372,7 +372,7 @@ pub fn smb2_write_request_record(state: &mut SMBState, r: &Smb2Record, nbss_rema smb_write_dcerpc_record(state, vercmd, hdr, wr.data); } else if is_pipe { SCLogDebug!("non-DCERPC pipe: skip rest of the record"); - state.set_skip(Direction::ToServer, wr.wr_len, wr.data.len() as u32); + state.set_skip(Direction::ToServer, nbss_remaining); } else { let tx = state.new_file_tx(&file_guid, &file_name, Direction::ToServer); tx.vercmd.set_smb2_cmd(SMB2_COMMAND_WRITE); @@ -386,10 +386,10 @@ pub fn smb2_write_request_record(state: &mut SMBState, r: &Smb2Record, nbss_rema if max_queue_size != 0 && tdf.file_tracker.get_inflight_size() + wr.wr_len as u64 > max_queue_size.into() { state.set_event(SMBEvent::WriteQueueSizeExceeded); - state.set_skip(Direction::ToServer, wr.wr_len, wr.data.len() as u32); + state.set_skip(Direction::ToServer, nbss_remaining); } else if max_queue_cnt != 0 && tdf.file_tracker.get_inflight_cnt() >= max_queue_cnt as usize { state.set_event(SMBEvent::WriteQueueCntExceeded); - state.set_skip(Direction::ToServer, wr.wr_len, wr.data.len() as u32); + state.set_skip(Direction::ToServer, nbss_remaining); } else { filetracker_newchunk(&mut tdf.file_tracker, &file_name, wr.data, wr.wr_offset,