detect/analyzer: add more details for the tcp ack keyword - v2

[0xEniola]

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

Previous PR: #9605

Describe changes:

• Added the detect-tcp-ack.h header to the file.
• Modified the code to properly handle the DETECT_ACK case; pass the int value of ack to the JsonBuilder.
• Corrected the test.rules signature.

test.rules:

alert tcp any any -> any any (msg:"SURICATA STREAM 3way handshake ACK"; ack:0; sid:1;)
alert tcp any any -> any any (msg:"SURICATA STREAM 3way handshake ACK"; ack:1; sid:2;)
alert tcp any any -> any any (msg:"SURICATA STREAM 3way handshake ACK"; ack:2; sid:3;)

test.yaml:

requires:
    min-version: 7.0.0
args:
    - --engine-analysis

checks:
- filter:
    filename: rules.json
    count: 1
    match:
      id: 1
      lists.packet.matches[0].name: "tcp.ack"

output:

{
  "raw":"alert tcp any any -> any any (msg:\"SURICATA STREAM 3way handshake ACK\"; ack:0; sid:1;)",
  "id":1,
  "gid":1,
  "rev":0,
  "msg":"SURICATA STREAM 3way handshake ACK",
  "app_proto":"unknown",
  "requirements": [],
  "type":"pkt",
  "flags": [
    "src_any",
    "dst_any",
    "sp_any",
    "dp_any",
    "need_packet",
    "toserver",
    "toclient"
  ],
  "pkt_engines": [
    {
      "name":"packet",
      "is_mpm":false
    }
  ],
  "frame_engines":[],
  "lists": {
    "packet":{
      "matches": [
        {
          "name":"tcp.ack"
        }
      ]
    }
  }
}

SV_BRANCH=OISF/suricata-verify#1423

[github-actions]

NOTE: This PR may contain new authors:

Daniel Olatunji <[email protected]>

[codecov]

Codecov Report

Merging #9608 (b7849c2) into master (1a132f4) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9608      +/-   ##
==========================================
+ Coverage   82.28%   82.37%   +0.08%     
==========================================
  Files         968      968              
  Lines      274275   274281       +6     
==========================================
+ Hits       225700   225946     +246     
+ Misses      48575    48335     -240     

Flag	Coverage Δ
fuzzcorpus	64.64% <0.00%> (+0.30%)	⬆️
suricata-verify	60.93% <100.00%> (+<0.01%)	⬆️
unittests	62.87% <0.00%> (-0.01%)	⬇️

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

[jufajardini]

Looking much better, as I indicated in the previous PR, we may need some discussion to define how will we settle on the output itself - but that's minor.

For this PR, it would be good to have an accompanying suricata-verify PR with a test similar to the ipopts one (see OISF/suricata-verify#1387).

Could you work on that, too? This will help us visualize that the changes you're adding here are the way we expect them to be.

If you do, make sure to add a check on the test.yaml for the actual values the ack flag has, in each rule ;)

[jufajardini]

Edited PR description to include link to SV PR, to see if CI checks run against it.

[jufajardini]

After considering our documentation, let's go with number for this inner field.

So the expectation is that for the output, we will see not only the keyword name, but also what was set in the rule for the ack number.

[0xEniola]

OK.

I'll check it out—you mean, I should do cd->number right?

[inashivb]

No. There is no cd->number. Your current object would look like ack.ack: 4, you should instead make it ack.number: 4 as per Juliana's comments. Does this make sense?

[0xEniola]

Ohhh! Yes it makes sense.

I understand now.

[jufajardini]

Please see the inline comment for a suggestion on how to name the ack integer value we want to log out :)