dns/dnsmessage: correctly validate SVCB record parameter order

Fix a bug which caused us to not properly validate the order
of parameters in SVCB/HTTPS records.

Fixes golang/go#79796

Change-Id: I9d3c43bde06f3d25786a5b89e45613e76a6a6964
Reviewed-on: https://go-review.googlesource.com/c/net/+/786346
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
Reviewed-by: Nicholas Husin <husin@google.com>

dns/dnsmessage/message_test.go

diff --git a/dns/dnsmessage/message_test.go b/dns/dnsmessage/message_test.go
index e5997dc..9e46c63 100644
--- a/dns/dnsmessage/message_test.go
+++ b/dns/dnsmessage/message_test.go
@@ -1940,6 +1940,30 @@
 				0x00, 0x01, 0x00, 0x00, 0x01, 0x00, 0x5d,
 			},
 		},
+		{
+			name: "SVCB out-of-order keys",
+			in: []byte{
+				0x12, 0x34, 0x81, 0x80, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00,
+				0x00, 0x00, 0x41, 0x00, 0x01,
+				0x00, 0x00, 0x41, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00,
+				0x00, 0x0b,
+				0x00, 0x01, 0x00,
+				0x00, 0x02, 0x00, 0x00,
+				0x00, 0x01, 0x00, 0x00,
+			},
+		},
+		{
+			name: "SVCB duplicate keys",
+			in: []byte{
+				0x12, 0x34, 0x81, 0x80, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00,
+				0x00, 0x00, 0x41, 0x00, 0x01,
+				0x00, 0x00, 0x41, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00,
+				0x00, 0x0b,
+				0x00, 0x01, 0x00,
+				0x00, 0x01, 0x00, 0x00,
+				0x00, 0x01, 0x00, 0x00,
+			},
+		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			var p Parser

dns/dnsmessage/svcb.go

diff --git a/dns/dnsmessage/svcb.go b/dns/dnsmessage/svcb.go
index 252401b..7729378 100644
--- a/dns/dnsmessage/svcb.go
+++ b/dns/dnsmessage/svcb.go
@@ -168,9 +168,8 @@
 	if err != nil {
 		return oldMsg, &nestedError{"SVCBResource.Target", err}
 	}
-	var previousKey SVCParamKey
 	for i, param := range r.Params {
-		if i > 0 && param.Key <= previousKey {
+		if i > 0 && param.Key <= r.Params[i-1].Key {
 			return oldMsg, &nestedError{"SVCBResource.Params", errParamOutOfOrder}
 		}
 		if len(param.Value) > (1<<16)-1 {
@@ -220,6 +219,7 @@
 		if off+int(size) > bodyEnd {
 			return SVCBResource{}, errResourceLen
 		}
+		previousKey = key
 		totalValueLen += size
 		off += int(size)
 		n++

dns/dnsmessage/svcb_test.go

diff --git a/dns/dnsmessage/svcb_test.go b/dns/dnsmessage/svcb_test.go
index 74fcccd..64a1bfd 100644
--- a/dns/dnsmessage/svcb_test.go
+++ b/dns/dnsmessage/svcb_test.go
@@ -366,28 +366,74 @@
 	testRecord(bytes, parsed)
 }
 
-func TestSVCBPackLongValue(t *testing.T) {
-	b := NewBuilder(nil, Header{})
-	b.StartQuestions()
-	b.StartAnswers()
-
-	res := SVCBResource{
-		Target: MustNewName("example.com."),
-		Params: []SVCParam{
-			{
-				Key:   SVCParamMandatory,
-				Value: make([]byte, math.MaxUint16+1),
+func TestSVCBPackErrors(t *testing.T) {
+	for _, test := range []struct {
+		name string
+		r    SVCBResource
+	}{
+		{
+			name: "long value",
+			r: SVCBResource{
+				Target: MustNewName("example.com."),
+				Params: []SVCParam{
+					{
+						Key:   SVCParamMandatory,
+						Value: make([]byte, math.MaxUint16+1),
+					},
+				},
 			},
 		},
-	}
+		{
+			name: "out-of-order keys",
+			r: SVCBResource{
+				Target: MustNewName("example.com."),
+				Params: []SVCParam{
+					{
+						Key:   SVCParamPort, // 3
+						Value: []byte("443"),
+					},
+					{
+						Key:   SVCParamALPN, // 1
+						Value: []byte("h2"),
+					},
+				},
+			},
+		},
+		{
+			name: "duplicate keys",
+			r: SVCBResource{
+				Target: MustNewName("example.com."),
+				Params: []SVCParam{
+					{
+						Key:   SVCParamALPN,
+						Value: []byte("h3"),
+					},
+					{
+						Key:   SVCParamALPN,
+						Value: []byte("h2"),
+					},
+				},
+			},
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			b := NewBuilder(nil, Header{})
+			b.StartQuestions()
+			b.StartAnswers()
+			if err := b.SVCBResource(ResourceHeader{
+				Name: MustNewName("example.com."),
+			}, test.r); err == nil {
+				t.Errorf("b.SVCBResource() succeeded; want error")
+			}
 
-	err := b.SVCBResource(ResourceHeader{Name: MustNewName("example.com.")}, res)
-	if err == nil || err.Error() != "ResourceBody: SVCBResource.Params: value too long (>65535 bytes)" {
-		t.Fatalf(`b.SVCBResource() = %v; want = "ResourceBody: SVCBResource.Params: value too long (>65535 bytes)"`, err)
-	}
-
-	err = b.HTTPSResource(ResourceHeader{Name: MustNewName("example.com.")}, HTTPSResource{res})
-	if err == nil || err.Error() != "ResourceBody: SVCBResource.Params: value too long (>65535 bytes)" {
-		t.Fatalf(`b.HTTPSResource() = %v; want = "ResourceBody: SVCBResource.Params: value too long (>65535 bytes)"`, err)
+			b = NewBuilder(nil, Header{})
+			b.StartQuestions()
+			b.StartAnswers()
+			if err := b.HTTPSResource(ResourceHeader{
+				Name: MustNewName("example.com."),
+			}, HTTPSResource{test.r}); err == nil {
+				t.Errorf("b.HTTPSResource() succeeded; want error")
+			}
+		})
 	}
 }