Bug #3109
closeddcerpc engine not generating alerts
Added by Travis Green about 5 years ago. Updated over 2 years ago.
Description
Rules using dce* keywords do not generate an alert despite matching packet contents. For example, given these two rules:
alert tcp any any -> $HOME_NET any (msg:"TGI LATERAL DCERPC ATSVC v1.0 Bind UUID 1ff70682-0a51-30e8-076d-740be8cee98b"; flow:established; dce_iface:1ff70682-0a51-30e8-076d-740be8cee98b,any_frag; reference:url,401trg.com/an-introduction-to-smb-for-network-security-analysts/; classtype:attempted-admin; sid:2610115; rev:1; metadata:notworking;) alert tcp any any -> $HOME_NET any (msg:"TGI LATERAL DCERPC ATSVC v1.0 Bind UUID"; flow:established; content:"|82 06 f7 1f 51 0a e8 30 07 6d 74 0b e8 ce e9 8b|"; reference:url,401trg.com/an-introduction-to-smb-for-network-security-analysts/; classtype:attempted-admin; sid:2610113; rev:1;)
and this packet: https://imgur.com/a/necQQvy
An alert is only generated for the rule that does not use dce_iface. Pcap attached for repro.
Files
20171220_smb_at_schedule.pcap (3.53 KB) 20171220_smb_at_schedule.pcap | Travis Green, 08/09/2019 11:01 PM |
Updated by Travis Green about 5 years ago
Eric Leblond had this deeper analysis to offer:
regit 4:39 PM ok so here we are: Diff diff --git a/rust/src/smb/detect.rs b/rust/src/smb/detect.rs index c3b890eef..1ab8f3f4d 100644 --- a/rust/src/smb/detect.rs +++ b/rust/src/smb/detect.rs @@ -182,13 +182,13 @@ pub extern "C" fn rs_smb_tx_get_dce_iface(state: &mut SMBState, ver_check: u16) -> u8 { - let is_dcerpc_request = match tx.type_data { +/* let is_dcerpc_request = match tx.type_data { Some(SMBTransactionTypeData::DCERPC(ref x)) => { x.req_cmd == 1 }, _ => { false }, }; if !is_dcerpc_request { return 0; - } + } */ let ifaces = match state.dcerpc_ifaces { Some(ref x) => x, _ => { @@ -197,12 +197,13 @@ pub extern "C" fn rs_smb_tx_get_dce_iface(state: &mut SMBState, }; let uuid = unsafe{std::slice::from_raw_parts(uuid_ptr, uuid_len as usize)}; - SCLogDebug!("looking for UUID {:?}", uuid); + SCLogNotice!("looking for UUID {:?}", uuid); for i in ifaces { - SCLogDebug!("stored UUID {:?} acked {} ack_result {}", i, i.acked, i.ack_result); + SCLogNotice!("stored UUID {:?} acked {} ack_result {}", i, i.acked, i.ack_result); - if i.acked && i.ack_result == 0 && i.uuid == uuid { + //if i.acked && i.ack_result == 0 && i.uuid == uuid { + if i.uuid == uuid { if match_version(ver_op as u8, ver_check as u16, i.ver) { return 1; } First code is working only on request second if is forcing check on fact the iface is acked and the ack_result is 0 First point: this contredict a previous commit https://github.com/OISF/suricata/commit/c4b56ca28917eb460ea9eb223b9bc98fbb9ee1d8 so we have a bug second point: I don't know enough the protocol but for me, if we specify a match on iface and we have iface we should have a match any way @tgreen with this patch I got 2 alerts on signature that was failing tgreen profile image regit 4:55 PM 4.1.x with rust and without is not matching too
Updated by Eric Leblond about 5 years ago
Side note: the pcap needs stream.midstream to true to get properly analysed but this does not alert correctly after that.
Updated by Andreas Herz about 5 years ago
- Assignee set to OISF Dev
- Target version set to TBD
Updated by Victor Julien about 5 years ago
Can you test against: Suricata 4.1.5 (no rust), 4.1.5 (with rust) and git master?
Updated by Victor Julien about 5 years ago
- Status changed from New to Feedback
- Assignee changed from OISF Dev to Travis Green
- Target version changed from TBD to 70
Updated by Travis Green about 5 years ago
Submitted PR for suricata-verify test https://github.com/OISF/suricata-verify/pull/139
Updated by Travis Green about 5 years ago
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-= PATCHED -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
$ cat /dev/null > ./fast.log && sudo /opt/suricata-git.latest/src/suricata -c /etc/suricata/suricata.testsuri4.yaml -l . -S ~/rules/lateral-rules/lateral.rules -k none -r ./merged.pcap && cat ./fast.log && wc ./fast.log
[29956] 21/10/2019 -- 12:36:03 - (suricata.c:1072) <Notice> (LogVersion) -- This is Suricata version 5.0.0-dev (412ae11ba 2019-10-12) running in USER mode
<snip>
56 1331 14696 ./fast.log
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-= UNPATCHED -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
$ cat /dev/null > ./fast.log && sudo /opt/suricata-git.debug/src/suricata -c /etc/suricata/suricata.testsuri4.yaml -l . -S ~/rules/lateral-rules/lateral.rules -k none -r ./merged.pcap && cat ./fast.log && wc ./fast.log
[30428] 21/10/2019 -- 12:41:15 - (suricata.c:1076) <Notice> (LogVersion) -- This is Suricata version 5.0.0-dev (494617bb3 2019-09-12) running in USER mode
<snip>
42 974 10832 ./fast.log
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-= UNPATCHED 4.1.5 w/rust -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
$ cat /dev/null > ./fast.log && sudo /opt/suricata-4.1.5.rust/src/suricata -c /etc/suricata/suricata.testsuri4.yaml -l . -S ~/rules/lateral-rules/lateral.rules -k none -r ./merged.pcap && cat ./fast.log && wc ./fast.log
21/10/2019 -- 12:36:49 - <Notice> - This is Suricata version 4.1.5 RELEASE
<snip>
42 974 10832 ./fast.log
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-= UNPATCHED 4.1.5 w/o rust -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
$ cat /dev/null > ./fast.log && sudo /opt/suricata-4.1.5.norust/src/suricata -c /etc/suricata/suricata.testsuri4.yaml -l . -S ~/rules/lateral-rules/lateral.rules -k none -r ./merged.pcap && cat ./fast.log && wc ./fast.log
21/10/2019 -- 12:36:54 - <Notice> - This is Suricata version 4.1.5 RELEASE
46 1076 11936 ./fast.log
details @ https://gist.github.com/travisbgreen/e3b34e848efbe2fe0dc37183786ce9be
Updated by Victor Julien over 4 years ago
- Assignee changed from Travis Green to OISF Dev
Updated by Victor Julien about 4 years ago
- Target version changed from 70 to TBD
I think this works as intended: bind followed by a request. IIRC this was done for Snort compatibility.
I agree it would be nicer to be able to just match on the bind, but I don't want to break existing rules either.
Perhaps we need a new set of keywords where compatibility doesn't have to be taken into account.
Updated by Philippe Antoine over 3 years ago
- Assignee changed from OISF Dev to Shivani Bhardwaj
Updated by Victor Julien almost 3 years ago
- Related to Bug #4769: dcerpc dce_iface just match a packet added
Updated by Victor Julien almost 3 years ago
- Related to Bug #4767: Rule error in SMB dce_iface and dce_opnum keywords added
Updated by Shivani Bhardwaj almost 3 years ago
- Status changed from Feedback to Assigned
- Priority changed from Normal to High
Updated by Shivani Bhardwaj almost 3 years ago
Victor Julien wrote in #note-10:
I think this works as intended: bind followed by a request. IIRC this was done for Snort compatibility.
I agree it would be nicer to be able to just match on the bind, but I don't want to break existing rules either.
Perhaps we need a new set of keywords where compatibility doesn't have to be taken into account.
I researched a bit and found that Victor's analysis is correct. We do not alert here because there is no request following the bind request. This way iface match works even in combination with opnum and stub_data (as a part of the non association calls).
So, do we need to implement any new keywords or close this issue?
Updated by Victor Julien almost 3 years ago
If the BIND, BIND_ACK and REQUEST are all part of the same TX, could we relax this requirement? Then a dce.iface
only rule could work, but a mix of dce.iface
and dce.opnum
as well.
Updated by Victor Julien over 2 years ago
Updated by Shivani Bhardwaj over 2 years ago
Asked some questions on https://github.com/OISF/suricata-verify/pull/799
Seems to be working with a slight rule change now in master and master-6.0.x so there was perhaps an accidental fix.
Updated by Shivani Bhardwaj over 2 years ago
- Status changed from Assigned to Closed
- Target version changed from TBD to 7.0.0-beta1
It seems to no longer be an issue in 6.0+ as demonstrated in https://github.com/OISF/suricata-verify/pull/845