make TsigVerify check time after signature per rfc2845bis

tsig.go

@@ -153,6 +153,11 @@ func TsigGenerate(m *Msg, secret, requestMAC string, timersOnly bool) ([]byte, s
 // If the signature does not validate err contains the
 // error, otherwise it is nil.
 func TsigVerify(msg []byte, secret, requestMAC string, timersOnly bool) error {
+	return tsigVerify(msg, secret, requestMAC, timersOnly, uint64(time.Now().Unix()))
+}
+
+// actual implementation of TsigVerify, taking the current time ('now') as a parameter for the convenience of tests.
+func tsigVerify(msg []byte, secret, requestMAC string, timersOnly bool, now uint64) error {
 	rawsecret, err := fromBase64([]byte(secret))
 	if err != nil {
 		return err
@@ -170,17 +175,6 @@ func TsigVerify(msg []byte, secret, requestMAC string, timersOnly bool) error {
 
 	buf := tsigBuffer(stripped, tsig, requestMAC, timersOnly)
 
-	// Fudge factor works both ways. A message can arrive before it was signed because
-	// of clock skew.
-	now := uint64(time.Now().Unix())
-	ti := now - tsig.TimeSigned
-	if now < tsig.TimeSigned {
-		ti = tsig.TimeSigned - now
-	}
-	if uint64(tsig.Fudge) < ti {
-		return ErrTime
-	}
-
 	var h hash.Hash
 	switch CanonicalName(tsig.Algorithm) {
 	case HmacMD5:
@@ -198,6 +192,19 @@ func TsigVerify(msg []byte, secret, requestMAC string, timersOnly bool) error {
 	if !hmac.Equal(h.Sum(nil), msgMAC) {
 		return ErrSig
 	}
+
+	// Fudge factor works both ways. A message can arrive before it was signed because
+	// of clock skew.
+	// We check this after verifying the signature, following draft-ietf-dnsop-rfc2845bis
+	// instead of RFC2845, in order to prevent a security vulnerability as reported in CVE-2017-3142/3143.
+	ti := now - tsig.TimeSigned
+	if now < tsig.TimeSigned {
+		ti = tsig.TimeSigned - now
+	}
+	if uint64(tsig.Fudge) < ti {
+		return ErrTime
+	}
+
 	return nil
 }
 

tsig_test.go

@@ -2,6 +2,8 @@ package dns
 
 import (
 	"encoding/binary"
+	"encoding/hex"
+	"fmt"
 	"testing"
 	"time"
 )
@@ -50,3 +52,58 @@ func TestTsigCase(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+const (
+	// A template wire-format DNS message (in hex form) containing a TSIG RR.
+	// Its time signed field will be filled by tests.
+	wireMsg = "c60028000001000000010001076578616d706c6503636f6d00000600010161c00c0001000100000e100004c0000201077465" +
+		"73746b65790000fa00ff00000000003d0b686d61632d73686132353600" +
+		"%012x" + // placeholder for the "time signed" field
+		"012c00208cf23e0081d915478a182edcea7ff48ad102948e6c7ef8e887536957d1fa5616c60000000000"
+	// A secret (in base64 format) with which the TSIG in wireMsg will be validated
+	testSecret        = "NoTCJU+DMqFWywaPyxSijrDEA/eC3nK0xi3AMEZuPVk="
+	// the 'time signed' field value that would make the TSIG RR valid with testSecret
+	timeSigned uint64 = 1594855491
+)
+
+func TestTsigErrors(t *testing.T) {
+	// Helper shortcut to build wire-format test message.
+	// TsigVerify can modify the slice, so we need to create a new one for each test case below.
+	buildMsgData := func(tm uint64) []byte {
+		msgData, err := hex.DecodeString(fmt.Sprintf(wireMsg, tm))
+		if err != nil {
+			t.Fatal(err)
+		}
+		return msgData
+	}
+
+	// the signature is valid but 'time signed' is too far from the "current time".
+	if err := tsigVerify(buildMsgData(timeSigned), testSecret, "", false, timeSigned+301); err != ErrTime {
+		t.Fatalf("expected an error '%v' but got '%v'", ErrTime, err)
+	}
+	if err := tsigVerify(buildMsgData(timeSigned), testSecret, "", false, timeSigned-301); err != ErrTime {
+		t.Fatalf("expected an error '%v' but got '%v'", ErrTime, err)
+	}
+
+	// the signature is invalid and 'time signed' is too far.
+	// the signature should be checked first, so we should see ErrSig.
+	if err := tsigVerify(buildMsgData(timeSigned+301), testSecret, "", false, timeSigned); err != ErrSig {
+		t.Fatalf("expected an error '%v' but got '%v'", ErrSig, err)
+	}
+
+	// tweak the algorithm name in the wire data, resulting in the "unknown algorithm" error.
+	msgData := buildMsgData(timeSigned)
+	copy(msgData[67:], "bogus")
+	if err := tsigVerify(msgData, testSecret, "", false, timeSigned); err != ErrKeyAlg {
+		t.Fatalf("expected an error '%v' but got '%v'", ErrKeyAlg, err)
+	}
+
+	// call TsigVerify with a message that doesn't contain a TSIG
+	msgData, _, err := stripTsig(buildMsgData(timeSigned))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := tsigVerify(msgData, testSecret, "", false, timeSigned); err != ErrNoSig {
+		t.Fatalf("expected an error '%v' but got '%v'", ErrNoSig, err)
+	}
+}