dns
dns copied to clipboard
miekg
Return ErrBuf when records processed < actual records send by upstream
This proposed PR is to address the bug outlined here - https://github.com/miekg/dns/issues/1492
Additional reference - https://github.com/coredns/coredns/pull/6371#discussion_r1374384048
Issue Description If there is a misbehaving upstream server which does not set TC bit and sends back a response length greater than pc.c.UDPSize or 512 default size, then there is a corner case where offset is equal to len(msg) as calculated in unpackHeader defined here github.com/miekg/msg_helpers.go. In this case, there is no Overflow/ErrBuff error returned, so the response will be truncated without TC bit set.
For example -
- When domain name has 15 characters, offset after unpackQuestion will be 32.
- Each loop adds 30 to offset and when number of A records in response is more than 15, then offset calculated in unpackRRslice will keep increasing in this sequence 62,92,122,152,182.....482,512
- In 16th loop, offset (32 question bytes + 30 * 16) == 512 which will be == len(msg) for default UDPsize, so in this case, there is no Overflow error returned.
- And the response will be truncated (ie response does not contain all the A records) and TC flag is false. This is a failure scenario.
Similarly,
- When domain name has 19 characters, offset after unpackQuestion will be 36.
- Each loop adds 34 to offset and when number of A records in response is more than 14, then offset calculated in unpackRRslice will keep increasing in this sequence 70,104,138,.....512
- In 14th loop, offset (36 question bytes + 34 * 14) == 512 which will be == len(msg) for default UDPsize, so in this case, there is no Overflow error returned.
- And the response will be truncated (ie response does not contain all the A records) and TC flag is false. This is a failure scenario.
As we can see, response is truncated and TC bit is false.
~----------------------------------------------------------------------------
kubectl exec dnsutils -- dig corednse2e.com +ignore +noedns +search +noshowsearch +time=10 +tries=6 @10.96.0.10
; <<>> DiG 9.9.5-9+deb8u19-Debian <<>> corednse2e.com +ignore +noedns +search +noshowsearch +time=10 +tries=6 @10.96.0.10
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 35567
;; flags: qr rd ra; QUERY: 1, ANSWER: 30, AUTHORITY: 0, ADDITIONAL: 0
;; QUESTION SECTION:
;corednse2e.com. IN A
;; ANSWER SECTION:
corednse2e.com. 30 IN A 10.96.0.55
corednse2e.com. 30 IN A 10.96.0.51
corednse2e.com. 30 IN A 10.96.0.246
corednse2e.com. 30 IN A 10.96.0.93
corednse2e.com. 30 IN A 10.96.0.54
corednse2e.com. 30 IN A 10.96.0.97
corednse2e.com. 30 IN A 76.223.105.230
corednse2e.com. 30 IN A 10.96.0.98
corednse2e.com. 30 IN A 10.96.0.242
corednse2e.com. 30 IN A 10.96.0.50
corednse2e.com. 30 IN A 10.96.0.91
corednse2e.com. 30 IN A 10.96.0.96
corednse2e.com. 30 IN A 10.96.0.250
corednse2e.com. 30 IN A 10.96.0.252
corednse2e.com. 30 IN A 10.96.0.99
corednse2e.com. 30 IN A 10.96.0.248
corednse2e.com. 30 IN A 13.248.243.5
corednse2e.com. 30 IN A 10.96.0.254
corednse2e.com. 30 IN A 10.96.0.92
corednse2e.com. 30 IN A 10.96.0.249
corednse2e.com. 30 IN A 10.96.0.52
corednse2e.com. 30 IN A 10.96.0.251
corednse2e.com. 30 IN A 10.96.0.244
corednse2e.com. 30 IN A 10.96.0.53
corednse2e.com. 30 IN A 10.96.0.243
corednse2e.com. 30 IN A 10.96.0.247
corednse2e.com. 30 IN A 10.96.0.253
corednse2e.com. 30 IN A 10.96.0.255
corednse2e.com. 30 IN A 10.96.0.95
corednse2e.com. 30 IN A 10.96.0.94
;; Query time: 37 msec
;; SERVER: 10.96.0.10#53(10.96.0.10)
;; WHEN: Thu Oct 26 05:31:44 UTC 2023
;; MSG SIZE rcvd: 512
Whereas full response should have 64 records.
~----------------------------------------------------------------------------
kubectl exec dnsutils -- dig corednse2e.com +ignore +noedns +search +noshowsearch +time=10 +tries=6 @168.63.129.16
; <<>> DiG 9.9.5-9+deb8u19-Debian <<>> corednse2e.com +ignore +noedns +search +noshowsearch +time=10 +tries=6 @168.63.129.16
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 59586
;; flags: qr rd ra; QUERY: 1, ANSWER: 64, AUTHORITY: 0, ADDITIONAL: 0
;; QUESTION SECTION:
;corednse2e.com. IN A
;; ANSWER SECTION:
corednse2e.com. 600 IN A 76.223.105.230
corednse2e.com. 600 IN A 13.248.243.5
corednse2e.com. 600 IN A 10.96.0.91
corednse2e.com. 600 IN A 10.96.0.92
corednse2e.com. 600 IN A 10.96.0.93
corednse2e.com. 600 IN A 10.96.0.94
corednse2e.com. 600 IN A 10.96.0.95
corednse2e.com. 600 IN A 10.96.0.96
corednse2e.com. 600 IN A 10.96.0.97
corednse2e.com. 600 IN A 10.96.0.98
corednse2e.com. 600 IN A 10.96.0.242
corednse2e.com. 600 IN A 10.96.0.243
corednse2e.com. 600 IN A 10.96.0.99
corednse2e.com. 600 IN A 10.96.0.244
corednse2e.com. 600 IN A 10.96.0.246
corednse2e.com. 600 IN A 10.96.0.247
corednse2e.com. 600 IN A 10.96.0.248
corednse2e.com. 600 IN A 10.96.0.249
corednse2e.com. 600 IN A 10.96.0.250
corednse2e.com. 600 IN A 10.96.0.251
corednse2e.com. 600 IN A 10.96.0.252
corednse2e.com. 600 IN A 10.96.0.253
corednse2e.com. 600 IN A 10.96.0.254
corednse2e.com. 600 IN A 10.96.0.255
corednse2e.com. 600 IN A 10.96.0.50
corednse2e.com. 600 IN A 10.96.0.51
corednse2e.com. 600 IN A 10.96.0.52
corednse2e.com. 600 IN A 10.96.0.53
corednse2e.com. 600 IN A 10.96.0.54
corednse2e.com. 600 IN A 10.96.0.55
corednse2e.com. 600 IN A 10.96.0.57
corednse2e.com. 600 IN A 10.96.0.58
corednse2e.com. 600 IN A 10.96.0.59
corednse2e.com. 600 IN A 10.96.0.60
corednse2e.com. 600 IN A 10.96.0.61
corednse2e.com. 600 IN A 10.96.0.62
corednse2e.com. 600 IN A 10.96.0.63
corednse2e.com. 600 IN A 10.96.0.64
corednse2e.com. 600 IN A 10.96.0.65
corednse2e.com. 600 IN A 10.96.0.66
corednse2e.com. 600 IN A 10.96.0.67
corednse2e.com. 600 IN A 10.96.0.68
corednse2e.com. 600 IN A 10.96.0.69
corednse2e.com. 600 IN A 10.96.0.70
corednse2e.com. 600 IN A 10.96.0.71
corednse2e.com. 600 IN A 10.96.0.72
corednse2e.com. 600 IN A 10.96.0.73
corednse2e.com. 600 IN A 10.96.0.74
corednse2e.com. 600 IN A 10.96.0.75
corednse2e.com. 600 IN A 10.96.0.76
corednse2e.com. 600 IN A 10.96.0.77
corednse2e.com. 600 IN A 10.96.0.78
corednse2e.com. 600 IN A 10.96.0.79
corednse2e.com. 600 IN A 10.96.0.80
corednse2e.com. 600 IN A 10.96.0.81
corednse2e.com. 600 IN A 10.96.0.82
corednse2e.com. 600 IN A 10.96.0.83
corednse2e.com. 600 IN A 10.96.0.84
corednse2e.com. 600 IN A 10.96.0.85
corednse2e.com. 600 IN A 10.96.0.86
corednse2e.com. 600 IN A 10.96.0.87
corednse2e.com. 600 IN A 10.96.0.88
corednse2e.com. 600 IN A 10.96.0.89
corednse2e.com. 600 IN A 10.96.0.90
;; Query time: 38 msec
;; SERVER: 168.63.129.16#53(168.63.129.16)
;; WHEN: Thu Oct 26 05:32:22 UTC 2023
;; MSG SIZE rcvd: 1056
With the proposed fix, below is the expected response.
~----------------------------------------------------------------------------
kubectl exec dnsutils -- dig corednse2e.com +ignore +noedns +search +noshowsearch +time=10 +tries=6 @10.96.0.10
; <<>> DiG 9.9.5-9+deb8u19-Debian <<>> corednse2e.com +ignore +noedns +search +noshowsearch +time=10 +tries=6 @10.96.0.10
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 49592
;; flags: qr tc rd ra; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 0
;; QUESTION SECTION:
;corednse2e.com. IN A
;; Query time: 82 msec
;; SERVER: 10.96.0.10#53(10.96.0.10)
;; WHEN: Thu Oct 26 05:35:25 UTC 2023
;; MSG SIZE rcvd: 32
Hmm, I'm not sure about this. It seems like we explicitly handle the provided count value being too large so this may be a breaking change. See unpackRRslice: https://github.com/miekg/dns/blob/0d504a67cffe8a05b3ff4057c7602b4afe0b26ef/msg.go#L663-L666
What behaviour do other DNS servers do in this case?
Ultimately isn't the bug really that the server hasn't set the TC bit even though the message is truncated?
@tmthrgd - Thank you for taking a look at this request.
We can easily repro this issue with a simple test case in latest coredns.
Simple test case to repro - https://github.com/SriHarsha001/fixcorednsudpbug/blob/34ee289d240f0dfa7d0998b0bfe6da2efae157b7/plugin/pkg/proxy/proxy_test.go#L133
A bit complex test cases to repro - https://github.com/SriHarsha001/fixcorednsudpbug/blob/1718b1e5c82997f2a520273df7f0f92b5deb86b8/plugin/pkg/proxy/proxy_test.go#L230
Below writeup is referring this func unpackRRslice
for i := 0; i < l; i++ {
off1 := off
r, off, err = UnpackRR(msg, off)
if err != nil {
off = len(msg)
break
}
// If offset does not increase anymore, l is a lie
if off1 == off {
break
}
When the domain name (example123.org.) has 15 characters - ~------------------------------------------------------------------------------------------- In 15th loop - off will be 512, off1 will be 482, so the if condition will be false In 16th loop - off will be 512, off1 will be 512, control goes into UnpackRR(msg, off) function. In this function, unpackHeader(msg, off) is called, which has below condition to check if we are the end of the array. if off == len(msg) { return hdr, off, msg, nil } Here off == len(msg) which is 512 (for Default UDP size) so response gets truncated (if upstream had sent more records).
Similarly
when the domain name (example1234567.org.) has 19 characters - ~------------------------------------------------------------------------------------------- In 13th loop - off will be 512, off1 will be 478, so the if condition will be false In 14th loop - off will be 512, off1 will be 512, control goes into UnpackRR(msg, off) function. In this function, unpackHeader(msg, off) is called, which has below condition to check if we are the end of the array. if off == len(msg) { return hdr, off, msg, nil } Here off == len(msg) which is 512 (Default UDP size) so response gets truncated (if upstream had sent more records)
So in these cases, if upstream has sent more records and no TC flag is set, records are truncated and TC flag is not set (in setHdr). This is the failing scenario.
As we can see from the below table, this problem can be seen for default UDP size when number of characters in domain name is 02, 15, 19, 36 (when off is 512). We can also repro this issue for all other numbers by setting Edns0 in the request.
No of Char Question Each loop Sequence of off ~----------------------------------------------------------------------- 02 Characters 19 Q +17 off in loops - 36, .... 512 03 Characters 20 Q +18 off in loops - 38, .... 524 04 Characters 21 Q +19 off in loops - 40, .... 534 05 Characters 22 Q +20 off in loops - 42, .... 522 06 Characters 23 Q +21 off in loops - 44, .... 548 07 Characters 24 Q +22 off in loops - 46, .... 530, 552 08 Characters 25 Q +23 off in loops - 48, .... 508, 531,554 09 Characters 26 Q +24 off in loops - 50, .... 506,530,554 10 Characters 27 Q +25 off in loops - 52, .... 552 11 Characters 28 Q +26 off in loops - 54, .... 520 12 Characters 29 Q +27 off in loops - 56, .... 515 13 Characters 30 Q +28 off in loops - 58, .... 506 14 Characters 31 Q +29 off in loops - 60, 89, 118 .... 524 15 Characters 32 Q +30 off in loops - 62, 92, 122 .... 512 16 Characters 33 Q +31 off in loops - 64, .... 529 17 Characters 34 Q +32 off in loops - 66, .... 514 18 Characters 35 Q +33 off in loops - 68, .... 530 19 Characters 36 Q +34 off in loops - 70, 104, 138.... 512 20 Characters 37 Q +35 off in loops - 72, 107, 142.... 527 21 Characters 38 Q +36 off in loops - 74, .... 542 22 Characters 39 Q +37 off in loops - 76, .... 542 23 Characters 40 Q +38 off in loops - 78, ............ 534 24 Characters 41 Q +39 off in loops - 80, ............ 509 25 Characters 42 Q +40 off in loops - 82, ............ 522 26 Characters 43 Q +41 off in loops - 84, ............ 535 27 Characters 44 Q +42 off in loops - 86, ............ 506 28 Characters 45 Q +43 off in loops - 88, ............ 518 29 Characters 46 Q +44 off in loops - 90, ............ 530 30 Characters 47 Q +45 off in loops - 92, ............ 542 31 Characters 48 Q +46 off in loops - 94, ............ 508 32 Characters 49 Q +47 off in loops - 96, ............ 519 33 Characters 50 Q +48 off in loops - 98, ............ 530 34 Characters 51 Q +49 off in loops - 100, ............ 541 35 Characters 52 Q +50 off in loops - 102, ............ 502 36 Characters 53 Q +51 off in loops - 104, ............ 512 37 Characters 54 Q +52 off in loops - 106, ............ 522 38 Characters 55 Q +53 off in loops - 108, ............ 532 39 Characters 56 Q +54 off in loops - 110, ............ 542 40 Characters 57 Q +55 off in loops - 112, ............ 497 41 Characters 58 Q +56 off in loops - 114, ............ 506 42 Characters 59 Q +57 off in loops - 116, ............ 515 43 Characters 60 Q +58 off in loops - 118, ............ 524 44 Characters 61 Q +59 off in loops - 120, ............ 533 45 Characters 62 Q +60 off in loops - 122, ............ 542
I hope I was able to explain in a way that can be understood clearly.
@tmthrgd - Just a gentle follow up. Requesting a review when you get few minutes.
I don't fully understand what being said here. You start with
If there is a misbehaving upstream server which does not set TC bit and sends back a response length greater than pc.c.UDPSize or 512 default size, then there is a corner case where offset is equal to len(msg) as calculated in unpackHeader defined here github.com/miekg/msg_helpers.go. In this case, there is no Overflow/ErrBuff error returned, so the response will be truncated without TC bit set.
so, I send a UDP message without EDNS to an upstream using UDP and it sends a reply exceeding 512B as if it assumes my buf is 4096B or something?
That is a clear violation of protocol and there will be no work around added for that here. Please complain to the upstream.
And then disregarding above what would then be buggy here? How would a simple unit test look like in this library that fails now and will be fixed with this PR?
Just for clarity. The problem which @SriHarsha001 is trying to solve here is 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.
And then disregarding above what would then be buggy here? How would a simple unit test look like in this library that fails now and will be fixed with this PR?
We can repro the issue by just changing the domain name from "example." to "example123.org." and requesting for TypeA in this test case - TestServingLargeResponses https://github.com/miekg/dns/blob/a16092f374522536ad637e5a3766585df2c160f0/server_test.go#L627
Change line number - 628 and 629 HandleFunc("example123.org.", HelloServerLargeResponse) defer HandleRemove("example123.org.")
Change line number - 639 m.SetQuestion("example123.org.", TypeA)
this needs a unit test that demonstrates the need
Yes, I have added unit tests to test the edge cases. I kindly request you to review when you get few minutes.
Just for clarity. The problem which @SriHarsha001 is trying to solve here is 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.
Thank you very much @rdrozhdzh
Thanks for the unit test.
I'll probably merge this pr first: #1507 which may or may not introduce a conflict here (FYI)
Thank you for your review. That PR has significant changes.
Just FYI... I did a quick test on unpack_cryptobyte branch and was able to repro the issue.
Thanks for the unit test. I'll probably merge this pr first: #1507 which may or may not introduce a conflict here (FYI)
Thank you for your review. That PR has significant changes.
Just FYI... I did a quick test on unpack_cryptobyte branch and was able to repro the issue.
Yes this is entirely expected. I'm not trying to change behaviour very much in that PR, merely the implementation. Future PRs can address this and other potential issues.
Having now worked on #1507, I better understand the issue you've observed (#1492) and agree a fix makes sense. It might be a breaking for a very specific corner case, but given other errors that are more likely to occur for truncated messages I think it's fine.
As part of the changes for #1507, the fix is actually much simpler than what you've proposed here. I'll open a new PR once that's been merged to address this for you.
Having now worked on #1507, I better understand the issue you've observed (#1492) and agree a fix makes sense. It might be a breaking for a very specific corner case, but given other errors that are more likely to occur for truncated messages I think it's fine.
As part of the changes for #1507, the fix is actually much simpler than what you've proposed here. I'll open a new PR once that's been merged to address this for you.
Sure sure, thank you very much. I will look out for updates in this lib.
Having now worked on #1507, I better understand the issue you've observed (#1492) and agree a fix makes sense. It might be a breaking for a very specific corner case, but given other errors that are more likely to occur for truncated messages I think it's fine. As part of the changes for #1507, the fix is actually much simpler than what you've proposed here. I'll open a new PR once that's been merged to address this for you.
@tmthrgd - Just a gentle follow up on this request. Tracking this https://github.com/miekg/dns/pull/1507, I see the PR is not merged. I am guessing the work is in progress.
@miekg and @tmthrgd - Just a gentle follow up on this request. Do we have an expected timelines on merge of https://github.com/miekg/dns/pull/1507. Any information will be greatly appreciated.
@miekg and @tmthrgd - Please don't mind me pinging couple of times here. I wanted to follow up on this PR. Please let me know if I can be of any help. Any information will be greatly appreciated.
came back to this to see if I could make sense of it for the 3rd time. The answers is no. First off all the test is completely flawed. But I thought I might be able to fix it, i did and then ran the test again and it still failed. because this is not all all how this package works.
the changed test:
package dns
import (
"fmt"
"net"
"testing"
)
func TestUnPackForErrBuf(t *testing.T) {
name := "example123.org."
i := 17
expectErr := true
m := new(Msg)
m.SetQuestion(name, TypeA)
m.Authoritative = true
for j := 0; j < i; j++ {
a := &A{Hdr: RR_Header{Name: name, Rrtype: TypeA, Class: ClassINET}, A: net.IPv4(127, 0, 0, byte(j+1)).To4()}
m.Answer = append(m.Answer, a)
}
fmt.Printf("%d\n%s", m.Len(), m)
packedmsg, err := m.Pack()
println("LEN PACKED", len(packedmsg))
if len(packedmsg) > MinMsgSize {
packedmsg = packedmsg[:MinMsgSize]
}
m1 := new(Msg)
err = m1.Unpack(packedmsg)
println(m1.String())
if expectErr {
println(err)
if err != ErrBuf {
t.Error("Expected ErrBuf due to truncation")
}
}
}
when running this prints
=== RUN TestUnPackForErrBuf
542
;; opcode: QUERY, status: NOERROR, id: 15960
;; flags: aa rd; QUERY: 1, ANSWER: 17, AUTHORITY: 0, ADDITIONAL: 0
;; QUESTION SECTION:
;example123.org. IN A
;; ANSWER SECTION:
example123.org. 0 IN A 127.0.0.1
example123.org. 0 IN A 127.0.0.2
example123.org. 0 IN A 127.0.0.3
example123.org. 0 IN A 127.0.0.4
example123.org. 0 IN A 127.0.0.5
example123.org. 0 IN A 127.0.0.6
example123.org. 0 IN A 127.0.0.7
example123.org. 0 IN A 127.0.0.8
example123.org. 0 IN A 127.0.0.9
example123.org. 0 IN A 127.0.0.10
example123.org. 0 IN A 127.0.0.11
example123.org. 0 IN A 127.0.0.12
example123.org. 0 IN A 127.0.0.13
example123.org. 0 IN A 127.0.0.14
example123.org. 0 IN A 127.0.0.15
example123.org. 0 IN A 127.0.0.16
example123.org. 0 IN A 127.0.0.17
LEN PACKED 542
;; opcode: QUERY, status: NOERROR, id: 15960
;; flags: aa rd; QUERY: 1, ANSWER: 16, AUTHORITY: 0, ADDITIONAL: 0
;; QUESTION SECTION:
;example123.org. IN A
;; ANSWER SECTION:
example123.org. 0 IN A 127.0.0.1
example123.org. 0 IN A 127.0.0.2
example123.org. 0 IN A 127.0.0.3
example123.org. 0 IN A 127.0.0.4
example123.org. 0 IN A 127.0.0.5
example123.org. 0 IN A 127.0.0.6
example123.org. 0 IN A 127.0.0.7
example123.org. 0 IN A 127.0.0.8
example123.org. 0 IN A 127.0.0.9
example123.org. 0 IN A 127.0.0.10
example123.org. 0 IN A 127.0.0.11
example123.org. 0 IN A 127.0.0.12
example123.org. 0 IN A 127.0.0.13
example123.org. 0 IN A 127.0.0.14
example123.org. 0 IN A 127.0.0.15
example123.org. 0 IN A 127.0.0.16
(0x0,0x0)
dns_truncated_test.go:35: Expected ErrBuf due to truncation
--- FAIL: TestUnPackForErrBuf (0.00s)
FAIL
exit status 1
FAIL github.com/miekg/dns 0.002s
which is the result of you have a message with 17 As and the FORCEFULLY SET THE BUFFER to 512 and then unpack, and you get back a PERFECTLY FINE message with 16 As. I don't know why you think something should trigger as if the thing should know that we should get 17 RRs? The m1 := new(Msg) is how things should work. That you are repurposing an old *Msg is 100% wrong and any code doing that is wrong.
All the code around unpack is also completely clear about not trusting counters:
// Qdcount, Ancount, Nscount, Arcount can't be trusted, as they are attacker controlled.
// This means we can't use them to pre-allocate slices.
// The header counts might have been wrong so we need to update it
dh.Arcount = uint16(len(dns.Extra))
so not only is this not a bug, any code that does that thing that you think is OK, is in fact not OK and completely wrong