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.

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

Dns dcerpc reversed v4 #12134

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ilya-bakhtin
Copy link
Contributor

@ilya-bakhtin ilya-bakhtin commented Nov 19, 2024

Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.

Contribution style:

Our Contribution agreements:

Changes (if applicable):

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

Describe changes:
This is a replacement of #11679
There are 2 commits.
The first one is intended to improve DCERPC UDP detection. False positives resulted in improper work of the detection framework.
The second commit simplifies the detection framework function AppLayerProtoDetectGetProto.
It previously contained a bug that combined with a false positive in DCERPC resulted in incorrect reporting of DNS flow direction.

Provide values to any of the below to override the defaults.

  • To use an LibHTP, Suricata-Verify or Suricata-Update pull request,
    link to the pull request in the respective _BRANCH variable.
  • Leave unused overrides blank or remove.

SV_REPO=
SV_BRANCH=OISF/suricata-verify#2151
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=

Several additional checks are added to the probing parser to avoid false
detection of DNS as DCERPC

Ticket - 7111
Protocol detection code is simplified. Removed dependency on explicit
alproto constants from the common part of code that must not be aware of
the each specific protocol features.

Ticket - 7111
Copy link

codecov bot commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.49%. Comparing base (5d766df) to head (1eae0b0).
Report is 49 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #12134       +/-   ##
===========================================
+ Coverage   62.68%   79.49%   +16.80%     
===========================================
  Files         840      909       +69     
  Lines      153669   257701   +104032     
===========================================
+ Hits        96323   204851   +108528     
+ Misses      57346    52850     -4496     
Flag Coverage Δ
fuzzcorpus 60.93% <100.00%> (?)
livemode 19.43% <0.00%> (?)
pcap 44.40% <100.00%> (?)
suricata-verify ?
unittests 59.27% <50.00%> (?)

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

@catenacyber
Copy link
Contributor

@ilya-bakhtin I think you need ro rebase your SV PR onto latest SV master to get CI green

@ilya-bakhtin
Copy link
Contributor Author

i'm sorry, my bad
i tested it locally but forgot to submit the rebased version
it seems the main PR must also be rebased since one unrelated test failed after SV rebasing.

anyhow SV - OISF/suricata-verify#2151

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants