Dcerpc incomplete 5699/v14

[inashivb]

Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/5699

Previous PR: #12219

Changes since v13:

• bug fixed for when the header exists in the state but in the opposite direction
• rebased on top of latest master

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.22%. Comparing base (6f937c7) to head (6814571).
Report is 127 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12320      +/-   ##
==========================================
- Coverage   83.26%   83.22%   -0.04%     
==========================================
  Files         912      912              
  Lines      257643   257274     -369     
==========================================
- Hits       214521   214121     -400     
- Misses      43122    43153      +31     

Flag	Coverage Δ
fuzzcorpus	61.21% <100.00%> (+0.06%)	⬆️
livemode	19.40% <0.00%> (+<0.01%)	⬆️
pcap	44.40% <80.55%> (-0.03%)	⬇️
suricata-verify	62.85% <91.66%> (-0.01%)	⬇️
unittests	59.12% <44.44%> (-0.08%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

[suricata-qa]

WARNING:

field	baseline	test	%
	SURI_TLPR1_stats_chk
.app_layer.flow.dcerpc_tcp	43	100	232.56%

Pipeline 24047

[inashivb]

This is my assessment of the stats deviations.

What data do the deviations occur for?

Any incomplete data that does not follow the direction rules expected by the parser.

Behavior in master

• We define two parameters in the protocol state that control direction: data_needed_for_dir and prev_dir. So, if the direction of current input data does not match the expected direction, an error is returned. ref: https://github.com/OISF/suricata/blob/master/rust/src/dcerpc/dcerpc.rs#L979
• Because the protocol parser returns an error, despite the initial detection being dcerpc, the flow counter is not incremented. Open ticket for this issue: https://redmine.openinfosecfoundation.org/issues/7238

Behavior in this branch

• There is no direction control, the data is parsed as it comes for whatever direction and if it is incomplete, it is conveyed to the common applayer parser. This is buggy as it might overwrite the common state params. But for the test data at hand, this falls in this particular case every time: https://github.com/OISF/suricata/blob/master/src/app-layer.c#L664
• As a result of this, the flow counter ends up being incremented as many times as the parser returned the data was incomplete. <- This looks like a bug to me. I think we should only once increment the flow counter in case of incomplete status of parser on the same flow or at the end of the detection. Thoughts? 👈🏽 [Answer this]

Information common to this entire situation

• The pcap is weird and all the "dcerpc" detected data seems to be trailing data in TCP sessions. Wireshark does not detect the traffic as dcerpc.

My verdict

I think that the stats deviations caused by this PR are incorrect.

1. Quick fix for that is to reintroduce direction control as in master. Shall I move forward w this? 👈🏽 [Answer this]
2. There is perhaps another issue being unmasked here about unnecessary flow counter increment. 👈🏽 [Answer this]

[catenacyber]

We define two parameters in the protocol state that control direction: data_needed_for_dir and prev_dir. So, if the direction of current input data does not match the expected direction, an error is returned.

Should we not be able to support both directions having incomplete data ?
For example HTTP2State has both c2s_buf and s2c_buf for this kind of reassembly over TCP.
As such, your PR looks like going in the right way.

As a result of this, the flow counter ends up being incremented as many times as the parser returned the data was incomplete. <- This looks like a bug to me. I think we should only once increment the flow counter in case of incomplete status of parser on the same flow or at the end of the detection. Thoughts? 👈🏽 [Answer this]

It is indeed a bug if a flow is counted multiple times.
This seems impossible for me at first sight for a single flow to pass multiple times at https://github.com/OISF/suricata/blob/master/src/app-layer.c#L664 because this sets a flow alproto tc or ts to failed, and we can only get there if this alproto tc or ts was unknown in the first place

Quick fix for that is to reintroduce direction control as in master. Shall I move forward w this? 👈🏽 [Answer this]

I do not think so

There is perhaps another issue being unmasked here about unnecessary flow counter increment. 👈🏽 [Answer this]

I think so, what will be next in this rabbit hole ?

[inashivb]

Should we not be able to support both directions having incomplete data ? For example HTTP2State has both c2s_buf and s2c_buf for this kind of reassembly over TCP.

I just looked at HTTP2State. What's done there is already done in the master for dcerpc. But, we don't want to buffer the data in the protocol parser otherwise that defeats the purpose of the applayer::incomplete API from what I understand. It is used so we don't maintain multiple copies of the same data at different places.

It is indeed a bug if a flow is counted multiple times. This seems impossible for me at first sight for a single flow to pass multiple times at https://github.com/OISF/suricata/blob/master/src/app-layer.c#L664 because this sets a flow alproto tc or ts to failed, and we can only get there if this alproto tc or ts was unknown in the first place

You are right indeed. Checking the flow id shows that even though the common tuple looks the same, they're different flows. I should have done this before. Thank you! 🙇🏽‍♀️

Quick fix for that is to reintroduce direction control as in master. Shall I move forward w this? 👈🏽 [Answer this]

I do not think so

ok. Then, I think we have to accept the stats deviations because:

• There are as many incomplete flows detected as dcerpc
• We no longer error out if we did not receive the data in the direction we were expecting

[catenacyber]

ok. Then, I think we have to accept the stats deviations because:

• There are as many incomplete flows detected as dcerpc
• We no longer error out if we did not receive the data in the direction we were expecting

Can you create a SV test that shows this behavior ?

[inashivb]

ok. Then, I think we have to accept the stats deviations because:

• There are as many incomplete flows detected as dcerpc
• We no longer error out if we did not receive the data in the direction we were expecting

Can you create a SV test that shows this behavior ?

Let me see if I can get TLPW for this.

[catenacyber]

TLPW or craft something with scapy entirely artificial now that you know the conditions...

[inashivb]

Victor believes that the behavior in master is more correct. So, added back the direction control in the new rev.
Also, there is an issue with the common detection logic for TLS and DCERPC. The traffic in the pcaps that are responsible for stats differences is actually TLS but is misdetected as DCERPC. TLPR

[catenacyber]

Victor believes that the behavior in master is more correct.

Maybe

So, added back the direction control in the new rev.

Not sure I agree here, but I guess we should share a pcap to discuss on it

[inashivb]

Not sure I agree here, but I guess we should share a pcap to discuss on it

Got into another rabbit hole as some stats weren't matching up despite direction control and it is now again seeming to be related to issue with the flow counter update. Sharing a PR just to fix the flow counter update on a special case to get feedback.