miekg / dns

DNS library in Go
https://miek.nl/2014/august/16/go-dns-package
BSD 3-Clause "New" or "Revised" License
7.99k stars 1.13k forks source link

Trailing backslash results in non-FQDN targets #1528

Closed janik-cloudflare closed 7 months ago

janik-cloudflare commented 8 months ago

Calling NewRR(". 1 IN CNAME example.com") normally turns example.com into an FQDN, example.com., before storing it in the RR's Target variable.

However, NewRR(". 1 IN CNAME example.com\\") with a trailing backslash results in example.com\., which is not an FQDN since the last dot is escaped.

It seems to me like the parser should either add another dot in these cases, or (perhaps better) reject the trailing backslash with an error.

package main

import (
    "fmt"

    "github.com/miekg/dns"
)

func test(content string) {
    rr, err := dns.NewRR(content)
    if err != nil {
        panic(err)
    }

    target := rr.(*dns.CNAME).Target
    fmt.Printf("target %s is FQDN: %t\n", target, dns.IsFqdn(target))
}

func main() {
    test(`. 1 IN CNAME example.com`)
    test(`. 1 IN CNAME example.com\`)
}
$ go run ./...
target example.com. is FQDN: true
target example.com\. is FQDN: false

This may be related to #1384.

miekg commented 8 months ago

but \\ is legal, no? There is def. a problem here, but if we are going to reject \\ there is bound to be more stuff that will be deemed illegal in the future. Is blindly Fqdn-ing the real problem here? Or is doing Fqdn in the first place the actual, actual problem?

janik-cloudflare commented 8 months ago

An even number of slashes definitely seems legal.

Digging a bit, it looks like CNAME.parse() receives an example.com\ token. Maybe (*zlexer).Next() should return an error when escape is still true when returning? I need to look more into how (and if) this would work.

FQDN-ing seems fine to me since CNAME.parse() calls toAbsoluteName based on the provided origin, right?

miekg commented 8 months ago

(on my phone)

Returning an error when escape is true looks like a good thing to do. Unsure if there is RFC text on that corner case.

On Fri, 19 Jan 2024, 16:10 Janik Rabe, @.***> wrote:

An even number of slashes definitely seems legal.

Digging a bit, it looks like CNAME.parse() receives an example.com\ token. Maybe (*zlexer).Next() should return an error when escape is still true when returning? I need to look more into how (and if) this would work.

FQDN-ing seems fine to me since CNAME.parse() calls toAbsoluteName based on the provided origin, right?

— Reply to this email directly, view it on GitHub https://github.com/miekg/dns/issues/1528#issuecomment-1900597647, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACWIWZ3Q72K43FDUVOOICLYPKEGZAVCNFSM6AAAAABB6KWFNKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBQGU4TONRUG4 . You are receiving this because you commented.Message ID: @.***>

gibson042 commented 8 months ago

It's not a corner case in the RFC; CNAME RDATA format is just a \ and per RFC 1035 section 5.1:

The labels in the domain name are expressed as character strings and separated by dots. Quoting conventions allow arbitrary characters to be stored in domain names. Domain names that end in a dot are called absolute, and are taken as complete. Domain names which do not end in a dot are called relative; the actual domain name is the concatenation of the relative part with an origin specified in a $ORIGIN, $INCLUDE, or as an argument to the master file loading routine. A relative name is an error when no origin is available.

(where «\ is expressed in one or two ways: as a contiguous set of characters without interior spaces, or as a string beginning with a " and ending with a "… [and] \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»)

So in a zone file, . 1 IN CNAME example.com\ is simply invalid while . 1 IN CNAME example.com and . 1 IN CNAME example.com\\ are both valid, and the target domain name for each is relative to the current origin. NewRR promoting relative domain names in isolated RRs to be root-relative is a design decision, and should be (and AFAIK, is) applied universally—to have those example records respectively target "example.com." (i.e., a domain name in which the first label is "example", the second label is "com", and the third and final label is the DNS root label) and "example.com\092." or equivalently "example.com\\." (i.e., a domain name in which the first label is "example", the second label is the four ASCII characters "com\", and the third and final label is the DNS root label).

miekg commented 8 months ago

ack

So in a zone file, . 1 IN CNAME example.com\ is simply invalid while . 1 IN CNAME example.com and . 1 IN CNAME example.com\ are both valid

you agree with returning an error from the parser is escape == true then too?

gibson042 commented 8 months ago

Yes, I think NewRR(". 1 IN CNAME example.com\\") or any other attempt to parse an incomplete escape should return an error. :+1: