Repository: miekg/dns
Pull request #1353 (2022-03-31 08:37:59)

Fix RSAMD5 keytag calucation.
Of course the wording was changed (for the better) in an errata:
https://www.rfc-editor.org/errata/eid193

We still followed the original RFC4034 text. Note I haven't given this
much thought, just changed the 2 into a 3 and ran the test.

Fixes: #1352

Signed-off-by: Miek Gieben <miek@miek.nl>



Patch:
diff --git a/dnssec.go b/dnssec.go
index e95f0377..ea01aa81 100644
--- a/dnssec.go
+++ b/dnssec.go
@@ -140,12 +140,12 @@ func (k *DNSKEY) KeyTag() uint16 {
 	var keytag int
 	switch k.Algorithm {
 	case RSAMD5:
-		// Look at the bottom two bytes of the modules, which the last
-		// item in the pubkey.
 		// This algorithm has been deprecated, but keep this key-tag calculation.
+		// Look at the bottom two bytes of the modules, which the last item in the pubkey.
+		// See https://www.rfc-editor.org/errata/eid193 .
 		modulus, _ := fromBase64([]byte(k.PublicKey))
 		if len(modulus) > 1 {
-			x := binary.BigEndian.Uint16(modulus[len(modulus)-2:])
+			x := binary.BigEndian.Uint16(modulus[len(modulus)-3:])
 			keytag = int(x)
 		}
 	default:
diff --git a/msg_helpers.go b/msg_helpers.go
index 10754c8b..b049028b 100644
--- a/msg_helpers.go
+++ b/msg_helpers.go
@@ -558,6 +558,16 @@ func packDataNsec(bitmap []uint16, msg []byte, off int) (int, error) {
 	if len(bitmap) == 0 {
 		return off, nil
 	}
+	if off > len(msg) {
+		return off, &Error{err: "overflow packing nsec"}
+	}
+	toZero := msg[off:]
+	if maxLen := typeBitMapLen(bitmap); maxLen < len(toZero) {
+		toZero = toZero[:maxLen]
+	}
+	for i := range toZero {
+		toZero[i] = 0
+	}
 	var lastwindow, lastlength uint16
 	for _, t := range bitmap {
 		window := t / 256
diff --git a/svcb.go b/svcb.go
index 3344253c..ff5e0108 100644
--- a/svcb.go
+++ b/svcb.go
@@ -13,16 +13,17 @@ import (
 // SVCBKey is the type of the keys used in the SVCB RR.
 type SVCBKey uint16
 
-// Keys defined in draft-ietf-dnsop-svcb-https-01 Section 12.3.2.
+// Keys defined in draft-ietf-dnsop-svcb-https-08 Section 14.3.2.
 const (
-	SVCB_MANDATORY       SVCBKey = 0
-	SVCB_ALPN            SVCBKey = 1
-	SVCB_NO_DEFAULT_ALPN SVCBKey = 2
-	SVCB_PORT            SVCBKey = 3
-	SVCB_IPV4HINT        SVCBKey = 4
-	SVCB_ECHCONFIG       SVCBKey = 5
-	SVCB_IPV6HINT        SVCBKey = 6
-	svcb_RESERVED        SVCBKey = 65535
+	SVCB_MANDATORY SVCBKey = iota
+	SVCB_ALPN
+	SVCB_NO_DEFAULT_ALPN
+	SVCB_PORT
+	SVCB_IPV4HINT
+	SVCB_ECHCONFIG
+	SVCB_IPV6HINT
+
+	svcb_RESERVED SVCBKey = 65535
 )
 
 var svcbKeyToStringMap = map[SVCBKey]string{
@@ -31,7 +32,7 @@ var svcbKeyToStringMap = map[SVCBKey]string{
 	SVCB_NO_DEFAULT_ALPN: "no-default-alpn",
 	SVCB_PORT:            "port",
 	SVCB_IPV4HINT:        "ipv4hint",
-	SVCB_ECHCONFIG:       "echconfig",
+	SVCB_ECHCONFIG:       "ech",
 	SVCB_IPV6HINT:        "ipv6hint",
 }
 
@@ -167,10 +168,14 @@ func (rr *SVCB) parse(c *zlexer, o string) *ParseError {
 		}
 		l, _ = c.Next()
 	}
+
+	// "In AliasMode, records SHOULD NOT include any SvcParams, and recipients MUST
+	// ignore any SvcParams that are present."
+	// However, we don't check rr.Priority == 0 && len(xs) > 0 here
+	// It is the responsibility of the user of the library to check this.
+	// This is to encourage the fixing of the source of this error.
+
 	rr.Value = xs
-	if rr.Priority == 0 && len(xs) > 0 {
-		return &ParseError{l.token, "SVCB aliasform can't have values", l}
-	}
 	return nil
 }
 
@@ -200,12 +205,12 @@ func makeSVCBKeyValue(key SVCBKey) SVCBKeyValue {
 	}
 }
 
-// SVCB RR. See RFC xxxx (https://tools.ietf.org/html/draft-ietf-dnsop-svcb-https-01).
+// SVCB RR. See RFC xxxx (https://tools.ietf.org/html/draft-ietf-dnsop-svcb-https-08).
 type SVCB struct {
 	Hdr      RR_Header
-	Priority uint16
+	Priority uint16         // If zero, Value must be empty or discarded by the user of this library
 	Target   string         `dns:"domain-name"`
-	Value    []SVCBKeyValue `dns:"pairs"` // Value must be empty if Priority is zero.
+	Value    []SVCBKeyValue `dns:"pairs"`
 }
 
 // HTTPS RR. Everything valid for SVCB applies to HTTPS as well.
@@ -235,15 +240,29 @@ type SVCBKeyValue interface {
 }
 
 // SVCBMandatory pair adds to required keys that must be interpreted for the RR
-// to be functional.
+// to be functional. If ignored, the whole RRSet must be ignored.
+// "port" and "no-default-alpn" are mandatory by default if present,
+// so they shouldn't be included here.
+//
+// It is incumbent upon the user of this library to reject the RRSet if
+// or avoid constructing such an RRSet that:
+// - "mandatory" is included as one of the keys of mandatory
+// - no key is listed multiple times in mandatory
+// - all keys listed in mandatory are present
+// - escape sequences are not used in mandatory
+// - mandatory, when present, lists at least one key
+//
 // Basic use pattern for creating a mandatory option:
 //
 //	s := &dns.SVCB{Hdr: dns.RR_Header{Name: ".", Rrtype: dns.TypeSVCB, Class: dns.ClassINET}}
 //	e := new(dns.SVCBMandatory)
-//	e.Code = []uint16{65403}
+//	e.Code = []uint16{dns.SVCB_ALPN}
 //	s.Value = append(s.Value, e)
+//	t := new(dns.SVCBAlpn)
+//	t.Alpn = []string{"xmpp-client"}
+//	s.Value = append(s.Value, t)
 type SVCBMandatory struct {
-	Code []SVCBKey // Must not include mandatory
+	Code []SVCBKey
 }
 
 func (*SVCBMandatory) Key() SVCBKey { return SVCB_MANDATORY }
@@ -302,7 +321,8 @@ func (s *SVCBMandatory) copy() SVCBKeyValue {
 }
 
 // SVCBAlpn pair is used to list supported connection protocols.
-// Protocol ids can be found at:
+// The user of this library must ensure that at least one protocol is listed when alpn is present.
+// Protocol IDs can be found at:
 // https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids
 // Basic use pattern for creating an alpn option:
 //
@@ -370,9 +390,13 @@ func (s *SVCBAlpn) copy() SVCBKeyValue {
 }
 
 // SVCBNoDefaultAlpn pair signifies no support for default connection protocols.
+// Should be used in conjunction with alpn.
 // Basic use pattern for creating a no-default-alpn option:
 //
 //	s := &dns.SVCB{Hdr: dns.RR_Header{Name: ".", Rrtype: dns.TypeSVCB, Class: dns.ClassINET}}
+//	t := new(dns.SVCBAlpn)
+//	t.Alpn = []string{"xmpp-client"}
+//	s.Value = append(s.Value, t)
 //	e := new(dns.SVCBNoDefaultAlpn)
 //	s.Value = append(s.Value, e)
 type SVCBNoDefaultAlpn struct{}
@@ -385,14 +409,14 @@ func (*SVCBNoDefaultAlpn) len() int              { return 0 }
 
 func (*SVCBNoDefaultAlpn) unpack(b []byte) error {
 	if len(b) != 0 {
-		return errors.New("dns: svcbnodefaultalpn: no_default_alpn must have no value")
+		return errors.New("dns: svcbnodefaultalpn: no-default-alpn must have no value")
 	}
 	return nil
 }
 
 func (*SVCBNoDefaultAlpn) parse(b string) error {
 	if b != "" {
-		return errors.New("dns: svcbnodefaultalpn: no_default_alpn must have no value")
+		return errors.New("dns: svcbnodefaultalpn: no-default-alpn must have no value")
 	}
 	return nil
 }
@@ -523,7 +547,7 @@ func (s *SVCBIPv4Hint) copy() SVCBKeyValue {
 }
 
 // SVCBECHConfig pair contains the ECHConfig structure defined in draft-ietf-tls-esni [RFC xxxx].
-// Basic use pattern for creating an echconfig option:
+// Basic use pattern for creating an ech option:
 //
 //	h := new(dns.HTTPS)
 //	h.Hdr = dns.RR_Header{Name: ".", Rrtype: dns.TypeHTTPS, Class: dns.ClassINET}
@@ -531,7 +555,7 @@ func (s *SVCBIPv4Hint) copy() SVCBKeyValue {
 //	e.ECH = []byte{0xfe, 0x08, ...}
 //	h.Value = append(h.Value, e)
 type SVCBECHConfig struct {
-	ECH []byte
+	ECH []byte // Specifically ECHConfigList including the redundant length prefix
 }
 
 func (*SVCBECHConfig) Key() SVCBKey     { return SVCB_ECHCONFIG }
@@ -555,7 +579,7 @@ func (s *SVCBECHConfig) unpack(b []byte) error {
 func (s *SVCBECHConfig) parse(b string) error {
 	x, err := fromBase64([]byte(b))
 	if err != nil {
-		return errors.New("dns: svcbechconfig: bad base64 echconfig")
+		return errors.New("dns: svcbech: bad base64 ech")
 	}
 	s.ECH = x
 	return nil
@@ -618,9 +642,6 @@ func (s *SVCBIPv6Hint) String() string {
 }
 
 func (s *SVCBIPv6Hint) parse(b string) error {
-	if strings.Contains(b, ".") {
-		return errors.New("dns: svcbipv6hint: expected ipv6, got ipv4")
-	}
 	str := strings.Split(b, ",")
 	dst := make([]net.IP, len(str))
 	for i, e := range str {
@@ -628,6 +649,9 @@ func (s *SVCBIPv6Hint) parse(b string) error {
 		if ip == nil {
 			return errors.New("dns: svcbipv6hint: bad ip")
 		}
+		if ip.To4() != nil {
+			return errors.New("dns: svcbipv6hint: expected ipv6, got ipv4-mapped-ipv6")
+		}
 		dst[i] = ip
 	}
 	s.Hint = dst


Test patch:
diff --git a/dnssec_test.go b/dnssec_test.go
index 641e319e..1511278c 100644
--- a/dnssec_test.go
+++ b/dnssec_test.go
@@ -855,3 +855,16 @@ func TestParseKeyReadError(t *testing.T) {
 		t.Errorf("expected a nil map, but got %v", m)
 	}
 }
+
+func TestRSAMD5KeyTag(t *testing.T) {
+	rr1, _ := NewRR("test.  IN DNSKEY  257 3 1 AwEAAcntNdoMnY8pvyPcpDTAaiqHyAhf53XUBANq166won/fjBFvmuzhTuP5r4el/pV0tzEBL73zpoU48BqF66uiL+qRijXCySJiaBUvLNll5rpwuduAOoVpmwOmkC4fV6izHOAx/Uy8c+pYP0YR8+1P7GuTFxgnMmt9sUGtoe+la0X/ ;{id = 27461 (ksk), size = 1024b}")
+	rr2, _ := NewRR("test.  IN DNSKEY  257 3 1 AwEAAf0bKO/m45ylk5BlSLmQHQRBLx1m/ZUXvyPFB387bJXxnTk6so3ub97L1RQ+8bOoiRh3Qm5EaYihjco7J8b/W5WbS3tVsE79nY584RfTKT2zcZ9AoFP2XLChXxPIf/6l0H9n6sH0aBjsG8vabEIp8e06INM3CXVPiMRPPeGNa0Ub ;{id = 27461 (ksk), size = 1024b}")
+
+	exp := uint16(27461)
+	if x := rr1.(*DNSKEY).KeyTag(); x != exp {
+		t.Errorf("expected %d, got %d, as keytag for rr1", exp, x)
+	}
+	if x := rr2.(*DNSKEY).KeyTag(); x != exp { // yes, same key tag
+		t.Errorf("expected %d, got %d, as keytag for rr2", exp, x)
+	}
+}
diff --git a/msg_helpers_test.go b/msg_helpers_test.go
index bd41e245..1f17c57c 100644
--- a/msg_helpers_test.go
+++ b/msg_helpers_test.go
@@ -19,7 +19,8 @@ func TestPackDataNsec(t *testing.T) {
 	tests := []struct {
 		name       string
 		args       args
-		want       int
+		wantOff    int
+		wantBytes  []byte
 		wantErr    bool
 		wantErrMsg string
 	}{
@@ -44,14 +45,14 @@ func TestPackDataNsec(t *testing.T) {
 			},
 			wantErr:    true,
 			wantErrMsg: "dns: overflow packing nsec",
-			want:       31,
+			wantOff:    48,
 		},
 		{
 			name: "disordered nsec bits",
 			args: args{
 				bitmap: []uint16{
 					8962,
-					0,
+					1,
 				},
 				msg: []byte{
 					48, 48, 48, 48, 0, 0, 0, 1, 0, 0, 0, 0,
@@ -72,13 +73,13 @@ func TestPackDataNsec(t *testing.T) {
 			},
 			wantErr:    true,
 			wantErrMsg: "dns: nsec bits out of order",
-			want:       155,
+			wantOff:    155,
 		},
 		{
 			name: "simple message with only one window",
 			args: args{
 				bitmap: []uint16{
-					0,
+					1,
 				},
 				msg: []byte{
 					48, 48, 48, 48, 0, 0,
@@ -89,13 +90,33 @@ func TestPackDataNsec(t *testing.T) {
 				},
 				off: 0,
 			},
-			wantErr: false,
-			want:    3,
+			wantErr:   false,
+			wantOff:   3,
+			wantBytes: []byte{0, 1, 64},
+		},
+		{
+			name: "multiple types",
+			args: args{
+				bitmap: []uint16{
+					TypeNS, TypeSOA, TypeRRSIG, TypeDNSKEY, TypeNSEC3PARAM,
+				},
+				msg: []byte{
+					48, 48, 48, 48, 0, 0,
+					0, 1, 0, 0, 0, 0,
+					0, 0, 50, 48, 48, 48,
+					48, 48, 48, 0, 54, 48,
+					48, 48, 48, 0, 19, 48, 48,
+				},
+				off: 0,
+			},
+			wantErr:   false,
+			wantOff:   9,
+			wantBytes: []byte{0, 7, 34, 0, 0, 0, 0, 2, 144},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := packDataNsec(tt.args.bitmap, tt.args.msg, tt.args.off)
+			gotOff, err := packDataNsec(tt.args.bitmap, tt.args.msg, tt.args.off)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("packDataNsec() error = %v, wantErr %v", err, tt.wantErr)
 				return
@@ -104,13 +125,49 @@ func TestPackDataNsec(t *testing.T) {
 				t.Errorf("packDataNsec() error msg = %v, wantErrMsg %v", err.Error(), tt.wantErrMsg)
 				return
 			}
-			if got != tt.want {
-				t.Errorf("packDataNsec() = %v, want %v", got, tt.want)
+			if gotOff != tt.wantOff {
+				t.Errorf("packDataNsec() = %v, want off %v", gotOff, tt.wantOff)
+			}
+			if err == nil && tt.args.off < len(tt.args.msg) && gotOff < len(tt.args.msg) {
+				if want, got := tt.wantBytes, tt.args.msg[tt.args.off:gotOff]; !bytes.Equal(got, want) {
+					t.Errorf("packDataNsec() = %v, want bytes %v", got, want)
+				}
 			}
 		})
 	}
 }
 
+func TestPackDataNsecDirtyBuffer(t *testing.T) {
+	zeroBuf := []byte{0, 0, 0, 0, 0, 0, 0, 0, 0}
+	dirtyBuf := []byte{1, 2, 3, 4, 5, 6, 7, 8, 9}
+	off1, _ := packDataNsec([]uint16{TypeNS, TypeSOA, TypeRRSIG}, zeroBuf, 0)
+	off2, _ := packDataNsec([]uint16{TypeNS, TypeSOA, TypeRRSIG}, dirtyBuf, 0)
+	if off1 != off2 {
+		t.Errorf("off1 %v != off2 %v", off1, off2)
+	}
+	if !bytes.Equal(zeroBuf[:off1], dirtyBuf[:off2]) {
+		t.Errorf("dirty buffer differs from zero buffer: %v, %v", zeroBuf[:off1], dirtyBuf[:off2])
+	}
+}
+
+func BenchmarkPackDataNsec(b *testing.B) {
+	benches := []struct {
+		name  string
+		types []uint16
+	}{
+		{"empty", nil},
+		{"typical", []uint16{TypeNS, TypeSOA, TypeRRSIG, TypeDNSKEY, TypeNSEC3PARAM}},
+		{"multiple_windows", []uint16{1, 300, 350, 10000, 20000}},
+	}
+	for _, bb := range benches {
+		b.Run(bb.name, func(b *testing.B) {
+			buf := make([]byte, 100)
+			for n := 0; n < b.N; n++ {
+				packDataNsec(bb.types, buf, 0)
+			}
+		})
+	}
+}
 func TestUnpackString(t *testing.T) {
 	msg := []byte("\x00abcdef\x0f\\\"ghi\x04mmm\x7f")
 	msg[0] = byte(len(msg) - 1)
diff --git a/parse_test.go b/parse_test.go
index 6c55a74a..444ac1e5 100644
--- a/parse_test.go
+++ b/parse_test.go
@@ -1639,6 +1639,19 @@ func TestParseSVCB(t *testing.T) {
 		`example.com. 3600 IN SVCB 0 cloudflare.com.`: `example.com.	3600	IN	SVCB	0 cloudflare.com.`,
 		`example.com. 3600 IN SVCB 65000 cloudflare.com. alpn=h2 ipv4hint=3.4.3.2`: `example.com.	3600	IN	SVCB	65000 cloudflare.com. alpn="h2" ipv4hint="3.4.3.2"`,
 		`example.com. 3600 IN SVCB 65000 cloudflare.com. key65000=4\ 3 key65001="\" " key65002 key65003= key65004="" key65005== key65006==\"\" key65007=\254 key65008=\032`: `example.com.	3600	IN	SVCB	65000 cloudflare.com. key65000="4\ 3" key65001="\"\ " key65002="" key65003="" key65004="" key65005="=" key65006="=\"\"" key65007="\254" key65008="\ "`,
+		// Explained in svcb.go "In AliasMode, records SHOULD NOT include any SvcParams,"
+		`example.com. 3600 IN SVCB 0 no-default-alpn`: `example.com.	3600	IN	SVCB	0 no-default-alpn.`,
+		// From the specification
+		`example.com.   HTTPS   0 foo.example.com.`: `example.com.	3600	IN	HTTPS	0 foo.example.com.`,
+		`example.com.   SVCB   1 .`: `example.com.	3600	IN	SVCB	1 .`,
+		`example.com.   SVCB   16 foo.example.com. port=53`: `example.com.	3600	IN	SVCB	16 foo.example.com. port="53"`,
+		`example.com.   SVCB   1 foo.example.com. key667=hello`: `example.com.	3600	IN	SVCB	1 foo.example.com. key667="hello"`,
+		`example.com.   SVCB   1 foo.example.com. key667="hello\210qoo"`: `example.com.	3600	IN	SVCB	1 foo.example.com. key667="hello\210qoo"`,
+		`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"`,
 	}
 	for s, o := range svcbs {
 		rr, err := NewRR(s)
@@ -1655,7 +1668,6 @@ func TestParseSVCB(t *testing.T) {
 func TestParseBadSVCB(t *testing.T) {
 	header := `example.com. 3600 IN HTTPS `
 	evils := []string{
-		`0 . no-default-alpn`,     // aliasform
 		`65536 . no-default-alpn`, // bad priority
 		`1 ..`,                    // bad domain
 		`1 . no-default-alpn=1`,   // value illegal
@@ -1682,10 +1694,12 @@ func TestParseBadSVCB(t *testing.T) {
 		`1 . ipv6hint=1.1.1.1`,    // not ipv6
 		`1 . ipv6hint=1:1:1:1`,    // not ipv6
 		`1 . ipv6hint=a`,          // not ipv6
+		`1 . ipv6hint=`,           // empty ipv6
 		`1 . ipv4hint=1.1.1.1.1`,  // not ipv4
 		`1 . ipv4hint=::fc`,       // not ipv4
 		`1 . ipv4hint=..11`,       // not ipv4
 		`1 . ipv4hint=a`,          // not ipv4
+		`1 . ipv4hint=`,           // empty ipv4
 		`1 . port=`,               // empty port
 		`1 . echconfig=YUd`,       // bad base64
 	}
diff --git a/svcb_test.go b/svcb_test.go
index 6994aca5..25494995 100644
--- a/svcb_test.go
+++ b/svcb_test.go
@@ -17,7 +17,7 @@ func TestSVCB(t *testing.T) {
 		{`ipv4hint`, `3.4.3.2,1.1.1.1`},
 		{`no-default-alpn`, ``},
 		{`ipv6hint`, `1::4:4:4:4,1::3:3:3:3`},
-		{`echconfig`, `YUdWc2JHOD0=`},
+		{`ech`, `YUdWc2JHOD0=`},
 		{`key65000`, `4\ 3`},
 		{`key65001`, `\"\ `},
 		{`key65002`, ``},