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

Comments

Projects
None yet
6 participants
@iangudger
Copy link
Contributor

commented Feb 18, 2016

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

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

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

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2016

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

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

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

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

/cc @mikioh

@mikioh

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2016

@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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2016

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

@iangudger

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2016

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

@iangudger

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2016

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

@mikioh

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2016

@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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2016

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

@mikioh

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2016

@gopherbot

This comment has been minimized.

Copy link

commented Feb 18, 2016

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

@gopherbot gopherbot closed this in 952c2fd Feb 19, 2016

@mikioh

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Feb 19, 2016

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.
You can’t perform that action at this time.