Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dcerpc incomplete/v4 #9276

Closed
wants to merge 3 commits into from
Closed

dcerpc incomplete/v4 #9276

wants to merge 3 commits into from

Conversation

inashivb
Copy link
Member

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

Previous PR: #9270

Changes since v3:

  • fix pre existing issue: error out on invalid header, use diff retvals (found by fuzzer post incomplete API changes)

@inashivb
dcerpc: use AppLayerResult::incomplete API
ac61012 
Instead of own internal mechanism of buffering in case of fragmented
data, use AppLayerResult::incomplete API to let the AppLayer Parser take
care of it. This makes the memory use more efficient.
Remove any unneeded variables and code with the introduction of this
API.

Ticket 5699
@inashivb
dcerpc: remove fragmented data tests
1c10dc5 
With the introduction of AppLayerResult::incomplete API, fragmented data
is no longer handled fully in the dcerpc code making the tests about
such data invalid.

Ticket 5699
@inashivb
dcerpc: return error on invalid header
2f73811
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #9276 (2f73811) into master (9a33c53) will decrease coverage by 0.01%.
The diff coverage is 94.11%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #9276      +/-   ##
==========================================
- Coverage   82.42%   82.41%   -0.01%     
==========================================
  Files         968      968              
  Lines      273952   273714     -238     
==========================================
- Hits       225807   225594     -213     
+ Misses      48145    48120      -25
Flag Coverage Δ
fuzzcorpus 64.71% <94.11%> (+0.02%) ⬆️
suricata-verify 60.79% <82.35%> (-0.02%) ⬇️
unittests 62.90% <82.35%> (-0.04%) ⬇️

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

@inashivb inashivb marked this pull request as ready for review July 21, 2023 16:18
@inashivb inashivb requested a review from jasonish as a code owner July 21, 2023 16:18
@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPR1_stats_chk
.app_layer.flow.dcerpc_tcp 4188 43 1.03%
.app_layer.tx.dcerpc_tcp 5949 4165 70.01%
.app_layer.error.dcerpc_tcp.parser 6248 10393 166.34%

Pipeline 15350

@victorjulien victorjulien added this to the 8.0 milestone Jul 24, 2023
catenacyber
catenacyber requested changes Aug 30, 2023
View reviewed changes
Copy link
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think tests need to be adapted instead of simply removed

@inashivb
Copy link
Member Author

This one is blocked bc:

  • I need to make a minimal reproducer of the pcap that causes the massive stats deviations and,
  • confirm/deny whether this PR is correct
    Will update once I have something working on that front.

@catenacyber
Copy link
Contributor

Did you get a minimal pcap reproducer for stats deviation ?
Can I help somehow here ?

@inashivb
Copy link
Member Author

Did you get a minimal pcap reproducer for stats deviation ?

No. I couldn't pull out the particular full dcerpc stream(s) that caused this. But, I haven't checked in a while now.

Can I help somehow here ?

Yes, if you could tell how to go about extracting such streams from very large pcaps?
The one I'm concerned with for the stats deviation in this PR is 190GB.

Please note this is what I had tried:
Filter out all the dcerpc traffic with tshark and run Suricata with midstream enabled on it. But, my extracted pkts were broken so I did something wrong. Victor said we shouldn't hack it and extract full streams. I was unable to figure out the correct filter for that at the time.

Challenges I had faced:

  • tshark tries to load entire pcap at once so memory was an issue at first
  • Saving the pkts extracted w filter requires more than just saving the extracted pkts

@catenacyber
Copy link
Contributor

Can I help somehow here ?

Yes, if you could tell how to go about extracting such streams from very large pcaps? The one I'm concerned with for the stats deviation in this PR is 190GB.

Please note this is what I had tried: Filter out all the dcerpc traffic with tshark and run Suricata with midstream enabled on it. But, my extracted pkts were broken so I did something wrong. Victor said we shouldn't hack it and extract full streams. I was unable to figure out the correct filter for that at the time.

I agree with Victor.
Can you have some filter based on ports ?

I also added some comments inline.
The bytes_consumed field seems to need more changes

@ct0br0
Copy link

ct0br0 commented Jan 9, 2024

Extracting a pcap with ports 135, 139 and 445 from TLPR1 is around 415MB and has 4140 .app_layer.error.dcerpc_tcp.parser but splitting by IP only has 13 from 192.168.0.1 which isn't what I'd expect. I will see if I can find anything that may help more.

@inashivb
Copy link
Member Author

Extracting a pcap with ports 135, 139 and 445 from TLPR1 is around 415MB and has 4140 .app_layer.error.dcerpc_tcp.parser but splitting by IP only has 13 from 192.168.0.1 which isn't what I'd expect. I will see if I can find anything that may help more.

Thanks a lot! Actually, I'm also interested in the 415MB pcap that has 4k+ dcerpc errors.

@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPR1_stats_chk
.app_layer.flow.dcerpc_tcp 4188 43 1.03%
.app_layer.tx.dcerpc_tcp 5949 4165 70.01%
.app_layer.error.dcerpc_tcp.parser 6248 10393 166.34%

Pipeline 15350

@catenacyber
Copy link
Contributor

Closing this PR as stale, let me know if I can help here Shivani

@catenacyber catenacyber closed this Apr 3, 2024
@inashivb inashivb deleted the opt-5699/v4 branch April 25, 2024 05:44
@inashivb inashivb mentioned this pull request Aug 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@catenacyber catenacyber catenacyber requested changes

@jlucovsky jlucovsky jlucovsky left review comments

@jasonish jasonish Awaiting requested review from jasonish jasonish is a code owner

Assignees
No one assigned
Labels
None yet
Milestone
8.0
Development

Successfully merging this pull request may close these issues.
6 participants
@inashivb @suricata-qa @catenacyber @ct0br0 @jlucovsky @victorjulien