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.

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

Closed
iangudger opened this issue Feb 18, 2016 · 20 comments
Closed

net: packDomainName encodes "." as "\0\0" instead of "\0" #14372

iangudger opened this issue Feb 18, 2016 · 20 comments
Milestone

Comments

@iangudger
Copy link
Contributor

dnsMsg.Pack() and/or dnsMsg.Unpack() in the net package are broken.

This test fails:

func TestDNSPackUnpack(t *testing.T) {
    want := dnsMsg{question: []dnsQuestion{{
        Name:   ".",
        Qtype:  dnsTypeNULL,
        Qclass: dnsClassINET,
    }}}
    b, _ := want.Pack()
    var got dnsMsg
    got.Unpack(b)
    if !reflect.DeepEqual(got, want) {
        t.Errorf("got = %+v, want = %+v", got, want)
    }
}

Output:

--- FAIL: TestDNSPackUnpack (0.00s)
    dnsmsg_test.go:23: got = {dnsMsgHdr:{id:0 response:false opcode:0 authoritative:false truncated:false recursion_desired:false recursion_available:false rcode:0} question:[{Name: Qtype:0 Qclass:2560}] answer:[] ns:[] extra:[]}, want = {dnsMsgHdr:{id:0 response:false opcode:0 authoritative:false truncated:false recursion_desired:false recursion_available:false rcode:0} question:[{Name:. Qtype:10 Qclass:1}] answer:[] ns:[] extra:[]}
FAIL

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}.

@bradfitz
Copy link
Contributor

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?

@bradfitz
Copy link
Contributor

Where error in this case is apparently an ok bool, but same point. (aside: *dnsMsg.Pack should probably return an error instead of a bool)

@iangudger
Copy link
Contributor Author

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)
    }
}

@bradfitz
Copy link
Contributor

I didn't include that because it isn't part of a minimal test case.

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.

@bradfitz bradfitz added this to the Unplanned milestone Feb 18, 2016
@bradfitz
Copy link
Contributor

/cc @mikioh

@mikioh
Copy link
Contributor

mikioh commented Feb 18, 2016

@iangudger,

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.

@iangudger
Copy link
Contributor Author

@mikioh,
I was having DNS related corruption issues and I tracked them down to this problem. The issue is that packing and then unpacking a message isn't lossless and changes the data in a way that doesn't make any sense. I have looked over the executed code and I don't think that changing the data in this way is intended.

@iangudger
Copy link
Contributor Author

I am fairly sure that the problem is a simple off by one error with the offset.

@iangudger
Copy link
Contributor Author

It is actually an off by two problem. 10 is A in hex. 2560 is A00 in hex.

@iangudger
Copy link
Contributor Author

Never mind, it is off by one. 00 is one byte.

@mikioh
Copy link
Contributor

mikioh commented Feb 18, 2016

@iangudger,

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.

@iangudger
Copy link
Contributor Author

@mikioh,
I don't think this is a common case. Unfortunately I can't elaborate on what I am doing.

I think I have identified the problem and am working on a patch.

@mikioh
Copy link
Contributor

mikioh commented Feb 18, 2016

@iangudger,

Thanks in advance for the patch. Feel free to send your CL for review.

@mdempsky mdempsky changed the title net: dnsMsg.Pack() and/or dnsMsg.Unpack() broken net: packDomainName encodes "." as "\0\0" instead of "\0" Feb 18, 2016
@mdempsky
Copy link
Member

Related issues:

  1. packDomainName doesn't reject empty labels. E.g., it encodes "google..com" as "\6google\0\3com\0", which is invalid.
  2. unpackDomainName decodes "\6google3\com\0" to "google.com.", but decodes "\0" to just "". I think decoding to "." would be more idiomatic.

@dgryski
Copy link
Contributor

dgryski commented Feb 18, 2016

Does it make sense to have the packing routines have go-fuzz thrown at them?

@mikioh
Copy link
Contributor

mikioh commented Feb 18, 2016

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.

@mikioh
Copy link
Contributor

mikioh commented Feb 18, 2016

@gopherbot
Copy link

CL https://golang.org/cl/19623 mentions this issue.

@mikioh
Copy link
Contributor

mikioh commented Feb 19, 2016

@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.

@mdempsky
Copy link
Member

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.

@mikioh mikioh modified the milestones: Go1.7, Unplanned Feb 19, 2016
@golang golang locked and limited conversation to collaborators Feb 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants