From feda8772776574ffe91fb3d5359d3bf3b44c1dac Mon Sep 17 00:00:00 2001 From: Shane Kerr Date: Tue, 10 May 2022 18:40:09 +0200 Subject: [PATCH] Properly parse alpn values in SVCB (#1363) * Modify the SVCBAlpn to properly parse/print * Remove debug * Change SVCB test from reflect to loop * Refactor SVCB code to reduce indentation * When stringifying SVCBAlpn, use strings.Builder for whole process * Update comment in svcb.go Co-authored-by: Miek Gieben * Describe why we use a specific size for the alpn buffer Co-authored-by: Miek Gieben --- parse_test.go | 8 +++-- svcb.go | 92 ++++++++++++++++++++++++++++++++++++++++++++++++--- svcb_test.go | 42 +++++++++++++++++++++++ 3 files changed, 136 insertions(+), 6 deletions(-) diff --git a/parse_test.go b/parse_test.go index 07d254bd..63e0fb53 100644 --- a/parse_test.go +++ b/parse_test.go @@ -1650,8 +1650,8 @@ func TestParseSVCB(t *testing.T) { `example.com. SVCB 1 foo.example.com. ipv6hint="2001:db8::1,2001:db8::53:1"`: `example.com. 3600 IN SVCB 1 foo.example.com. ipv6hint="2001:db8::1,2001:db8::53:1"`, `example.com. SVCB 1 example.com. ipv6hint="2001:db8::198.51.100.100"`: `example.com. 3600 IN SVCB 1 example.com. ipv6hint="2001:db8::c633:6464"`, `example.com. SVCB 16 foo.example.org. alpn=h2,h3-19 mandatory=ipv4hint,alpn ipv4hint=192.0.2.1`: `example.com. 3600 IN SVCB 16 foo.example.org. alpn="h2,h3-19" mandatory="ipv4hint,alpn" ipv4hint="192.0.2.1"`, - `example.com. SVCB 16 foo.example.org. alpn="f\\\\oo\\,bar,h2"`: `example.com. 3600 IN SVCB 16 foo.example.org. alpn="f\\\\oo\\,bar,h2"`, - `example.com. SVCB 16 foo.example.org. alpn=f\\\092oo\092,bar,h2`: `example.com. 3600 IN SVCB 16 foo.example.org. alpn="f\\\092oo\092,bar,h2"`, + `example.com. SVCB 16 foo.example.org. alpn="f\\\\oo\\,bar,h2"`: `example.com. 3600 IN SVCB 16 foo.example.org. alpn="f\\\092oo\\\044bar,h2"`, + `example.com. SVCB 16 foo.example.org. alpn=f\\\092oo\092,bar,h2`: `example.com. 3600 IN SVCB 16 foo.example.org. alpn="f\\\092oo\\\044bar,h2"`, // From draft-ietf-add-ddr-06 `_dns.example.net. SVCB 1 example.net. alpn=h2 dohpath=/dns-query{?dns}`: `_dns.example.net. 3600 IN SVCB 1 example.net. alpn="h2" dohpath="/dns-query{?dns}"`, } @@ -1704,6 +1704,10 @@ func TestParseBadSVCB(t *testing.T) { `1 . ipv4hint=`, // empty ipv4 `1 . port=`, // empty port `1 . echconfig=YUd`, // bad base64 + `1 . alpn=h\`, // unterminated escape + `1 . alpn=h2\\.h3`, // comma-separated list with bad character + `1 . alpn=h2,,h3`, // empty protocol identifier + `1 . alpn=h3,`, // final protocol identifier empty } for _, o := range evils { _, err := NewRR(header + o) diff --git a/svcb.go b/svcb.go index 316fca28..b9c6a952 100644 --- a/svcb.go +++ b/svcb.go @@ -342,13 +342,57 @@ func (s *SVCBMandatory) copy() SVCBKeyValue { // h.Hdr = dns.RR_Header{Name: ".", Rrtype: dns.TypeHTTPS, Class: dns.ClassINET} // e := new(dns.SVCBAlpn) // e.Alpn = []string{"h2", "http/1.1"} -// h.Value = append(o.Value, e) +// h.Value = append(h.Value, e) type SVCBAlpn struct { Alpn []string } -func (*SVCBAlpn) Key() SVCBKey { return SVCB_ALPN } -func (s *SVCBAlpn) String() string { return strings.Join(s.Alpn, ",") } +func (*SVCBAlpn) Key() SVCBKey { return SVCB_ALPN } + +func (s *SVCBAlpn) String() string { + // An ALPN value is a comma-separated list of values, each of which can be + // an arbitrary binary value. In order to allow parsing, the comma and + // backslash characters are themselves excaped. + // + // However, this escaping is done in addition to the normal escaping which + // happens in zone files, meaning that these values must be + // double-escaped. This looks terrible, so if you see a never-ending + // sequence of backslash in a zone file this may be why. + // + // https://datatracker.ietf.org/doc/html/draft-ietf-dnsop-svcb-https-08#appendix-A.1 + var str strings.Builder + for i, alpn := range s.Alpn { + // 4*len(alpn) is the worst case where we escape every character in the alpn as \123, plus 1 byte for the ',' separating the alpn from others + str.Grow(4*len(alpn) + 1) + if i > 0 { + str.WriteByte(',') + } + for j := 0; j < len(alpn); j++ { + e := alpn[j] + if ' ' > e || e > '~' { + str.WriteString(escapeByte(e)) + continue + } + switch e { + // We escape a few characters which may confuse humans or parsers. + case '"', ';', ' ': + str.WriteByte('\\') + str.WriteByte(e) + // The comma and backslash characters themselves must be + // doubly-escaped. We use `\\` for the first backslash and + // the escaped numeric value for the other value. We especially + // don't want a comma in the output. + case ',': + str.WriteString(`\\\044`) + case '\\': + str.WriteString(`\\\092`) + default: + str.WriteByte(e) + } + } + } + return str.String() +} func (s *SVCBAlpn) pack() ([]byte, error) { // Liberally estimate the size of an alpn as 10 octets @@ -383,7 +427,47 @@ func (s *SVCBAlpn) unpack(b []byte) error { } func (s *SVCBAlpn) parse(b string) error { - s.Alpn = strings.Split(b, ",") + if len(b) == 0 { + s.Alpn = []string{} + return nil + } + + alpn := []string{} + a := []byte{} + for p := 0; p < len(b); { + c, q := nextByte(b, p) + if q == 0 { + return errors.New("dns: svcbalpn: unterminated escape") + } + p += q + // If we find a comma, we have finished reading an alpn. + if c == ',' { + if len(a) == 0 { + return errors.New("dns: svcbalpn: empty protocol identifier") + } + alpn = append(alpn, string(a)) + a = []byte{} + continue + } + // If it's a backslash, we need to handle a comma-separated list. + if c == '\\' { + dc, dq := nextByte(b, p) + if dq == 0 { + return errors.New("dns: svcbalpn: unterminated escape decoding comma-separated list") + } + if dc != '\\' && dc != ',' { + return errors.New("dns: svcbalpn: bad escaped character decoding comma-separated list") + } + p += dq + c = dc + } + a = append(a, c) + } + // Add the final alpn. + if len(a) == 0 { + return errors.New("dns: svcbalpn: last protocol identifier empty") + } + s.Alpn = append(alpn, string(a)) return nil } diff --git a/svcb_test.go b/svcb_test.go index 0f940e7e..63a40102 100644 --- a/svcb_test.go +++ b/svcb_test.go @@ -95,6 +95,48 @@ func TestDecodeBadSVCB(t *testing.T) { } } +func TestPresentationSVCBAlpn(t *testing.T) { + tests := map[string]string{ + "h2": "h2", + "http": "http", + "\xfa": `\250`, + "some\"other,chars": `some\"other\\\044chars`, + } + for input, want := range tests { + e := new(SVCBAlpn) + e.Alpn = []string{input} + if e.String() != want { + t.Errorf("improper conversion with String(), wanted %v got %v", want, e.String()) + } + } +} + +func TestSVCBAlpn(t *testing.T) { + tests := map[string][]string{ + `. 1 IN SVCB 10 one.test. alpn=h2`: {"h2"}, + `. 2 IN SVCB 20 two.test. alpn=h2,h3-19`: {"h2", "h3-19"}, + `. 3 IN SVCB 30 three.test. alpn="f\\\\oo\\,bar,h2"`: {`f\oo,bar`, "h2"}, + `. 4 IN SVCB 40 four.test. alpn="part1,part2,part3\\,part4\\\\"`: {"part1", "part2", `part3,part4\`}, + `. 5 IN SVCB 50 five.test. alpn=part1\,\p\a\r\t2\044part3\092,part4\092\\`: {"part1", "part2", `part3,part4\`}, + } + for s, v := range tests { + rr, err := NewRR(s) + if err != nil { + t.Error("failed to parse RR: ", err) + continue + } + alpn := rr.(*SVCB).Value[0].(*SVCBAlpn).Alpn + if len(v) != len(alpn) { + t.Fatalf("parsing alpn failed, wanted %v got %v", v, alpn) + } + for i := range v { + if v[i] != alpn[i] { + t.Fatalf("parsing alpn failed, wanted %v got %v", v, alpn) + } + } + } +} + func TestCompareSVCB(t *testing.T) { val1 := []SVCBKeyValue{ &SVCBPort{