Skip to content
New issue

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

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

Already on GitHub? # to your account

dns: improved handling of corrupt additionals #11752

Closed

Conversation

catenacyber
Copy link
Contributor

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

Describe changes:

  • dns: improved handling of corrupt additionals

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

SV_BRANCH=OISF/suricata-verify#2032

#11746 with review taken into account like using invalid instead of corrupt

Ticket: 7228

That means log the rest of queries and answers, even if the
final field additionals is corrupt.
Set an event in this case.
Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.62%. Comparing base (79aa486) to head (9ed857d).
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11752      +/-   ##
==========================================
- Coverage   82.63%   82.62%   -0.02%     
==========================================
  Files         919      919              
  Lines      248943   248971      +28     
==========================================
- Hits       205716   205704      -12     
- Misses      43227    43267      +40     
Flag Coverage Δ
fuzzcorpus 60.86% <100.00%> (+<0.01%) ⬆️
livemode 18.71% <0.00%> (-0.04%) ⬇️
pcap 44.13% <83.87%> (-0.04%) ⬇️
suricata-verify 61.88% <83.87%> (+0.01%) ⬆️
unittests 59.01% <74.19%> (-0.01%) ⬇️

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

@suricata-qa
Copy link

Information:

ERROR: QA failed on SURI_TLPR1_alerts_cmp.

field baseline test %
SURI_TLPR1_stats_chk
.app_layer.error.dns_tcp.parser 30 3 10.0%

Pipeline 22522

@catenacyber
Copy link
Contributor Author

@ct0br0 could I get the pcap for the 30 flows with app_layer.error.dns_tcp.parser ?

@ct0br0
Copy link

ct0br0 commented Sep 11, 2024

ya, will try to extract those in a few.

Comment on lines +369 to +377
match authorities_parsed {
Ok((i, authorities_ok)) => {
authorities = authorities_ok;
i_next = i;
}
_ =>{
invalid_authorities = true;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: if let Ok would look better to the eye.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, will do

@catenacyber catenacyber marked this pull request as draft September 12, 2024 20:35
@catenacyber
Copy link
Contributor Author

ya, will try to extract those in a few.

I get 0 errors with master instead of 30 as reported by QA.
I run :
./src/suricata -k none -c suricata.yaml -S rules/dns-events.rules -l log -r /Users/catena/Downloads/pcaps-dnstcp/
And then :
jq 'select(.stats) | .stats.app_layer.error.dns_tcp' log/eve.json

@catenacyber
Copy link
Contributor Author

Next in #11785

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

Successfully merging this pull request may close these issues.

4 participants