dnp3: preserve events when no current tx; better recoverty - v2

[jasonish]

Ticket: https://redmine.openinfosecfoundation.org/issues/8979
Ticket: https://redmine.openinfosecfoundation.org/issues/8980

SV_BRANCH=OISF/suricata-verify#3347

[suricata-review-bot]

AI-generated review posted automatically by Suricata ai-review.

Verdict: request changes

• Medium — src/app-layer-dnp3.c:1105 and src/app-layer-dnp3.c:1239: Both handlers still return a fatal parser error on non-05 64 input, so the valid-frame/garbage/valid-frame scenario from Redmine #8980 continues to disable later DNP3 logging and transaction inspection. Recovery also fails when a bad-CRC frame is split: resynchronization consumes the first slice, then its mid-frame continuation hits this fatal path. suricata-verify PR #3347 tests only whole malformed frames. Resynchronize invalid-start input in both directions, preserve a trailing 0x05 across slices, and add garbage-between-frames and split-malformed-frame regressions.

[codecov]

Codecov Report

❌ Patch coverage is 91.30435% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.15%. Comparing base (b8084b3) to head (4cacc0d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16170      +/-   ##
==========================================
+ Coverage   83.14%   83.15%   +0.01%     
==========================================
  Files        1004     1004              
  Lines      277579   277596      +17     
==========================================
+ Hits       230789   230848      +59     
+ Misses      46790    46748      -42     

Flag	Coverage Δ
fuzzcorpus	61.78% <68.88%> (+0.13%)	⬆️
livemode	18.44% <0.00%> (+0.03%)	⬆️
netns	22.83% <0.00%> (-0.05%)	⬇️
pcap	45.41% <0.00%> (-0.04%)	⬇️
suricata-verify	67.37% <71.11%> (+0.03%)	⬆️
unittests	58.53% <23.91%> (-0.01%)	⬇️

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[suricata-qa]

Information: QA ran without warnings.

Pipeline = 33714

[victorjulien]

Is the review bot comment something that still needs looking into @jasonish ?

[jasonish]

Is the review bot comment something that still needs looking into @jasonish ?

No, this is calculated changed based on discussion with @catenacyber and I added it to the ticket: https://redmine.openinfosecfoundation.org/issues/8980#note-10 for transparency. The @suricata-review-bot doesn't appear to pick that up, but it shoud.

[suricata-review-bot]

@jasonish

⚠️ AI-generated response. This answer was produced by the Suricata ai-review bot for this question against commit 4cacc0d64b1f.

Question from @jasonish:

Is the review bot comment something that still needs looking into @jasonish ?

No, this is calculated changed based on discussion with @catenacyber and I added it to the ticket: https://redmine.openinfosecfoundation.org/issues/8980#note-10 for transparency. The @suricata-review-bot doesn't appear to pick that up, but it shoud.

Understood. The linked tracker note was unavailable to the runner, and the public inline discussion only explained why header->len cannot be trusted after a bad CRC. That left the terminal returns at src/app-layer-dnp3.c:1105 and :1239 looking unintentional.

Given your clarification that this behavior is an accepted tradeoff, the finding can be treated as resolved. Recording that rationale directly in the PR discussion or description will make it visible to future bot reviews.

[catenacyber]

Thanks for the work,

CI/QA/AI : ✅
Git ID set : looks fine for me
CLA : you already contributed
Doc update : not needed
Redmine ticket : ok, maybe we should create a tracking ticket for all the protocols to "preserve events when no current tx"
Tests : cool
Dependencies added: none
Code : good
Commits segmentation : cool
Commit messages : ok, but I do not understand (nor see in the code) " Also consume a response frame whose trusted length is too short for the required response headers, so later responses remain available."

[jasonish]

Commit messages : ok, but I do not understand (nor see in the code) " Also consume a response frame whose trusted length is too short for the required response headers, so later responses remain available."

If the outer header was valid but the has user data check failed, we would error out the protocol. As the outer header is trusted (crc check), we can skip to the next frame instead of error'ing out in this case - consume what we have as we know its good. Not the best wording I suppose.

[victorjulien]

This was merged, thanks!