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 <[email protected]>

* Describe why we use a specific size for the alpn buffer

Co-authored-by: Miek Gieben <[email protected]>

Author: shane-ns1

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)

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
 }
 

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{