Detect validate callbacks 5634 v4

[catenacyber]

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

Describe changes:

• detect: change the ValidateCallback prototype to deduplicate code
• detect: use one function for md5-like keywords (quic-cyu-hash, ssh-hassh, tls-ja3-hash...)

#11902 with review taken into account : ValidateCallback is not specialized, but extended to have the buffer id, so that keywords can share the same validator code when this buffer id is the only thing changing

This was done on the way to convert quic keywords to rust like #11575

With this PR, it should be easy to add a wrapper for rust for this ValidateCallback stuff

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.78%. Comparing base (378f678) to head (d9acacd).
Report is 173 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11963      +/-   ##
==========================================
+ Coverage   82.76%   82.78%   +0.01%     
==========================================
  Files         910      910              
  Lines      249014   248910     -104     
==========================================
- Hits       206105   206055      -50     
+ Misses      42909    42855      -54     

Flag	Coverage Δ
fuzzcorpus	60.76% <100.00%> (-0.02%)	⬇️
livemode	18.71% <18.60%> (+0.01%)	⬆️
pcap	44.16% <60.46%> (+0.30%)	⬆️
suricata-verify	62.20% <65.11%> (+0.03%)	⬆️
unittests	59.03% <58.13%> (+0.02%)	⬆️

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

[suricata-qa]

Information: QA ran without warnings.

Pipeline 23117

[inashivb]

Validation logic and generalized callbacks do seem to make sense and use lesser lines of code as intended.

Not approving as I do not understand if there can be side effects of the generalization on common buffer keywords.

[catenacyber]

Not approving as I do not understand if there can be side effects of the generalization on common buffer keywords.

The side effect is that some invalid signatures get rejected as mentioned in the doc upgrade...

[jasonish]

I like the idea, but I think there is an issue. For example, with et/open, and pawpatrules with git master:

Warning: detect-tls-ja3-hash: rule 3321277: ja3.hash should not be used together with nocase, since the rule is automatically lowercased anyway which makes nocase redundant. [DetectTlsJa3HashValidateCallback:detect-tls-ja3-hash.c:198]
Notice: suricata: Configuration provided was successfully loaded. Exiting. [SuricataInit:suricata.c:2961]

(Exit code: 0)

With this PR, as-is and rebased:

Warning: detect: rule 3321277: md5-like keyword should not be used together with nocase, since the rule is automatically lowercased anyway which makes nocase redundant. [DetectMd5ValidateCallback:detect-engine.c:5012]
Warning: detect-http-host: rule 2048269: http.host keyword specified along with "nocase". The hostname buffer is normalized to lowercase, specifying nocase is redundant. [DetectHttpHostValidateCallback:detect-http-host.c:197]
Error: detect: error parsing signature "alert http $HOME_NET any -> $EXTERNAL_NET any (msg:"ET DYN_DNS DYNAMIC_DNS HTTP Request to a softether .net Domain"; flow:established,to_server; http.host; dotprefix; content:".softether.net"; nocase; endswith; fast_pattern; reference:url,softether.net; classtype:bad-unknown; sid:2048269; rev:7; metadata:affected_product Any, attack_target Client_and_Server, created_at 2023_09_26, deployment Perimeter, deployment SSLDecrypt, performance_impact Low, confidence High, signature_severity Informational, updated_at 2024_12_01, mitre_tactic_id TA0011, mitre_tactic_name Command_And_Control, mitre_technique_id T1568, mitre_technique_name Dynamic_Resolution;)" from file /opt/suricata/8.0.0-dev/var/lib/suricata/rules/suricata.rules at line 29172 [DetectLoadSigFile:detect-engine-loader.c:182]
Error: suricata: Loading signatures failed. [LoadSignatures:suricata.c:2413]

(Exit code: 1)

It appears that this new warning on http.host and nocase is causing Suricata to error out. Existing behavior for this combination is just a warning yet still successful.

I did not test/check for any change of behavior around the md5-like keywords.

I do not think we should move to an error for this case, especially if its error for some keywords like http.host and just a warning for others like md5-like keywords. Hmm, I guess we also lose log precision in what keyword cause the issue.

[catenacyber]

Looks like an error with transforms and not taking the right buf_id

[catenacyber]

Hmm, I guess we also lose log precision in what keyword cause the issue.

Indeed, will add back

[catenacyber]

So, changes are requested :

• do not lose log precision in what keyword cause the issue for md5-like
• do not change behavior for transformed buffers

[catenacyber]

Next version in #12346 thanks Jason