dcerpc: mimic gap behavior for invalid data#14805
Conversation
If invalid data is sent to the parser then instead of rejecting it at the first few bytes that do not conform to the header standards, mimic gap behavior and try to skip a few bytes until a possibly good DCERPC record is found. Ticket: 7251
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14805 +/- ##
==========================================
+ Coverage 82.15% 82.16% +0.01%
==========================================
Files 1003 1003
Lines 263691 263705 +14
==========================================
+ Hits 216626 216678 +52
+ Misses 47065 47027 -38
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
WARNING:
Pipeline = 29572 |
|
|
||
| let _hdr = Frame::new(flow, &stream_slice, &cur_i[consumed as usize..], DCERPC_HDR_LEN as i64, DCERPCFrameType::Hdr as u8, None); | ||
| let _pdu = Frame::new(flow, &stream_slice, &cur_i[consumed as usize..], fraglen as i64, DCERPCFrameType::Pdu as u8, None); | ||
| let _hdr = Frame::new(flow, &stream_slice, cur_i, DCERPC_HDR_LEN as i64, DCERPCFrameType::Hdr as u8, None); |
There was a problem hiding this comment.
Are you reverting fc9da1c which was incorrect as found by oss-fuzz
Am I understanding this correctly ?
There was a problem hiding this comment.
Are you reverting fc9da1c which was incorrect as found by oss-fuzz Am I understanding this correctly ?
No, I'm not reverting it. I'm creating the slice in the beginning now so it'll be wrong to index here at consumed. L816: https://github.com/OISF/suricata/pull/14805/files#diff-bb348f48dcbb4845f97a114f74772ee4849098c3254221826e5e0a61cbe35830R816
There was a problem hiding this comment.
This was already done at line 716
cur_i = &cur_i[offset..];
(with the line after being consumed = offset as u32; which is why fc9da1c is wrong)
There was a problem hiding this comment.
I think you're right. I'm surprised fuzzers didn't catch that. Behavior in this PR looks correct to me. wdyt?
Edit: I think I misunderstood what you are saying in this thread. Are you referring to fuzzers finding the crash in #14766 and that being fixed here? Then, yes. I did not realize that the crash found by the fuzzers was not because of the new code.
4 participants
Previous PR: #14803
Changes since v4:
Link to ticket: https://redmine.openinfosecfoundation.org/issues/7251
SV_BRANCH=OISF/suricata-verify#2904
Note: QA deviations on dcerpc stats are expected but need one review.