Repository: miekg/dns
Pull request #1135 (2020-07-16 22:50:17)

make TsigVerify check time after signature per rfc2845bis
The current implementation of `TsigVerify` is compliant to RFC2845 in that it first checks 'time signed' and then verifies the signature.  But this logic is known to have a security vulnerability as reported in CVE-2017-3142/3143.

The protocol is being revised as draft-ietf-dnsop-rfc2845bis, reversing the order of these checks.  This PR implements the new behavior in a straightforward way.  While the new protocol spec is still an internet draft, it's already in the RFC editor queue, so I believe it's mature enough.

(Added test covers this specific case with some other error conditions from `TsigVerify`)

codecov-commenter: # [Codecov](https://codecov.io/gh/miekg/dns/pull/1135?src=pr&el=h1) Report
> Merging [#1135](https://codecov.io/gh/miekg/dns/pull/1135?src=pr&el=desc) into [master](https://codecov.io/gh/miekg/dns/commit/50b4756e47a1e69825c82175d5c1fafc9109cda3&el=desc) will **increase** coverage by `0.10%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/miekg/dns/pull/1135/graphs/tree.svg?width=650&height=150&src=pr&token=uT9fYPSeKT)](https://codecov.io/gh/miekg/dns/pull/1135?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1135      +/-   ##
==========================================
+ Coverage   55.32%   55.43%   +0.10%     
==========================================
  Files          41       41              
  Lines       10109    10110       +1     
==========================================
+ Hits         5593     5604      +11     
+ Misses       3488     3482       -6     
+ Partials     1028     1024       -4     
```


| [Impacted Files](https://codecov.io/gh/miekg/dns/pull/1135?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [tsig.go](https://codecov.io/gh/miekg/dns/pull/1135/diff?src=pr&el=tree#diff-dHNpZy5nbw==) | `54.67% <100.00%> (+6.78%)` | :arrow_up: |
| [msg.go](https://codecov.io/gh/miekg/dns/pull/1135/diff?src=pr&el=tree#diff-bXNnLmdv) | `76.53% <0.00%> (-0.69%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/miekg/dns/pull/1135?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/miekg/dns/pull/1135?src=pr&el=footer). Last update [50b4756...3d75d77](https://codecov.io/gh/miekg/dns/pull/1135?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

miekg: /merge

On Sat, 18 Jul 2020, 06:13 Tom Thorogood, <notifications@github.com> wrote:

> *@tmthrgd* approved this pull request.
>
> LGTM.
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <https://github.com/miekg/dns/pull/1135#pullrequestreview-451029611>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AACWIWZZNVD4ERRPH73U64LR4EONHANCNFSM4O5COLAA>
> .
>


Patch:
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
 }
 


Test patch:
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)
+	}
+}