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

Return ErrBuf when records processed < actual records send by upstream #1493

Closed
wants to merge 6 commits into from
Closed

Return ErrBuf when records processed < actual records send by upstream #1493

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
17e5f49
Return ErrBuf when actual records processed is lesser than records se…
SriHarsha001 Oct 30, 2023
936c5a0
Add unit tests
SriHarsha001 Nov 7, 2023
cee8881
Add shorter test case
SriHarsha001 Nov 7, 2023
cb842ef
Address comments in the PR
SriHarsha001 Nov 8, 2023
3cece45
remove unnecessart brackets
SriHarsha001 Nov 8, 2023
c96f858
Address PR comments
SriHarsha001 Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions msg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -852,16 +852,25 @@ func (dns *Msg) unpack(dh Header, msg []byte, off int) (err error) {
}

dns.Answer, off, err = unpackRRslice(int(dh.Ancount), msg, off)
if err == nil && int(dh.Ancount) > int(len(dns.Answer)) {
return ErrBuf
}
Copy link
Owner

Choose a reason for hiding this comment

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

Now we return ErrBuf and the code below is only executed when the count is smaller which is weird. So this def. feels wrong. And can't go in as such

Again: an upstream sends back a message that is too large without TC? That is a protocol violation, unless I still misunderstand

Copy link
Author

Choose a reason for hiding this comment

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

Thank you very much for the reply - @miekg. Sure sure I understand. Yes, this is a scenario seen due to non-complying DNS server.

@tmthrgd mentioned the fix is actually much simpler than what I proposed here #1493 (comment). I am kindly requesting Tom's help here.

@rdrozhdzh has very nicely summarized the issue I am trying to explain - #1493 (comment).

Quote - Sporadic "legitimization" of malformed DNS responses by dns library under some condition, i.e. the library doesn't return error, but instead it silently returns (possibly truncated or "fixed") DNS message.

The type of malformed message being considered here is when the number of resource records declared in the message header does not correspond to the actual number of resource records existing in DNS response message. As a special case, this may happen when dns library allocates smaller UDP buffer than the size of DNS response coming from (non-complying) DNS server.

The condition when the error is not reported by dns library is if the last octet of DNS message (i.e. the last byte of the buffer) corresponds to last octet of one of resource records.

Copy link
Author

@SriHarsha001 SriHarsha001 Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@tmthrgd - Earlier you had mentioned about a simpler fix for this issue, would you be able to help here please ?

// The header counts might have been wrong so we need to update it
dh.Ancount = uint16(len(dns.Answer))
if err == nil {
dns.Ns, off, err = unpackRRslice(int(dh.Nscount), msg, off)
}
if err == nil && int(dh.Nscount) > int(len(dns.Ns)) {
SriHarsha001 marked this conversation as resolved.
Show resolved Hide resolved
return ErrBuf
}
// The header counts might have been wrong so we need to update it
dh.Nscount = uint16(len(dns.Ns))
if err == nil {
dns.Extra, _, err = unpackRRslice(int(dh.Arcount), msg, off)
}
if err == nil && int(dh.Arcount) > int(len(dns.Extra)) {
SriHarsha001 marked this conversation as resolved.
Show resolved Hide resolved
return ErrBuf
}
// The header counts might have been wrong so we need to update it
dh.Arcount = uint16(len(dns.Extra))

Expand Down