Do not sign BADKEY and BADSIG TSIG error responses (#1316)

* Per RFC 8945 5.3.2, responses with BADKEY and BADSIG errors must not be signed. Signed-off-by: Chris O'Haver <[email protected]> * refactor to remove else block Signed-off-by: Chris O'Haver <[email protected]> * skip signing only for BADKEY and BADSIG Signed-off-by: Chris O'Haver <[email protected]>
miekg · Dec 20, 2021 · 3a58872 · 3a58872
1 parent 3b8982c
commit 3a58872
Show file tree

Hide file tree

Showing 2 changed files with 71 additions and 6 deletions.
diff --git a/tsig.go b/tsig.go
@@ -162,20 +162,29 @@ func tsigGenerateProvider(m *Msg, provider TsigProvider, requestMAC string, time
 	if err != nil {
 		return nil, "", err
 	}
+
 	buf, err := tsigBuffer(mbuf, rr, requestMAC, timersOnly)
 	if err != nil {
 		return nil, "", err
 	}
 
 	t := new(TSIG)
-	// Copy all TSIG fields except MAC and its size, which are filled using the computed digest.
+	// Copy all TSIG fields except MAC, its size, and time signed which are filled when signing.
 	*t = *rr
-	mac, err := provider.Generate(buf, rr)
-	if err != nil {
-		return nil, "", err
+	t.TimeSigned = 0
+	t.MAC = ""
+	t.MACSize = 0
+
+	// Sign unless there is a key or MAC validation error (RFC 8945 5.3.2)
+	if rr.Error != RcodeBadKey && rr.Error != RcodeBadSig {
+		mac, err := provider.Generate(buf, rr)
+		if err != nil {
+			return nil, "", err
+		}
+		t.TimeSigned = rr.TimeSigned
+		t.MAC = hex.EncodeToString(mac)
+		t.MACSize = uint16(len(t.MAC) / 2) // Size is half!
 	}
-	t.MAC = hex.EncodeToString(mac)
-	t.MACSize = uint16(len(t.MAC) / 2) // Size is half!
 
 	tbuf := make([]byte, Len(t))
 	off, err := PackRR(t, tbuf, 0, nil, false)

diff --git a/tsig_test.go b/tsig_test.go
@@ -55,6 +55,62 @@ func TestTsigCase(t *testing.T) {
 	}
 }
 
+func TestTsigErrorResponse(t *testing.T) {
+	for _, rcode := range []uint16{RcodeBadSig, RcodeBadKey} {
+		m := newTsig(strings.ToUpper(HmacSHA256))
+		m.IsTsig().Error = rcode
+		buf, _, err := TsigGenerate(m, "pRZgBrBvI4NAHZYhxmhs/Q==", "", false)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		err = m.Unpack(buf)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		mTsig := m.IsTsig()
+		if mTsig.MAC != "" {
+			t.Error("Expected empty MAC")
+		}
+		if mTsig.MACSize != 0 {
+			t.Error("Expected 0 MACSize")
+		}
+		if mTsig.TimeSigned != 0 {
+			t.Errorf("Expected TimeSigned to be 0, got %v", mTsig.TimeSigned)
+		}
+	}
+}
+
+func TestTsigBadTimeResponse(t *testing.T) {
+	clientTime := uint64(time.Now().Unix()) - 3600
+	m := newTsig(strings.ToUpper(HmacSHA256))
+	m.IsTsig().Error = RcodeBadTime
+	m.IsTsig().TimeSigned = clientTime
+
+	buf, _, err := TsigGenerate(m, "pRZgBrBvI4NAHZYhxmhs/Q==", "", false)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = m.Unpack(buf)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	mTsig := m.IsTsig()
+	if mTsig.MAC == "" {
+		t.Error("Expected non-empty MAC")
+	}
+	if int(mTsig.MACSize) != len(mTsig.MAC)/2 {
+		t.Errorf("Expected MACSize %v, got %v", len(mTsig.MAC)/2, mTsig.MACSize)
+	}
+
+	if mTsig.TimeSigned != clientTime {
+		t.Errorf("Expected TimeSigned %v to be retained, got %v", clientTime, mTsig.TimeSigned)
+	}
+}
+
 const (
 	// A template wire-format DNS message (in hex form) containing a TSIG RR.
 	// Its time signed field will be filled by tests.