dcerpc: prevent integer underflow

[catenacyber]

Link to ticket: https://redmine.openinfosecfoundation.org/issues/
None, oss-fuzz https://issues.oss-fuzz.com/u/1/issues/393414238

Describe changes:

• DCERPC: prevent int underflow
• DCERPC: use different header for both directions

@inashivb what do you think ?

[jasonish]

This does appear to restore correct behavior.

[catenacyber]

@inashivb what do you think of the behavior when fraglen < 16 ?

I think we should set an event

[inashivb]

@inashivb what do you think of the behavior when fraglen < 16 ?

I think we should set an event

Agreed.

[jasonish]

For commit dcerpc: use different header for different directions, there is no reason why, or ticket, about why this commit is being done.

[jasonish]

Looks like a CI error with an SCLogDebug.

[suricata-qa]

ERROR:

ERROR: QA failed on ASAN_TLPR1_suri.

Pipeline 24561

[catenacyber]

For commit dcerpc: use different header for different directions, there is no reason why, or ticket, about why this commit is being done.

I guess we need one or more tickets, but wanted Shivani's opinion first ;-)

[catenacyber]

TODOs :

• fix header handling (by removing it from persistent DCERPC state)
• take fraglen into account like &cur_i[parsed as usize..] should be &cur_i[parsed as usize..fraglen]
• how to handle DCERPC headers having a fraglen < 16 ?
• handle multiple PDUs in one call of handle_input_data

Header handling was wrong in the case

• packet A to server is fragmented (return AppLayerResult::incomplete)
• packet B is to client, but uses the header of the to_server packet

[codecov]

Codecov Report

Attention: Patch coverage is 98.41270% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.69%. Comparing base (d4330ef) to head (323bf95).

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #12528   +/-   ##
=======================================
  Coverage   80.68%   80.69%           
=======================================
  Files         925      925           
  Lines      258914   258858   -56     
=======================================
- Hits       208914   208880   -34     
+ Misses      50000    49978   -22     

Flag	Coverage Δ
fuzzcorpus	56.83% <98.18%> (+<0.01%)	⬆️
livemode	19.41% <0.00%> (+<0.01%)	⬆️
pcap	44.19% <90.90%> (+<0.01%)	⬆️
suricata-verify	63.38% <94.54%> (-0.01%)	⬇️
unittests	58.38% <88.88%> (-0.01%)	⬇️

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

[suricata-qa]

Information: QA ran without warnings.

Pipeline 24577

[suricata-qa]

Information: QA ran without warnings.

Pipeline 24577

[inashivb]

TODOs :

* fix header handling (by removing it from persistent DCERPC state)

Indeed. This surfaced while doing the work recently.

* take fraglen into account like `&cur_i[parsed as usize..]` should be `&cur_i[parsed as usize..fraglen]`

sounds reasonable.

* how to handle DCERPC headers having a fraglen < 16 ?

you suggested setting an event and that sounds good to me.

* handle multiple PDUs in one call of `handle_input_data`

There's a ticket for this already. If you're working on it, please assign it to yourself: https://redmine.openinfosecfoundation.org/issues/7254

Header handling was wrong in the case

* packet A to server is fragmented (return AppLayerResult::incomplete)

* packet B is to client, but uses the header of the to_server packet

Sounds correct. This case is not covered with the current header reuse approach. This explanation should go in the commit and there should be another ticket for this.

Thank you for improving this parser! :)