Rfb max length 8731 v3.6

[catenacyber]

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

Describe changes:

• rfb: limit strings length

SV_BRANCH=OISF/suricata-verify#3342

#16163 with git diff staged and pushed

[suricata-review-bot]

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

Verdict: request changes

• Medium — rust/src/rfb/rfb.rs:775: The CI-selected suricata-verify PR #3342 tests only 32-byte strings with a 24-byte cap, each contained in one TCP payload. It never exercises the nonzero cross-segment to_skip_tc carry path—the path that caused a remotely triggered Suricata abort in the preceding revision. Incorporate rfb-max-8731-longname-span from suricata-verify PR #3340 into the CI-selected test PR, retaining its engine-survival, 4096-byte truncation, and too_long_string assertions.

[codecov]

Codecov Report

❌ Patch coverage is 78.18182% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.11%. Comparing base (cd7f775) to head (54a486b).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16164      +/-   ##
==========================================
- Coverage   83.11%   83.11%   -0.01%     
==========================================
  Files        1004     1004              
  Lines      277558   277598      +40     
==========================================
+ Hits       230692   230725      +33     
- Misses      46866    46873       +7     

Flag	Coverage Δ
fuzzcorpus	61.61% <44.44%> (+0.01%)	⬆️
livemode	18.41% <3.70%> (+0.01%)	⬆️
netns	22.84% <3.70%> (+0.01%)	⬆️
pcap	45.41% <66.66%> (-0.04%)	⬇️
suricata-verify	67.27% <77.77%> (-0.07%)	⬇️
unittests	58.53% <38.18%> (-0.02%)	⬇️

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 = 33645

[victorjulien]

My reviewer flagged another accounting issue, but it's not leading to a panic. So perhaps it can be addressed layer.

OISF/suricata-verify@3c4ffc4

@suricata-review-bot do you agree with the analysis in that commit?

[suricata-review-bot]

@victorjulien

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

Question from @victorjulien:

My reviewer flagged another accounting issue, but it's not leading to a panic. So perhaps it can be addressed layer.

OISF/suricata-verify@3c4ffc4

@suricata-review-bot do you agree with the analysis in that commit?

I agree with the accounting principle, but it is not an outstanding defect at this head. parse_failure_reason leaves the uncaptured tail in rem (rust/src/rfb/parser.rs:182-190), while AppLayerResult::ok() consumes the entire input slice (src/app-layer-parser.h:56-58). Accordingly, rust/src/rfb/rfb.rs:728-730 carries only request.to_skip - rem.len().

For the linked 5000-byte/1024-byte-cap case, both values are 3976, so the pending skip becomes zero and the following segment is not over-skipped. The suricata-verify PR #3340 scenario is useful regression coverage, but its analysis does not reveal a remaining bug in this revision.

[catenacyber]

My reviewer flagged another accounting issue, but it's not leading to a panic. So perhaps it can be addressed layer.

This is already present in main, independently of this PR, and I am not sure what the behavior should be anyways

See OISF/suricata-verify#3340 (comment)

Something like

diff --git a/rust/src/rfb/rfb.rs b/rust/src/rfb/rfb.rs
index 931d2feace..6d6eebb5f5 100644
--- a/rust/src/rfb/rfb.rs
+++ b/rust/src/rfb/rfb.rs
@@ -740,6 +740,10 @@ impl RFBState {
                             } else {
                                 debug_validate_fail!("no transaction set at failure reason stage");
                             }
+                            if !rem.is_empty() {
+                                current_transaction.set_event(RFBEvent::DataAfterCloseState);
+                                return AppLayerResult::err();
+                            }

[victorjulien]

My bot is happy now, but I do think it would be good to use (some of) the tests from OISF/suricata-verify#3340 as well

[catenacyber]

Replaced by #16177

[catenacyber]

My bot is happy now, but I do think it would be good to use (some of) the tests from OISF/suricata-verify#3340 as well

Took one test

For the others, they seem to me that they enforce a current behavior of Suricata that is not the expected one (do you want me to create a ticket about it ? )

[victorjulien]

My bot is happy now, but I do think it would be good to use (some of) the tests from OISF/suricata-verify#3340 as well

Took one test

For the others, they seem to me that they enforce a current behavior of Suricata that is not the expected one (do you want me to create a ticket about it ? )

I think this is a tricky effect of these auto-generated tests, they expose existing behavior but offer little guidance on what correct behavior is. I will leave the judgement of adding a ticket with you :)

[catenacyber]

I think this is a tricky effect of these auto-generated tests, they expose existing behavior but offer little guidance on what correct behavior is. I will leave the judgement of adding a ticket with you :)

At least, these tests were helpful for debugging :-)

Not creating the ticket now