dcerpc: mimic gap behavior for invalid data

[inashivb]

Previous PR: #14805

Changes since v5:

• fixed var name as per review
• rebased on top of latest main

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

SV_BRANCH=OISF/suricata-verify#2904

Note: QA deviations on dcerpc stats are expected. The pcaps I got from QA lab showed no errors and an increased number of txs looking like the corresponding s-v test that was updated.

[suricata-qa]

WARNING:

field	baseline	test	%
	SURI_TLPR1_stats_chk
.app_layer.error.dcerpc_tcp.parser	10319	3100	30.04%
.app_layer.tx.dcerpc_tcp	4180	6294	150.57%

Pipeline = 29640

[inashivb]

Needs baseline update as per my evaluation. If you think it's incorrect, please feel free to remove the label and leave a comment about why you think so. Thanks a lot!

[inashivb]

How does this look to you, @catenacyber ?

[catenacyber]

I guess it is good but I feel it hard to read this handle_gap with skip_error_record instead of keeping the code in the main handle_input_data function

[inashivb]

ok but then i'd be duplicating the code at two places..

[catenacyber]

I think the problem here is https://redmine.openinfosecfoundation.org/issues/7254 we do not parse multiple PDUs in one go when we should as packet 67 has the beginning of a next PDU

[catenacyber]

A simpler fix would be

diff --git a/rust/src/dcerpc/dcerpc.rs b/rust/src/dcerpc/dcerpc.rs
index b13914a572..3fa3ff09c4 100644
--- a/rust/src/dcerpc/dcerpc.rs
+++ b/rust/src/dcerpc/dcerpc.rs
@@ -929,6 +929,9 @@ impl DCERPCState {
                 return AppLayerResult::err();
             }
         }
+        if rem > fraglen as u32 {
+            return AppLayerResult::incomplete(fraglen.into(), DCERPC_HDR_LEN.into());
+        }
 
         self.post_gap_housekeeping(direction);
         return AppLayerResult::ok();

[inashivb]

I think the problem here is https://redmine.openinfosecfoundation.org/issues/7254 we do not parse multiple PDUs in one go when we should as packet 67 has the beginning of a next PDU

Do you mean that this ticket is invalid and parsing multiple PDUs is all that needs to be done?

[inashivb]

A simpler fix would be

diff --git a/rust/src/dcerpc/dcerpc.rs b/rust/src/dcerpc/dcerpc.rs
index b13914a572..3fa3ff09c4 100644
--- a/rust/src/dcerpc/dcerpc.rs
+++ b/rust/src/dcerpc/dcerpc.rs
@@ -929,6 +929,9 @@ impl DCERPCState {
                 return AppLayerResult::err();
             }
         }
+        if rem > fraglen as u32 {
+            return AppLayerResult::incomplete(fraglen.into(), DCERPC_HDR_LEN.into());
+        }
 
         self.post_gap_housekeeping(direction);
         return AppLayerResult::ok();

Very nice. Minimalistic multi PDU support :P

[catenacyber]

I think the problem here is https://redmine.openinfosecfoundation.org/issues/7254 we do not parse multiple PDUs in one go when we should as packet 67 has the beginning of a next PDU

Do you mean that this ticket is invalid and parsing multiple PDUs is all that needs to be done?

I think so wrt the SV test, unless you have other test cases that do not fit in this framework...

[catenacyber]

A simpler fix would be

diff --git a/rust/src/dcerpc/dcerpc.rs b/rust/src/dcerpc/dcerpc.rs
index b13914a572..3fa3ff09c4 100644
--- a/rust/src/dcerpc/dcerpc.rs
+++ b/rust/src/dcerpc/dcerpc.rs
@@ -929,6 +929,9 @@ impl DCERPCState {
                 return AppLayerResult::err();
             }
         }
+        if rem > fraglen as u32 {
+            return AppLayerResult::incomplete(fraglen.into(), DCERPC_HDR_LEN.into());
+        }
 
         self.post_gap_housekeeping(direction);
         return AppLayerResult::ok();

Very nice. Minimalistic multi PDU support :P

Minimalistic, but not very nice :-p

[inashivb]

A simpler fix would be

diff --git a/rust/src/dcerpc/dcerpc.rs b/rust/src/dcerpc/dcerpc.rs
index b13914a572..3fa3ff09c4 100644
--- a/rust/src/dcerpc/dcerpc.rs
+++ b/rust/src/dcerpc/dcerpc.rs
@@ -929,6 +929,9 @@ impl DCERPCState {
                 return AppLayerResult::err();
             }
         }
+        if rem > fraglen as u32 {
+            return AppLayerResult::incomplete(fraglen.into(), DCERPC_HDR_LEN.into());
+        }
 
         self.post_gap_housekeeping(direction);
         return AppLayerResult::ok();

wait a second. This does not have correct rem as it is never updated after the initial header handling. wdyt?

Packet 67 indeed is rendered in wireshark to have next PDU's beginning (are those correct btw?) but it's a response packet so even if we do support multiple PDUs, how do we get to the next request that is rejected?

[inashivb]

Do you mean that this ticket is invalid and parsing multiple PDUs is all that needs to be done?

I think so wrt the SV test, unless you have other test cases that do not fit in this framework...

Based on an internal discussion, we want both this and the multi PDU support as we should be able to recover from errors happening due to invalid data anyway