Dns invalid additionals 7228 v4 #11794
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.
To accomodate cases found in QA. The case looks like a CnC channel on port 53, but not using DNS. First request has a very big record length, but all the header fields indicating numbers are 0. First reply has a very big record length as well, there is one non-zero field for number, but the flag indicates a request...
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #11794 +/- ##
==========================================
+ Coverage 82.53% 82.57% +0.03%
==========================================
Files 919 919
Lines 248979 249012 +33
==========================================
+ Hits 205506 205624 +118
+ Misses 43473 43388 -85
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
WARNING:
Pipeline 22741 |
| let is_request = header.flags & 0x8000 == 0; | ||
| if dir == Direction::ToClient && is_request && rlen > 0x1000 { |
There was a problem hiding this comment.
Can we just use a number here?
There was a problem hiding this comment.
What do you mean ?
There was a problem hiding this comment.
rlen > 4096? Or is there some reason to use the hex value here?
| let is_request = header.flags & 0x8000 == 0; | ||
| if dir == Direction::ToClient && is_request && rlen > 0x1000 { |
There was a problem hiding this comment.
Also, shoud we check the direction here? The direction could be flipped based on the return value of this function.
There was a problem hiding this comment.
That is a good question :
My point is to gather 2 hints that this is not DNS : direction is flipped, and record length is big (as the comment states below)
Do you have real cases in DNS where the direction is actually flipped ?
There was a problem hiding this comment.
And I guess my question to you is how would you handle the private pcaps ?
There was a problem hiding this comment.
Do you have real cases in DNS where the direction is actually flipped ?
This can happen when there is packet loss and the request is lost, the response will be picked up as a request I believe. The S-V test dns-reversed-udp-1 shows this "flow" flipping in action - however, lacks the test to test it, which should probably be added.
There was a problem hiding this comment.
| @@ -718,7 +734,7 @@ impl DNSState { | |||
|
|
|||
| const DNS_HEADER_SIZE: usize = 12; | |||
|
|
|||
| fn probe_header_validity(header: &DNSHeader, rlen: usize) -> (bool, bool, bool) { | |||
| fn probe_header_validity(header: &DNSHeader, rlen: usize, dir: Direction) -> (bool, bool, bool) { | |||
There was a problem hiding this comment.
I also think its time this function get some proper documentation as its no longer trivial.
There was a problem hiding this comment.
sure, but do you mean you agree with the changes ? (if there is nothing to change, should we document it ? )
There was a problem hiding this comment.
I agree with the changes, but as its got more complex I think it should now contain what all the constraints are as function documentation.
Needs a ticket for sure. |
Will do |
|
Created https://redmine.openinfosecfoundation.org/issues/7279 how does it look ? |
|
Status : to be rebased after merge of #11859 or later |
catenacyber
added
needs rebase
|
Rebased in #12349 |
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/7228
Describe changes:
SV_BRANCH=OISF/suricata-verify#2032
#11785 with newer commit for the DNS probing parser improvement
@jasonish what do you think about it ?
Should we have a separate ticket and PR to handle first the probing parser ?