diff --git a/tsig.go b/tsig.go index 9451f1a8..0b010091 100644 --- a/tsig.go +++ b/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 } diff --git a/tsig_test.go b/tsig_test.go index 3f1f3c8c..8e4cc91b 100644 --- a/tsig_test.go +++ b/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) + } +}