detect/dataset: fix dataset set logic

[regit]

Update of #8114.

The set operation of dataset keyword was not done only signature when there is a full match. This was not correct with regards to expectation.

This patch changes the behavior of the dataset keyword to do a match and a post match for the set operation. In the match, the buffer data that needs to end up in the set is captured and in post match the dataset is updated (if ever the signature is fully matching).

Ticket: #5576

• I have read the contributing guide lines at https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Contributing
• I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/
• I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

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

Describe changes:

• Don't use memcpy to transmit data

suricata-verify-pr: 959

[regit]

Setting it as draft to do more testing and analysis.

[codecov]

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (160c778) 81.67% compared to head (c91fc12) 81.61%.
Report is 1516 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8123      +/-   ##
==========================================
- Coverage   81.67%   81.61%   -0.06%     
==========================================
  Files         962      962              
  Lines      276781   276833      +52     
==========================================
- Hits       226052   225939     -113     
- Misses      50729    50894     +165     

Flag	Coverage Δ
fuzzcorpus	63.28% <82.14%> (-0.08%)	⬇️
suricata-verify	59.38% <82.14%> (+0.01%)	⬆️
unittests	63.33% <1.78%> (-0.02%)	⬇️

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

[victorjulien]

I wonder if this will work if there are more than one dataset set in a rule.

[victorjulien]

I wonder if this will work if there are more than one dataset set in a rule.

Can you add a SV test for this case?

[suricata-qa]

WARNING:

field	baseline	test	%
	TREX_GENERIC_stats_chk
.capture.kernel_drops	0	130544	-
.flow.end.tcp_state.syn_sent	0	245	-
.flow.end.tcp_state.syn_recv	0	3	-
.flow.end.tcp_state.fin_wait1	0	33	-
.flow.end.tcp_state.fin_wait2	0	10	-
.flow.end.tcp_state.time_wait	0	10	-
.flow.end.tcp_state.last_ack	0	11	-
.flow.end.tcp_state.close_wait	0	53	-
.tcp.reassembly_gap	80952	102742	126.92%
.tcp.overlap	0	1	-
.app_layer.error.http.parser	0	15	-
.app_layer.error.ftp.gap	0	6	-
.app_layer.error.smtp.gap	0	31	-
.app_layer.error.dcerpc_tcp.parser	0	6	-

Pipeline 10318

[norg]

I wonder if this will work if there are more than one dataset set in a rule.

Can you add a SV test for this case?

Should it be just a second or third dataset to be set and make sure that those are also not updated if the rule does not match?
Or do you see even more complex scenarios that might have not been covered yet?

I did a test with just adding more datasets that would also set the data but to a different setname and it was correct. But maybe we can think of even more complex rules with different data to be set in different sets.

[victorjulien]

I wonder if this will work if there are more than one dataset set in a rule.

Can you add a SV test for this case?

Should it be just a second or third dataset to be set and make sure that those are also not updated if the rule does not match? Or do you see even more complex scenarios that might have not been covered yet?

I did a test with just adding more datasets that would also set the data but to a different setname and it was correct. But maybe we can think of even more complex rules with different data to be set in different sets.

Yeah lets start with that. If you see obvious other scenarios we can add tests for those as well.

[norg]

OISF/suricata-verify#959 S-V for this PR

[suricata-qa]

WARNING:

field	baseline	test	%
	TREX_GENERIC_stats_chk
.capture.kernel_drops	0	130544	-
.flow.end.tcp_state.syn_sent	0	245	-
.flow.end.tcp_state.syn_recv	0	3	-
.flow.end.tcp_state.fin_wait1	0	33	-
.flow.end.tcp_state.fin_wait2	0	10	-
.flow.end.tcp_state.time_wait	0	10	-
.flow.end.tcp_state.last_ack	0	11	-
.flow.end.tcp_state.close_wait	0	53	-
.tcp.reassembly_gap	80952	102742	126.92%
.tcp.overlap	0	1	-
.app_layer.error.http.parser	0	15	-
.app_layer.error.ftp.gap	0	6	-
.app_layer.error.smtp.gap	0	31	-
.app_layer.error.dcerpc_tcp.parser	0	6	-

Pipeline 10318

[suricata-qa]

ERROR:

ERROR: QA failed on TREX_GENERIC_stats_chk.

Pipeline 10318

[catenacyber]

@victorjulien do you need help with reviewing this ?

[suricata-qa]

ERROR:

ERROR: QA failed on TREX_GENERIC_stats_chk.

Pipeline 10318

[victorjulien]

I wonder if we should just have a pointer to InspectionBuffer here, or even just an index into the array of buffers

[catenacyber]

DetectEngineContentInspectionInternal uses const uint8_t *buffer, const uint32_t buffer_len and not an InspectionBuffer

[suricata-qa]

ERROR:

ERROR: QA failed on TREX_GENERIC_stats_chk.

Pipeline 10318

[catenacyber]

@regit will you work on this again ?

[suricata-qa]

ERROR:

ERROR: QA failed on TREX_GENERIC_stats_chk.

Pipeline 10318

[catenacyber]

I will take this over

[catenacyber]

Continued in #11600