nebius/SWE-rebench-V2-PRs huggingface dataset:
Repository: miekg/dns
Pull request #1649 (2025-04-23 15:01:08)

Fix logic in xfr ReadMsg + add test
Hi! 

This fixes a possible bug where a xfr client can skip tsig validation if the server doesn't respond with a tsig. 

Say I setup an xfr client with a tsig provider. If it connects to a server which doesn't implement tsig, but that server can perform a transfer without adding the tsig to the response then the current logic will skip validation. 

`m.IsTsig() && t.tsigProvider()` is going to look for the tsig in the message, see none and consider the transfer valid, but I think the client should be checking the signature since the tsig provider is configured on the client.

miekg: your rational makes sense

Patch:
diff --git a/xfr.go b/xfr.go
index 5cfbb516..97a64247 100644
--- a/xfr.go
+++ b/xfr.go
@@ -251,10 +251,13 @@ func (t *Transfer) ReadMsg() (*Msg, error) {
 	if err := m.Unpack(p); err != nil {
 		return nil, err
 	}
-	if ts, tp := m.IsTsig(), t.tsigProvider(); ts != nil && tp != nil {
+
+	if tp := t.tsigProvider(); tp != nil {
 		// Need to work on the original message p, as that was used to calculate the tsig.
 		err = TsigVerifyWithProvider(p, tp, t.tsigRequestMAC, t.tsigTimersOnly)
-		t.tsigRequestMAC = ts.MAC
+		if ts := m.IsTsig(); ts != nil {
+			t.tsigRequestMAC = ts.MAC
+		}
 	}
 	return m, err
 }


Test patch:
diff --git a/xfr_test.go b/xfr_test.go
index 04801a2e..c580ceef 100644
--- a/xfr_test.go
+++ b/xfr_test.go
@@ -2,6 +2,7 @@ package dns
 
 import (
 	"crypto/tls"
+	"errors"
 	"testing"
 	"time"
 )
@@ -221,6 +222,29 @@ func axfrTestingSuiteWithCustomTsig(t *testing.T, addrstr string, provider TsigP
 	}
 }
 
+func axfrTestingSuiteWithMsgNotSigned(t *testing.T, addrstr string, provider TsigProvider) {
+	tr := new(Transfer)
+	m := new(Msg)
+	var err error
+	tr.Conn, err = Dial("tcp", addrstr)
+	if err != nil {
+		t.Fatal("failed to dial", err)
+	}
+	tr.TsigProvider = provider
+	m.SetAxfr("miek.nl.")
+
+	c, err := tr.In(m, addrstr)
+	if err != nil {
+		t.Fatal("failed to zone transfer in", err)
+	}
+
+	for msg := range c {
+		if !errors.Is(msg.Error, ErrNoSig) {
+			t.Fatal("expecting ErrNoSig error")
+		}
+	}
+}
+
 func TestCustomTsigProvider(t *testing.T) {
 	HandleFunc("miek.nl.", SingleEnvelopeXfrServer)
 	defer HandleRemove("miek.nl.")
@@ -235,3 +259,16 @@ func TestCustomTsigProvider(t *testing.T) {
 
 	axfrTestingSuiteWithCustomTsig(t, addrstr, tsigSecretProvider(tsigSecret))
 }
+
+func TestTSIGNotSigned(t *testing.T) {
+	HandleFunc("miek.nl.", SingleEnvelopeXfrServer)
+	defer HandleRemove("miek.nl.")
+
+	s, addrstr, _, err := RunLocalTCPServer(":0")
+	if err != nil {
+		t.Fatalf("unable to run test server: %s", err)
+	}
+	defer s.Shutdown()
+
+	axfrTestingSuiteWithMsgNotSigned(t, addrstr, tsigSecretProvider(tsigSecret))
+}