Merge pull request #215 from aaronlehmann/dnssec-invalid-rrset

Check that the RRs passed to Sign and Verify form a valid RFC2181 RRSet

defaults.go

@@ -188,6 +188,33 @@ func IsFqdn(s string) bool {
 	return s[l-1] == '.'
 }
 
+// IsRRset checks if a set of RRs is a valid RRset as defined by RFC 2181.
+// This means the RRs need to have the same type, name, and class. Returns true
+// if the RR set is valid, otherwise false.
+func IsRRset(rrset []RR) bool {
+	if len(rrset) == 0 {
+		return false
+	}
+	if len(rrset) == 1 {
+		return true
+	}
+	rrHeader := rrset[0].Header()
+	rrType := rrHeader.Rrtype
+	rrClass := rrHeader.Class
+	rrName := rrHeader.Name
+
+	for _, rr := range rrset[1:] {
+		curRRHeader := rr.Header()
+		if curRRHeader.Rrtype != rrType || curRRHeader.Class != rrClass || curRRHeader.Name != rrName {
+			// Mismatch between the records, so this is not a valid rrset for
+			//signing/verifying
+			return false
+		}
+	}
+
+	return true
+}
+
 // Fqdn return the fully qualified domain name from s.
 // If s is already fully qualified, it behaves as the identity function.
 func Fqdn(s string) string {

dnssec.go

@@ -296,7 +296,7 @@ func (rr *RRSIG) Sign(k PrivateKey, rrset []RR) error {
 // This function copies the rdata of some RRs (to lowercase domain names) for the validation to work.
 func (rr *RRSIG) Verify(k *DNSKEY, rrset []RR) error {
 	// First the easy checks
-	if len(rrset) == 0 {
+	if !IsRRset(rrset) {
 		return ErrRRset
 	}
 	if rr.KeyTag != k.KeyTag() {
@@ -314,14 +314,17 @@ func (rr *RRSIG) Verify(k *DNSKEY, rrset []RR) error {
 	if k.Protocol != 3 {
 		return ErrKey
 	}
-	for _, r := range rrset {
-		if r.Header().Class != rr.Hdr.Class {
-			return ErrRRset
-		}
-		if r.Header().Rrtype != rr.TypeCovered {
-			return ErrRRset
-		}
+
+	// IsRRset checked that we have at least one RR and that the RRs in
+	// the set have consistent type, class, and name. Also check that type and
+	// class matches the RRSIG record.
+	if rrset[0].Header().Class != rr.Hdr.Class {
+		return ErrRRset
 	}
+	if rrset[0].Header().Rrtype != rr.TypeCovered {
+		return ErrRRset
+	}
+
 	// RFC 4035 5.3.2.  Reconstructing the Signed Data
 	// Copy the sig, except the rrsig data
 	sigwire := new(rrsigWireFmt)
@@ -409,7 +412,8 @@ func (rr *RRSIG) Verify(k *DNSKEY, rrset []RR) error {
 
 // ValidityPeriod uses RFC1982 serial arithmetic to calculate
 // if a signature period is valid. If t is the zero time, the
-// current time is taken other t is.
+// current time is taken other t is. Returns true if the signature
+// is valid at the given time, otherwise returns false.
 func (rr *RRSIG) ValidityPeriod(t time.Time) bool {
 	var utc int64
 	if t.IsZero() {

dnssec_test.go

@@ -656,3 +656,64 @@ PrivateKey: WURgWHCcYIYUPWgeLmiPY2DJJk02vgrmTfitxgqcL4vwW7BOrbawVmVe0d9V94SR`
 		t.Fatalf("RRSIG record differs:\n%v\n%v", ourRRSIG, rrRRSIG.(*RRSIG))
 	}
 }
+
+func TestInvalidRRSet(t *testing.T) {
+	goodRecords := make([]RR, 2)
+	goodRecords[0] = &TXT{Hdr: RR_Header{Name: "name.cloudflare.com.", Rrtype: TypeTXT, Class: ClassINET, Ttl: 0}, Txt: []string{"Hello world"}}
+	goodRecords[1] = &TXT{Hdr: RR_Header{Name: "name.cloudflare.com.", Rrtype: TypeTXT, Class: ClassINET, Ttl: 0}, Txt: []string{"_o/"}}
+
+	// Generate key
+	keyname := "cloudflare.com."
+	key := &DNSKEY{
+		Hdr:       RR_Header{Name: keyname, Rrtype: TypeDNSKEY, Class: ClassINET, Ttl: 0},
+		Algorithm: ECDSAP256SHA256,
+		Flags:     ZONE,
+		Protocol:  3,
+	}
+	privatekey, err := key.Generate(256)
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+
+	// Need to fill in: Inception, Expiration, KeyTag, SignerName and Algorithm
+	curTime := time.Now()
+	signature := &RRSIG{
+		Inception:  uint32(curTime.Unix()),
+		Expiration: uint32(curTime.Add(time.Hour).Unix()),
+		KeyTag:     key.KeyTag(),
+		SignerName: keyname,
+		Algorithm:  ECDSAP256SHA256,
+	}
+
+	// Inconsistent name between records
+	badRecords := make([]RR, 2)
+	badRecords[0] = &TXT{Hdr: RR_Header{Name: "name.cloudflare.com.", Rrtype: TypeTXT, Class: ClassINET, Ttl: 0}, Txt: []string{"Hello world"}}
+	badRecords[1] = &TXT{Hdr: RR_Header{Name: "nama.cloudflare.com.", Rrtype: TypeTXT, Class: ClassINET, Ttl: 0}, Txt: []string{"_o/"}}
+
+	if IsRRset(badRecords) {
+		t.Fatal("Record set with inconsistent names considered valid")
+	}
+
+	badRecords[0] = &TXT{Hdr: RR_Header{Name: "name.cloudflare.com.", Rrtype: TypeTXT, Class: ClassINET, Ttl: 0}, Txt: []string{"Hello world"}}
+	badRecords[1] = &A{Hdr: RR_Header{Name: "name.cloudflare.com.", Rrtype: TypeA, Class: ClassINET, Ttl: 0}}
+
+	if IsRRset(badRecords) {
+		t.Fatal("Record set with inconsistent record types considered valid")
+	}
+
+	badRecords[0] = &TXT{Hdr: RR_Header{Name: "name.cloudflare.com.", Rrtype: TypeTXT, Class: ClassINET, Ttl: 0}, Txt: []string{"Hello world"}}
+	badRecords[1] = &TXT{Hdr: RR_Header{Name: "name.cloudflare.com.", Rrtype: TypeTXT, Class: ClassCHAOS, Ttl: 0}, Txt: []string{"_o/"}}
+
+	if IsRRset(badRecords) {
+		t.Fatal("Record set with inconsistent record class considered valid")
+	}
+
+	// Sign the good record set and then make sure verification fails on the bad record set
+	if err := signature.Sign(privatekey, goodRecords); err != nil {
+		t.Fatal("Signing good records failed")
+	}
+
+	if err := signature.Verify(key, badRecords); err != ErrRRset {
+		t.Fatal("Verification did not return ErrRRset with inconsistent records")
+	}
+}