dns: improved handling of corrupt additionals #11746
Conversation
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.
|
Not sure I fully understand. Is this about logging a partial corrupt message? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #11746 +/- ##
=======================================
Coverage 82.63% 82.63%
=======================================
Files 919 919
Lines 248943 248951 +8
=======================================
+ Hits 205716 205724 +8
Misses 43227 43227
Flags with carried forward coverage won't be shown. Click here to find out more. |
yes |
If the "additionals" are corrupt, the whole message fails to parse, so the tx isn't logged at all. I think the idea here is that we should log what we can. |
| @@ -8,3 +8,4 @@ alert dns any any -> any any (msg:"SURICATA DNS Not a response"; flow:to_client; | |||
| # Z flag (reserved) not 0 | |||
| alert dns any any -> any any (msg:"SURICATA DNS Z flag set"; app-layer-event:dns.z_flag_set; classtype:protocol-command-decode; sid:2240006; rev:2;) | |||
| alert dns any any -> any any (msg:"SURICATA DNS Invalid opcode"; app-layer-event:dns.invalid_opcode; classtype:protocol-command-decode; sid:2240007; rev:1;) | |||
| alert dns any any -> any any (msg:"SURICATA DNS corrupt additionals"; app-layer-event:dns.corrupt_additionals; classtype:protocol-command-decode; sid:2240008; rev:1;) | |||
| let (i, answers) = dns_parse_answer(i, message, header.answer_rr as usize)?; | ||
| let (i, authorities) = dns_parse_answer(i, message, header.authority_rr as usize)?; | ||
| let (i, additionals) = dns_parse_answer(i, message, header.additional_rr as usize)?; | ||
| let additionals_parsed = dns_parse_answer(i, message, header.additional_rr as usize); |
There was a problem hiding this comment.
Should the same logic be applied to answers and authorities as well?
There was a problem hiding this comment.
It looks a good idea. I began with the last one
|
ERROR: ERROR: QA failed on SURI_TLPR1_alerts_cmp. Pipeline 22502 |
|
Continued in #11752 |
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/7228
Describe changes:
Provide values to any of the below to override the defaults.
SV_BRANCH=OISF/suricata-verify#2032