Repository: miekg/dns
Pull request #1136 (2020-07-20 17:41:35)

catch error from tsigBuffer, mainly to detect other data overflow
Currently, `tsigBuffer` ignores any error from `packXXX` functions, so `TsigGenerate` or `TsigVerify` can't notice that condition.  but since the buffer used for those `packXXX` only has `DefaultMsgSize` (4096) bytes, and "other data" can be up to 64KB, an arbitrary sender of TSIG message can cause this error in `TsigVerify`.  In practice it would just result in failing to verify the signature, but I think it's cleaner and safer to catch this kind (i.e., "overflow") of error conditions explicitly.

This PR does just that (also catching other minor errors).


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

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

```diff
@@            Coverage Diff             @@
##           master    #1136      +/-   ##
==========================================
+ Coverage   55.44%   55.52%   +0.08%     
==========================================
  Files          41       41              
  Lines       10110    10120      +10     
==========================================
+ Hits         5605     5619      +14     
+ Misses       3481     3478       -3     
+ Partials     1024     1023       -1     
```


| [Impacted Files](https://codecov.io/gh/miekg/dns/pull/1136?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [tsig.go](https://codecov.io/gh/miekg/dns/pull/1136/diff?src=pr&el=tree#diff-dHNpZy5nbw==) | `55.35% <58.82%> (+0.68%)` | :arrow_up: |
| [server.go](https://codecov.io/gh/miekg/dns/pull/1136/diff?src=pr&el=tree#diff-c2VydmVyLmdv) | `70.71% <0.00%> (-0.28%)` | :arrow_down: |
| [zmsg.go](https://codecov.io/gh/miekg/dns/pull/1136/diff?src=pr&el=tree#diff-em1zZy5nbw==) | `40.03% <0.00%> (+0.10%)` | :arrow_up: |
| [msg\_helpers.go](https://codecov.io/gh/miekg/dns/pull/1136/diff?src=pr&el=tree#diff-bXNnX2hlbHBlcnMuZ28=) | `68.01% <0.00%> (+0.40%)` | :arrow_up: |
| [msg.go](https://codecov.io/gh/miekg/dns/pull/1136/diff?src=pr&el=tree#diff-bXNnLmdv) | `77.21% <0.00%> (+0.68%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/miekg/dns/pull/1136?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/1136?src=pr&el=footer). Last update [9093928...b29954e](https://codecov.io/gh/miekg/dns/pull/1136?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/tsig.go b/tsig.go
index 0b010091..1c9e9024 100644
--- a/tsig.go
+++ b/tsig.go
@@ -111,7 +111,10 @@ func TsigGenerate(m *Msg, secret, requestMAC string, timersOnly bool) ([]byte, s
 	if err != nil {
 		return nil, "", err
 	}
-	buf := tsigBuffer(mbuf, rr, requestMAC, timersOnly)
+	buf, err := tsigBuffer(mbuf, rr, requestMAC, timersOnly)
+	if err != nil {
+		return nil, "", err
+	}
 
 	t := new(TSIG)
 	var h hash.Hash
@@ -173,7 +176,10 @@ func tsigVerify(msg []byte, secret, requestMAC string, timersOnly bool, now uint
 		return err
 	}
 
-	buf := tsigBuffer(stripped, tsig, requestMAC, timersOnly)
+	buf, err := tsigBuffer(stripped, tsig, requestMAC, timersOnly)
+	if err != nil {
+		return err
+	}
 
 	var h hash.Hash
 	switch CanonicalName(tsig.Algorithm) {
@@ -209,7 +215,7 @@ func tsigVerify(msg []byte, secret, requestMAC string, timersOnly bool, now uint
 }
 
 // Create a wiredata buffer for the MAC calculation.
-func tsigBuffer(msgbuf []byte, rr *TSIG, requestMAC string, timersOnly bool) []byte {
+func tsigBuffer(msgbuf []byte, rr *TSIG, requestMAC string, timersOnly bool) ([]byte, error) {
 	var buf []byte
 	if rr.TimeSigned == 0 {
 		rr.TimeSigned = uint64(time.Now().Unix())
@@ -226,7 +232,10 @@ func tsigBuffer(msgbuf []byte, rr *TSIG, requestMAC string, timersOnly bool) []b
 		m.MACSize = uint16(len(requestMAC) / 2)
 		m.MAC = requestMAC
 		buf = make([]byte, len(requestMAC)) // long enough
-		n, _ := packMacWire(m, buf)
+		n, err := packMacWire(m, buf)
+		if err != nil {
+			return nil, err
+		}
 		buf = buf[:n]
 	}
 
@@ -235,7 +244,10 @@ func tsigBuffer(msgbuf []byte, rr *TSIG, requestMAC string, timersOnly bool) []b
 		tsig := new(timerWireFmt)
 		tsig.TimeSigned = rr.TimeSigned
 		tsig.Fudge = rr.Fudge
-		n, _ := packTimerWire(tsig, tsigvar)
+		n, err := packTimerWire(tsig, tsigvar)
+		if err != nil {
+			return nil, err
+		}
 		tsigvar = tsigvar[:n]
 	} else {
 		tsig := new(tsigWireFmt)
@@ -248,7 +260,10 @@ func tsigBuffer(msgbuf []byte, rr *TSIG, requestMAC string, timersOnly bool) []b
 		tsig.Error = rr.Error
 		tsig.OtherLen = rr.OtherLen
 		tsig.OtherData = rr.OtherData
-		n, _ := packTsigWire(tsig, tsigvar)
+		n, err := packTsigWire(tsig, tsigvar)
+		if err != nil {
+			return nil, err
+		}
 		tsigvar = tsigvar[:n]
 	}
 
@@ -258,7 +273,7 @@ func tsigBuffer(msgbuf []byte, rr *TSIG, requestMAC string, timersOnly bool) []b
 	} else {
 		buf = append(msgbuf, tsigvar...)
 	}
-	return buf
+	return buf, nil
 }
 
 // Strip the TSIG from the raw message.


Test patch:
diff --git a/tsig_test.go b/tsig_test.go
index 8e4cc91b..65d4b363 100644
--- a/tsig_test.go
+++ b/tsig_test.go
@@ -4,6 +4,7 @@ import (
 	"encoding/binary"
 	"encoding/hex"
 	"fmt"
+	"strings"
 	"testing"
 	"time"
 )
@@ -99,11 +100,28 @@ func TestTsigErrors(t *testing.T) {
 	}
 
 	// call TsigVerify with a message that doesn't contain a TSIG
-	msgData, _, err := stripTsig(buildMsgData(timeSigned))
+	msgData, tsig, 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)
 	}
+
+	// replace the test TSIG with a bogus one with large "other data", which would cause overflow in TsigVerify.
+	// The overflow should be caught without disruption.
+	tsig.OtherData = strings.Repeat("00", 4096)
+	tsig.OtherLen = uint16(len(tsig.OtherData) / 2)
+	msg := new(Msg)
+	if err = msg.Unpack(msgData); err != nil {
+		t.Fatal(err)
+	}
+	msg.Extra = append(msg.Extra, tsig)
+	if msgData, err = msg.Pack(); err != nil {
+		t.Fatal(err)
+	}
+	err = tsigVerify(msgData, testSecret, "", false, timeSigned)
+	if err == nil || !strings.Contains(err.Error(), "overflow") {
+		t.Errorf("expected error to contain %q, but got %v", "overflow", err)
+	}
 }