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
Open
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@gibson042
Copy link
Contributor

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 andybons added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 7, 2018
@andybons
Copy link
Member

andybons commented Mar 7, 2018

/cc @bradfitz @iangudger

@iangudger
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link

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

@gopherbot
Copy link

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

@andybons andybons added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 13, 2018
gopherbot pushed a commit to golang/net that referenced this issue Mar 13, 2018
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
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants