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

x/net/dns: invalid assumptions about domain names and character strings #24288

Open
gibson042 opened this Issue Mar 7, 2018 · 5 comments

Comments

Projects
None yet
4 participants
@gibson042
Copy link
Contributor

gibson042 commented Mar 7, 2018

What version of Go are you using (go version)?

go version go1.10 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What did you do?

Reviewed go cl 37879 and dns/dnsmessage/message.go as of 2018-03-06.

What did you expect to see?

Correct handling of DNS data.

What did you see instead?

  • func (n *Name) pack incorrectly assumes that terminal dots imply absolute names, falsely accepting names that end with escaped dots (e.g., example\.).
  • func (n *Name) unpack assumes that label contents can be directly copied from wire format to presentation format, and therefore incorrectly converts dots in labels to label-separating dots (e.g., equating the distinct names a.b.example. [1 0x61 1 0x62 7 0x65 0x78 0x61 0x6d 0x70 0x6c 0x65 0] and a\.b.example. [3 0x61 0x2E 0x62 7 0x65 0x78 0x61 0x6d 0x70 0x6c 0x65 0]) and produces invalid domain names by naïvely including backslashes and control characters.
  • type TXTResource incorrectly assumes that TXT RDATA is a single string (when it reality it is a sequence of character strings 1, each of which is limited to 255 octets in wire format 2), making it impossible to differentiate TXT "" "" (two empty character strings) from TXT "" (one empty character string). The associated pack/unpack code also fails to reject overly-long strings (e.g., "aa-ab-ac-…-az-ba-bb-bc-…-zx-zy-zz"), instead naïvely combining/splitting 255-octet chunks.

@gopherbot gopherbot added this to the Unreleased milestone Mar 7, 2018

@andybons

This comment has been minimized.

Copy link
Member

andybons commented Mar 7, 2018

@iangudger

This comment has been minimized.

Copy link
Contributor

iangudger commented Mar 7, 2018

x/net/dns/dnsmessage is functionally based on the code currently in the net package. I don't believe any of these are new problems. x/net/dns/dnsmessage is mostly focused on improving performance, but does contain some bug fixes. As far as I know, it is a strict improvement correctness-wise.

That said, I agree that these are problems and should be fixed. @gibson042, I have seen your golang.org/cl/73830. @mikioh requested that you port your changes to x/net/dns/dnsmessage. Is filing this issue your way of saying that you want me to port golang.org/cl/73830 to x/net/dns/dnsmessage before submitting golang.org/cl/37879?

@gibson042

This comment has been minimized.

Copy link
Contributor Author

gibson042 commented Mar 8, 2018

Is filing this issue your way of saying that you want me to port golang.org/cl/73830 to x/net/dns/dnsmessage before submitting golang.org/cl/37879?

If you are able to do so, that would be great. It wouldn't address the issues with character-strings RDATA, but those are logically separate from domain names anyway—I just wanted to get the issue documented more visibly.

And thank you for taking these issues seriously, even though they are rarely-encountered edge cases.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 9, 2018

Change https://golang.org/cl/99623 mentions this issue: dns/dnsmessage: fix handling of non-LDH domain names

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 12, 2018

Change https://golang.org/cl/100196 mentions this issue: dns/dnsmessage: correctly handle muultiple and >255 byte TXT records

gopherbot pushed a commit to golang/net that referenced this issue Mar 13, 2018

dns/dnsmessage: correctly handle multiple and >255 byte TXT records
Previously, we only accepted a single string for TXT records and then
chunked it into 255 byte segments. This is wrong. TXT records are a
sequence of strings, each up to 255 bytes.

Updates golang/go#24288

Change-Id: Ib2c085ec127ccecf0c7bda930100b667cabc1f4b
Reviewed-on: https://go-review.googlesource.com/100196
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.