discovery: fix panic in DNS fallback SRV lookup
What changed, and why it matters
This commit fixes a bug in LND's DNS seed bootstrap code that could crash the node. The code assumed every record in a DNS response was an SRV record, so a non-SRV record (like a normal A or CNAME record) would cause a panic. The fix safely skips non-SRV records and also handles cases where the DNS lookup returns no addresses at all. The commit message says the bug is hard to exploit because it requires a malicious DNS server or a network attacker, and such attackers already have easier ways to disrupt a node.
Apply the patch. It is a defensive hardening fix that prevents a remote-triggered daemon panic via DNS responses. Even though the commit message downplays exploitability, the fix is low-risk and improves robustness against malformed or malicious DNS seed responses and network errors.
Security signals we found
Unconditional type assertion panic in DNS fallback path
Missing bounds check on LookupHost result before array indexing
Missing network deadline on manually dialed DNS TCP connection
Downstream bootstrap errors converted from abort to skip-and-continue, improving resilience
Test coverage added for malformed/unauthenticated DNS responses
Evidence from the diff
In discovery/bootstrapper.go, fallBackSRVLookup previously used an unconditional type assertion rr.(*dns.SRV) on every Answer record, causing a runtime panic if the DNS response contained A, CNAME, or other non-SRV records. The patch switches to the comma-ok form, logs and skips non-SRV records, and returns an error if no SRV records remain. It also guards against an empty LookupHost result before indexing addrs[0], adds a connection deadline, and changes several downstream error paths in the bootstrap loop from fatal returns to log-and-continue so one bad seed entry does not abort the whole bootstrap. Tests are added to cover non-SRV records, empty answers, no SRV records, and empty shim addresses.
Changed components
discovery/bootstrapper.goDNSSeedBootstrapper.fallBackSRVLookupDNSSeedBootstrapper.SampleNodeAddrs bootstrap loopInspect captured patch +252 / −5
diff --git a/discovery/bootstrapper.go b/discovery/bootstrapper.go
index 28ef0d9..73fb2e7 100644
--- a/discovery/bootstrapper.go
+++ b/discovery/bootstrapper.go
@@ -382,6 +382,11 @@ func (d *DNSSeedBootstrapper) fallBackSRVLookup(soaShim string,
return nil, err
}
+ if len(addrs) == 0 {
+ return nil, fmt.Errorf("no addresses for fallback DNS seed "+
+ "shim %v", soaShim)
+ }
+
// Once we have the IP address, we'll establish a TCP connection using
// port 53.
dnsServer := net.JoinHostPort(addrs[0], "53")
@@ -389,6 +394,7 @@ func (d *DNSSeedBootstrapper) fallBackSRVLookup(soaShim string,
if err != nil {
return nil, err
}
+ _ = conn.SetDeadline(time.Now().Add(d.timeout))
dnsHost := fmt.Sprintf("_nodes._tcp.%v.", targetEndPoint)
dnsConn := &dns.Conn{Conn: conn}
@@ -416,7 +422,18 @@ func (d *DNSSeedBootstrapper) fallBackSRVLookup(soaShim string,
// that net.LookupSRV would normally return.
var rrs []*net.SRV
for _, rr := range resp.Answer {
- srv := rr.(*dns.SRV)
+ // The answer section may contain records other than SRV
+ // (e.g. A or CNAME), so use the comma-ok form to skip any
+ // non-SRV record instead of panicking on a failed type
+ // assertion.
+ srv, ok := rr.(*dns.SRV)
+ if !ok {
+ log.Infof("Skipping non-SRV record %T in fallback "+
+ "DNS seed response for %v", rr, targetEndPoint)
+
+ continue
+ }
+
rrs = append(rrs, &net.SRV{
Target: srv.Target,
Port: srv.Port,
@@ -425,6 +442,11 @@ func (d *DNSSeedBootstrapper) fallBackSRVLookup(soaShim string,
})
}
+ if len(rrs) == 0 {
+ return nil, fmt.Errorf("no SRV records in fallback DNS seed "+
+ "response for %v", targetEndPoint)
+ }
+
return rrs, nil
}
@@ -498,7 +520,9 @@ search:
bechNodeHost := nodeSrv.Target
addrs, err := d.net.LookupHost(bechNodeHost)
if err != nil {
- return nil, err
+ log.Tracef("Skipping node %v: %v",
+ bechNodeHost, err)
+ continue
}
if len(addrs) == 0 {
@@ -523,7 +547,9 @@ search:
bechNode := strings.Split(bechNodeHost, ".")
_, nodeBytes5Bits, err := bech32.Decode(bechNode[0])
if err != nil {
- return nil, err
+ log.Tracef("Skipping node %v: %v",
+ bechNodeHost, err)
+ continue
}
// Once we have the bech32 decoded pubkey, we'll need
@@ -534,11 +560,15 @@ search:
nodeBytes5Bits, 5, 8, false,
)
if err != nil {
- return nil, err
+ log.Tracef("Skipping node %v: %v",
+ bechNodeHost, err)
+ continue
}
nodeKey, err := btcec.ParsePubKey(nodeBytes)
if err != nil {
- return nil, err
+ log.Tracef("Skipping node %v: %v",
+ bechNodeHost, err)
+ continue
}
// If we have an ignore list, and this node is in the
diff --git a/discovery/bootstrapper_test.go b/discovery/bootstrapper_test.go
index a323b46..67769d1 100644
--- a/discovery/bootstrapper_test.go
+++ b/discovery/bootstrapper_test.go
@@ -2,12 +2,15 @@ package discovery
import (
"context"
+ "fmt"
"net"
"testing"
+ "time"
"github.com/btcsuite/btcd/btcec/v2"
"github.com/lightningnetwork/lnd/autopilot"
"github.com/lightningnetwork/lnd/tor"
+ "github.com/miekg/dns"
"github.com/stretchr/testify/require"
)
@@ -55,6 +58,60 @@ func (s *stubChannelGraph) ForEachNodesChannels(_ context.Context,
return nil
}
+// fallbackNet is a tor.Net stub used to drive fallBackSRVLookup. LookupHost
+// returns shimAddrs and Dial serves a single DNS response, written by
+// serveResp, over an in-memory pipe so the fallback path can be exercised
+// without a real DNS server.
+type fallbackNet struct {
+ shimAddrs []string
+ serveResp func(question *dns.Msg) *dns.Msg
+}
+
+// Dial returns one end of an in-memory pipe and spins up a goroutine acting as
+// the DNS server on the other end, which reads the SRV query and writes back
+// the response produced by serveResp.
+func (n *fallbackNet) Dial(_, _ string,
+ _ time.Duration) (net.Conn, error) {
+
+ client, server := net.Pipe()
+
+ // Act as the DNS server on the far end of the pipe: read the SRV
+ // query, then write back the crafted response.
+ go func() {
+ srvConn := &dns.Conn{Conn: server}
+ defer srvConn.Close()
+
+ query, err := srvConn.ReadMsg()
+ if err != nil {
+ return
+ }
+
+ _ = srvConn.WriteMsg(n.serveResp(query))
+ }()
+
+ return client, nil
+}
+
+// LookupHost returns the configured shim addresses used to reach the fallback
+// DNS server.
+func (n *fallbackNet) LookupHost(_ string) ([]string, error) {
+ return n.shimAddrs, nil
+}
+
+// LookupSRV is unsupported by this stub; the fallback path under test issues
+// the SRV query manually over the Dial connection instead.
+func (n *fallbackNet) LookupSRV(_, _, _ string,
+ _ time.Duration) (string, []*net.SRV, error) {
+
+ return "", nil, fmt.Errorf("unsupported")
+}
+
+// ResolveTCPAddr is unsupported by this stub as it is not exercised by the
+// fallback SRV lookup path.
+func (n *fallbackNet) ResolveTCPAddr(_, _ string) (*net.TCPAddr, error) {
+ return nil, fmt.Errorf("unsupported")
+}
+
// TestGraphBootstrapperSkipsV2Onion ensures SampleNodeAddrs strips Tor v2
// .onion entries from the returned candidate set so the connection manager
// never attempts to dial an obsolete v2 hidden service surfaced through the
@@ -145,3 +202,163 @@ func TestGraphBootstrapperSkipsV2Onion(t *testing.T) {
"v2-only node %x should contribute nothing", s.pub)
}
}
+
+// TestFallBackSRVLookupSkipsNonSRV ensures a DNS response whose Answer section
+// contains non-SRV records (which an on-path attacker or malicious seed can
+// inject, since the response is unauthenticated) is filtered rather than
+// triggering a type-assertion panic that would crash the daemon.
+func TestFallBackSRVLookupSkipsNonSRV(t *testing.T) {
+ t.Parallel()
+
+ const target = "nodes.lightning.directory"
+
+ srvTarget := "ln1qexample._nodes._tcp." + target + "."
+
+ netStub := &fallbackNet{
+ shimAddrs: []string{"127.0.0.1"},
+ serveResp: func(q *dns.Msg) *dns.Msg {
+ resp := new(dns.Msg)
+ resp.SetReply(q)
+ resp.Rcode = dns.RcodeSuccess
+
+ // A hostile/malformed Answer section: an A record and a
+ // CNAME interleaved with a single valid SRV record.
+ resp.Answer = []dns.RR{
+ &dns.A{
+ Hdr: dns.RR_Header{
+ Name: q.Question[0].Name,
+ Rrtype: dns.TypeA,
+ },
+ A: net.ParseIP("1.2.3.4"),
+ },
+ &dns.CNAME{
+ Hdr: dns.RR_Header{
+ Name: q.Question[0].Name,
+ Rrtype: dns.TypeCNAME,
+ },
+ Target: "evil.example.",
+ },
+ &dns.SRV{
+ Hdr: dns.RR_Header{
+ Name: q.Question[0].Name,
+ Rrtype: dns.TypeSRV,
+ },
+ Target: srvTarget,
+ Port: 9735,
+ },
+ }
+
+ return resp
+ },
+ }
+
+ bs := NewDNSSeedBootstrapper(
+ [][2]string{{target, "soa.lightning.directory"}},
+ netStub, time.Second,
+ )
+ d, ok := bs.(*DNSSeedBootstrapper)
+ require.True(t, ok)
+
+ // The non-SRV records must be skipped, leaving only the valid SRV
+ // record. Crucially, this must not panic.
+ srvs, err := d.fallBackSRVLookup("soa.lightning.directory", target)
+ require.NoError(t, err)
+ require.Len(t, srvs, 1)
+ require.Equal(t, srvTarget, srvs[0].Target)
+}
+
+// TestFallBackSRVLookupNoSRVRecords ensures a successful DNS response whose
+// Answer section holds no SRV records (only CNAME/A entries, or is empty)
+// returns an error rather than (nil, nil), so the caller does not mistake "no
+// usable records" for a successful query.
+func TestFallBackSRVLookupNoSRVRecords(t *testing.T) {
+ t.Parallel()
+
+ const target = "nodes.lightning.directory"
+
+ netStub := &fallbackNet{
+ shimAddrs: []string{"127.0.0.1"},
+ serveResp: func(q *dns.Msg) *dns.Msg {
+ resp := new(dns.Msg)
+ resp.SetReply(q)
+ resp.Rcode = dns.RcodeSuccess
+
+ // Only a non-SRV record is present in the Answer
+ // section, leaving zero usable SRV targets.
+ resp.Answer = []dns.RR{
+ &dns.A{
+ Hdr: dns.RR_Header{
+ Name: q.Question[0].Name,
+ Rrtype: dns.TypeA,
+ },
+ A: net.ParseIP("1.2.3.4"),
+ },
+ }
+
+ return resp
+ },
+ }
+
+ bs := NewDNSSeedBootstrapper(
+ [][2]string{{target, "soa.lightning.directory"}},
+ netStub, time.Second,
+ )
+ d, ok := bs.(*DNSSeedBootstrapper)
+ require.True(t, ok)
+
+ srvs, err := d.fallBackSRVLookup("soa.lightning.directory", target)
+ require.Error(t, err)
+ require.Empty(t, srvs)
+}
+
+// TestFallBackSRVLookupEmptyAnswer ensures a successful DNS response with an
+// entirely empty Answer section returns an error rather than (nil, nil), so the
+// caller does not mistake an empty response for a successful query.
+func TestFallBackSRVLookupEmptyAnswer(t *testing.T) {
+ t.Parallel()
+
+ const target = "nodes.lightning.directory"
+
+ netStub := &fallbackNet{
+ shimAddrs: []string{"127.0.0.1"},
+ serveResp: func(q *dns.Msg) *dns.Msg {
+ resp := new(dns.Msg)
+ resp.SetReply(q)
+ resp.Rcode = dns.RcodeSuccess
+
+ // Leave the Answer section empty.
+ resp.Answer = nil
+
+ return resp
+ },
+ }
+
+ bs := NewDNSSeedBootstrapper(
+ [][2]string{{target, "soa.lightning.directory"}},
+ netStub, time.Second,
+ )
+ d, ok := bs.(*DNSSeedBootstrapper)
+ require.True(t, ok)
+
+ srvs, err := d.fallBackSRVLookup("soa.lightning.directory", target)
+ require.Error(t, err)
+ require.Empty(t, srvs)
+}
+
+// TestFallBackSRVLookupNoShimAddrs ensures an empty LookupHost result for the
+// fallback shim returns an error instead of panicking on an out-of-bounds
+// index.
+func TestFallBackSRVLookupNoShimAddrs(t *testing.T) {
+ t.Parallel()
+
+ netStub := &fallbackNet{shimAddrs: nil}
+
+ bs := NewDNSSeedBootstrapper(nil, netStub, time.Second)
+ d, ok := bs.(*DNSSeedBootstrapper)
+ require.True(t, ok)
+
+ _, err := d.fallBackSRVLookup(
+ "soa.lightning.directory", "nodes.lightning.directory",
+ )
+ require.Error(t, err)
+}
Why this scored 47/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.