From 8b570c0293f1d7b1c1796b8aac51187625de9e85 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Thu, 20 Dec 2018 09:11:21 +0100 Subject: [PATCH] smb: improve request/response mapping Only use ssn_id and msg_id for mapping a response to a request. By not using the tree_id it can always be included in the tx.hdr which means it can be logged properly in case of IOCTL and DCERPC. --- rust/src/smb/dcerpc.rs | 2 +- rust/src/smb/session.rs | 2 +- rust/src/smb/smb.rs | 14 ++++++++++---- rust/src/smb/smb2_ioctl.rs | 20 ++++++-------------- 4 files changed, 18 insertions(+), 20 deletions(-) diff --git a/rust/src/smb/dcerpc.rs b/rust/src/smb/dcerpc.rs index 9b71f13cbd..9d9536d2fb 100644 --- a/rust/src/smb/dcerpc.rs +++ b/rust/src/smb/dcerpc.rs @@ -232,7 +232,7 @@ impl SMBState { SCLogDebug!("looking for {:?}", dce_hdr); for tx in &mut self.transactions { - let found = dce_hdr == tx.hdr.to_dcerpc(vercmd) && + let found = dce_hdr.compare(&tx.hdr.to_dcerpc(vercmd)) && match tx.type_data { Some(SMBTransactionTypeData::DCERPC(ref x)) => { x.call_id == call_id diff --git a/rust/src/smb/session.rs b/rust/src/smb/session.rs index 741a0d63d0..9f35c6d7c7 100644 --- a/rust/src/smb/session.rs +++ b/rust/src/smb/session.rs @@ -62,7 +62,7 @@ impl SMBState { -> Option<&mut SMBTransaction> { for tx in &mut self.transactions { - let hit = tx.hdr == hdr && match tx.type_data { + let hit = tx.hdr.compare(&hdr) && match tx.type_data { Some(SMBTransactionTypeData::SESSIONSETUP(_)) => { true }, _ => { false }, }; diff --git a/rust/src/smb/smb.rs b/rust/src/smb/smb.rs index 9f2f1d42e4..82d2d5ea83 100644 --- a/rust/src/smb/smb.rs +++ b/rust/src/smb/smb.rs @@ -701,6 +701,12 @@ impl SMBCommonHdr { msg_id : msg_id, } } + + // don't include tree id + pub fn compare(&self, hdr: &SMBCommonHdr) -> bool { + (self.rec_type == hdr.rec_type && self.ssn_id == hdr.ssn_id && + self.msg_id == hdr.msg_id) + } } #[derive(Hash, Eq, PartialEq, Debug)] @@ -973,10 +979,10 @@ impl SMBState { let found = if tx.vercmd.get_version() == smb_ver { if smb_ver == 1 { let (_, cmd) = tx.vercmd.get_smb1_cmd(); - cmd as u16 == smb_cmd && tx.hdr == *key + cmd as u16 == smb_cmd && tx.hdr.compare(key) } else if smb_ver == 2 { let (_, cmd) = tx.vercmd.get_smb2_cmd(); - cmd == smb_cmd && tx.hdr == *key + cmd == smb_cmd && tx.hdr.compare(key) } else { false } @@ -1054,7 +1060,7 @@ impl SMBState { -> Option<&mut SMBTransaction> { for tx in &mut self.transactions { - let hit = tx.hdr == hdr && match tx.type_data { + let hit = tx.hdr.compare(&hdr) && match tx.type_data { Some(SMBTransactionTypeData::TREECONNECT(_)) => { true }, _ => { false }, }; @@ -1090,7 +1096,7 @@ impl SMBState { for tx in &mut self.transactions { let found = match tx.type_data { Some(SMBTransactionTypeData::CREATE(ref _d)) => { - *hdr == tx.hdr + tx.hdr.compare(&hdr) }, _ => { false }, }; diff --git a/rust/src/smb/smb2_ioctl.rs b/rust/src/smb/smb2_ioctl.rs index f06941648d..43ce24270e 100644 --- a/rust/src/smb/smb2_ioctl.rs +++ b/rust/src/smb/smb2_ioctl.rs @@ -59,14 +59,13 @@ impl SMBState { // IOCTL responses ASYNC don't set the tree id pub fn smb2_ioctl_request_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>) { + let hdr = SMBCommonHdr::from2(r, SMBHDR_TYPE_HEADER); match parse_smb2_request_ioctl(r.data) { IResult::Done(_, rd) => { SCLogDebug!("IOCTL request data: {:?}", rd); let is_dcerpc = rd.is_pipe && match state.get_service_for_guid(&rd.guid) { (_, x) => x, }; - let hdr = SMBCommonHdr::new(SMBHDR_TYPE_HEADER, - r.session_id, 0, r.message_id); if is_dcerpc { SCLogDebug!("IOCTL request data is_pipe. Calling smb_write_dcerpc_record"); let vercmd = SMBVerCmdStat::new2(SMB2_COMMAND_IOCTL); @@ -78,8 +77,6 @@ pub fn smb2_ioctl_request_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>) } }, _ => { - let hdr = SMBCommonHdr::new(SMBHDR_TYPE_HEADER, - r.session_id, 0, r.message_id); let tx = state.new_generic_tx(2, r.command, hdr); tx.set_event(SMBEvent::MalformedData); }, @@ -89,6 +86,7 @@ pub fn smb2_ioctl_request_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>) // IOCTL responses ASYNC don't set the tree id pub fn smb2_ioctl_response_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>) { + let hdr = SMBCommonHdr::from2(r, SMBHDR_TYPE_HEADER); match parse_smb2_response_ioctl(r.data) { IResult::Done(_, rd) => { SCLogDebug!("IOCTL response data: {:?}", rd); @@ -98,16 +96,12 @@ pub fn smb2_ioctl_response_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>) }; if is_dcerpc { SCLogDebug!("IOCTL response data is_pipe. Calling smb_read_dcerpc_record"); - let hdr = SMBCommonHdr::new(SMBHDR_TYPE_HEADER, - r.session_id, 0, r.message_id); let vercmd = SMBVerCmdStat::new2_with_ntstatus(SMB2_COMMAND_IOCTL, r.nt_status); SCLogDebug!("TODO passing empty GUID"); smb_read_dcerpc_record(state, vercmd, hdr, &[],rd.data); } else { - let tx_key = SMBCommonHdr::new(SMBHDR_TYPE_HEADER, - r.session_id, 0, r.message_id); - SCLogDebug!("SMB2_COMMAND_IOCTL/SMB_NTSTATUS_PENDING looking for {:?}", tx_key); - match state.get_generic_tx(2, SMB2_COMMAND_IOCTL, &tx_key) { + SCLogDebug!("SMB2_COMMAND_IOCTL/SMB_NTSTATUS_PENDING looking for {:?}", hdr); + match state.get_generic_tx(2, SMB2_COMMAND_IOCTL, &hdr) { Some(tx) => { tx.set_status(r.nt_status, false); if r.nt_status != SMB_NTSTATUS_PENDING { @@ -119,10 +113,8 @@ pub fn smb2_ioctl_response_record<'b>(state: &mut SMBState, r: &Smb2Record<'b>) } }, _ => { - let tx_key = SMBCommonHdr::new(SMBHDR_TYPE_HEADER, - r.session_id, 0, r.message_id); - SCLogDebug!("SMB2_COMMAND_IOCTL/SMB_NTSTATUS_PENDING looking for {:?}", tx_key); - match state.get_generic_tx(2, SMB2_COMMAND_IOCTL, &tx_key) { + SCLogDebug!("SMB2_COMMAND_IOCTL/SMB_NTSTATUS_PENDING looking for {:?}", hdr); + match state.get_generic_tx(2, SMB2_COMMAND_IOCTL, &hdr) { Some(tx) => { SCLogDebug!("updated status of tx {}", tx.id); tx.set_status(r.nt_status, false);