tcp: flag established even on error

[catenacyber]

Follows commit 2fb5059, concurrent against #10286

Link to redmine ticket:
Should there be one specific for this ?

Describe changes:

• stream/tcp: flag established even on error (error being Suricata StreamTcpStateDispatch returning an error value)

SV_BRANCH=OISF/suricata-verify#1623

[catenacyber]

In bug-2576-02, this PR makes both signatures trigger

alert http any any -> any any (msg:"nbpackets http"; flow:established,to_client; flow.pkts_toclient: 6; sid: 4; rev: 1;)
alert ip any any -> any any (msg:"nbpackets ip"; flow:established,to_client; flow.pkts_toclient: 6; sid: 5; rev: 1;)

When only the second one triggers on current version even if the flow's alproto is http

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (244a35d) 73.31% compared to head (01d6313) 82.33%.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #10299       +/-   ##
===========================================
+ Coverage   73.31%   82.33%    +9.02%     
===========================================
  Files         895      978       +83     
  Lines      148215   272031   +123816     
===========================================
+ Hits       108666   223987   +115321     
- Misses      39549    48044     +8495     

Flag	Coverage Δ
fuzzcorpus	63.59% <100.00%> (+0.10%)	⬆️
suricata-verify	61.50% <100.00%> (-0.03%)	⬇️
unittests	62.84% <100.00%> (?)

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

[victorjulien]

I'm not sure I agree with this change. The idea is that an error packet is faulty, possibly malicious (e.g. injected), so accepting it as part of the established connection seems wrong.

[catenacyber]

I'm not sure I agree with this change. The idea is that an error packet is faulty, possibly malicious (e.g. injected), so accepting it as part of the established connection seems wrong.

Do you mean that it should not be associated with the flow ?

What do you call accepting it as part of the established connection ?

The code p->flags |= PKT_STREAM_EST; only means "use the flow's alproto for packet/stream engines" in DetectRunPrefilterPkt + DetectRulePacketRules, right ?

What do you think about the test cases ?

alert http any any -> any any (msg:"5 packets http"; flow:established,to_client; flow.pkts_toclient: 5; sid: 5; rev: 1;)
alert http any any -> any any (msg:"6 packets http"; flow:established,to_client; flow.pkts_toclient: 6; sid: 6; rev: 1;)
alert http any any -> any any (msg:"7 packets http"; flow:established,to_client; flow.pkts_toclient: 7; sid: 7; rev: 1;)
alert ip any any -> any any (msg:"nbpackets ip"; flow:established,to_client; flow.pkts_toclient: 6; sid: 61; rev: 1;)

Does this make sense to you that a flow matches 5, 7 (and 61) but not 6 ?

[victorjulien]

If a packet is rejected by the stream engine, we shouldn't inspect it as a stream packet (so inspect only the raw payload, not the stream data it's not a part of).

[catenacyber]

If a packet is rejected by the stream engine, we shouldn't inspect it as a stream packet (so inspect only the raw payload, not the stream data it's not a part of).

Makes sense.
So see #10304