x/net/dnsmessage: dots inside dns name label should be considered invalid

[mateusz834]

func main() {
	parser := dnsmessage.Parser{}
	msg := []byte{}

	// Header
	msg = binary.BigEndian.AppendUint16(msg, 0)
	msg = binary.BigEndian.AppendUint16(msg, 0)
	msg = binary.BigEndian.AppendUint16(msg, 1)
	msg = binary.BigEndian.AppendUint16(msg, 0)
	msg = binary.BigEndian.AppendUint16(msg, 0)
	msg = binary.BigEndian.AppendUint16(msg, 0)

	nameIN := []byte{2, '.', '.', 0} // valid RFC 1035 name

	// Question
	msg = append(msg, nameIN...)
	msg = binary.BigEndian.AppendUint16(msg, 1)
	msg = binary.BigEndian.AppendUint16(msg, 1)

	hdr, err := parser.Start(msg)
	if err != nil {
		panic(err)
	}

	q, err := parser.Question()
	if err != nil {
		panic(err)
	}

	fmt.Println(q.Name.Data[:q.Name.Length]) // prints [46 46 46] (three dots "...")

	builder := dnsmessage.NewBuilder(nil, hdr)
	err = builder.StartQuestions()
	if err != nil {
		panic(err)
	}

	err = builder.Question(q)
	if err != nil {
		panic(err) // this panics
	}
}

[46 46 46]
panic: Name: zero length segment

goroutine 1 [running]:
main.main()
        /tmp/aaaa/main.go:49 +0x525
exit status 2

Unpacking []byte{2, '.', '.', 0} causes the Name field of dnsmessage.Question to be: [46 46 46] (which is an invalid name). And thus while packing that name inside builder.Question we get an error (even thought the parser did not report any error while unpacking).

[]byte{2, '.', '.', 0} is a valid RFC 1035 name, but this package does not handle it correctly. (we are not able to express this name using the Name struct)
I don't know what we should do with that. We might just return an error (while unpacking) when there is a dot inside a label (I don't know a better solution to fix that).

This is not a critical bug, I found it while testing this package. Any Thoughts?

[mateusz834]

Or maybe it could lead to some security issues?

// Resource Header
msg = append(msg, nameIN...)
msg = binary.BigEndian.AppendUint16(msg, uint16(dnsmessage.TypeCNAME))
msg = binary.BigEndian.AppendUint16(msg, uint16(dnsmessage.ClassINET))
msg = binary.BigEndian.AppendUint32(msg, 1)

nameCNAME := []byte{12, 's', 't', 'h', '.', 'i', 'n', 't', 'e', 'r', 'n', 'a', 'l', 2, 'g', 'o', 3, 'd', 'e', 'v', 0} // valid RFC 1035 name
msg = binary.BigEndian.AppendUint16(msg, uint16(len(nameCNAME)))
msg = append(msg, nameCNAME...)

_, err := parser.Start(msg)
if err != nil {
	panic(err)
}

err = parser.SkipAllQuestions()
if err != nil {
	panic(err)
}

_, err = parser.AnswerHeader()
if err != nil {
	panic(err)
}

cname, err := parser.CNAMEResource()
if err != nil {
	panic(err)
}

fmt.Println(cname.CNAME.Data[:cname.CNAME.Length])
fmt.Println(cname) 

[115 116 104 46 105 110 116 101 114 110 97 108 46 103 111 46 100 101 118 46]
{sth.internal.go.dev.}

[]byte{12, 's', 't', 'h', '.', 'i', 'n', 't', 'e', 'r', 'n', 'a', 'l', 2, 'g', 'o', 3, 'd', 'e', 'v', 0} (subdomain of go.dev. was interpreted as subdomain of internal.go.dev.).

Having two different interpretations in never good in terms of security.

[gopherbot]

Change https://go.dev/cl/443215 mentions this issue: dns/dnsmessage: reject names with dots inside label

[mateusz834]

We might also try to add \. escape as defined in RFC 1035 (and probably \DDD Edit: \\). But the array inside the Name struct would not be able to hold all possible names.

[dr2chase]

@neild, can you give this a look?

[rolandshoemaker]

Hey @mateusz834 I'd like to try to get this fixed for 1.20, but I think we should be implementing the full 1034 escaping behavior (similar to what miekg/dns does), rather than just hard failing on this one specific. I'm happy to review if you'd like to update your CL to implement this, but we have a hard freeze at the end of next week before which we need to land the change. If you're short on time I'm also happy to take over and implement this behavior if you don't think you can get to it before then.

[mateusz834]

Edit: I also found a discussion about the same issue here: #10631 (but marked as net).

@rolandshoemaker I can try to implement this, give me few days.

full 1034 escaping behavior

You mean RFC 1035? Only these two?

\X              where X is any character other than a digit (0-9), is
                used to quote that character so that its special meaning
                does not apply.  For example, "\." can be used to place
                a dot character in a label.

\DDD            where each D is a digit is the octet corresponding to
                the decimal number described by DDD.  The resulting
                octet is assumed to be text and is not checked for
                special meaning.

How to handle case in which we have to unpack a 255B (dns encoded name) that requires one character to be encoded as \DDD. (It will not fit inside the Name struct, it will take 257B) Should we just return an error?

[rolandshoemaker]

Oh bah, I missed we use a sized byte slice to avoid allocations, this ends up being more complicated than I anticipated. Perhaps just failing out here is the lowest impact fix we can reasonably apply in the short term. That said I think we should be failing on all non-LDH characters though, since otherwise we'll still be mis-parsing other special characters.

[mateusz834]

@rolandshoemaker Note that the net package always checks for invalid hostnames:

go/src/net/dnsclient.go

Lines 72 to 75 in a11cd6f

	// isDomainName checks if a string is a presentation-format domain name
	// (currently restricted to hostname-compatible "preferred name" LDH labels and
	// SRV-like "underscore labels"; see golang.org/issue/12421).
	func isDomainName(s string) bool {

This function is executed for every input/output hostname. So let's say that MX query returns []byte{3, 11, 22, 33, 2, 'g', 'o', 3, 'd', 'e', 'v', 0}. This will be rejected in LookupMX by isDomainName function. So this issue affects the net package only in case when someone sends a dot inside a label. []byte{12, 's', 't', 'h', '.', 'i', 'n', 't', 'e', 'r', 'n', 'a', 'l', 2, 'g', 'o', 3, 'd', 'e', 'v', 0}

Edit:

That said I think we should be failing on all non-LDH characters though, since otherwise we'll still be mis-parsing other special characters.

If we really need something like that we probably should reject dots and ASCII characters: char < ' ' || char > '~'. Rejecting non-LDH characters can break backwards compatibility.

[mateusz834]

Do we want to do something with this for go 1.21?

[ianlancetaylor]

ping @rolandshoemaker