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.
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
net: packDomainName encodes "." as "\0\0" instead of "\0" #14372
Comments
Always check your errors, especially in unit tests. It hurts the credibility of a bug report when you show code that doesn't work and that code is intentionally throwing away what might be a big clue if not the answer. Once you do that, which version of Go? |
Where error in this case is apparently an |
I didn't include that because it isn't part of a minimal test case. For what it is worth, this test has the exact same output: func TestDNSPackUnpack(t *testing.T) {
want := dnsMsg{question: []dnsQuestion{{
Name: ".",
Qtype: dnsTypeNULL,
Qclass: dnsClassINET,
}}}
b, ok := want.Pack()
if !ok {
t.Error("Packing failed.")
}
var got dnsMsg
ok = got.Unpack(b)
if !ok {
t.Error("Unpacking failed.")
}
if !reflect.DeepEqual(got, want) {
t.Errorf("got = %+v, want = %+v", got, want)
}
} |
A minimal test case includes checking errors if there are errors to be checked. Otherwise you spend more time having discussions like this one, asking what happens if you do check the errors. |
/cc @mikioh |
I'm not sure what you are trying to archive. Both methods are just for internal use and have no control knob for enabling/disabling message compression described in RFC 1035. I didn't take a look at your code but I guess that perhaps you are comparing a vanilla message with a compressed message. |
@mikioh, |
I am fairly sure that the problem is a simple off by one error with the offset. |
It is actually an off by two problem. 10 is A in hex. 2560 is A00 in hex. |
Never mind, it is off by one. 00 is one byte. |
Can you please provide a DNS wire message that shows us the details of your DNS related corruption What we need to do first is to address your issue, and to fix the issue for Go 1.6.1 if it's a real problem or a root cause of vulnerability for the exposed APIs. Thanks. |
@mikioh, I think I have identified the problem and am working on a patch. |
Thanks in advance for the patch. Feel free to send your CL for review. |
Related issues:
|
Does it make sense to have the packing routines have go-fuzz thrown at them? |
Yes, though it sounds scary... but it's a good time for overhauling DNS message parsers at yard. |
CL https://golang.org/cl/19623 mentions this issue. |
@iangudger and @mdempsky, Thanks for the quick fix. I'm not sure whether the fix deserves to go in Go 1.6.1. Please change the milestone to Go 1.6.1 if you think so. |
I don't feel strongly either way on backporting for 1.6.1. The bugs have been there at least since 1.5, so I don't think it can be that severe/widespread. |
dnsMsg.Pack() and/or dnsMsg.Unpack() in the net package are broken.
This test fails:
Output:
The problem is that Qtype and Qclass get corrupted. The question starts off as
{Name:. Qtype:10 Qclass:1}
and after a Pack/Unpack cycle turn into{Name: Qtype:0 Qclass:2560}
.The text was updated successfully, but these errors were encountered: