Free pending_query_count slot when DNS proof build fails
What the developer wrote
Authored by Elias Rohrer
Free pending_query_count slot when DNS proof build fails
`OMDomainResolver` rate-limits in-flight DNSSEC proof builds via a
`pending_query_count` counter capped at `MAX_PENDING_RESPONSES` (1024).
The counter was only released when the proof build succeeded, so any
failure mode -- NXDOMAIN, insecure zones, unreachable resolvers, I/O
timeouts, malformed names -- permanently consumed a slot.
Because the queried name is attacker-controlled (it travels in over a
`DNSSECQuery` onion message from any LN peer, given DNS resolution is
an opt-in network-advertised feature), an adversary could exhaust the
counter with ~1025 failing queries and persistently DoS the resolver
for any subsequent legitimate BIP-353 lookups, until the process is
restarted.
Always release the slot once the proof build completes, regardless of
outcome, and add a regression test which points the resolver at a
TCP-refusing local port and asserts the counter returns to zero.
Co-Authored-By: HAL 9000
What changed, and why it matters
This commit fixes a denial-of-service bug in rust-lightning's optional DNS resolver feature. A counter that limits how many DNS lookups can run at once was only being reset when a lookup succeeded; any failed lookup (bad name, unreachable server, timeout, etc.) permanently consumed one slot. Because any Lightning peer can ask for a DNS lookup, an attacker could send about 1,025 failing requests and permanently block the resolver until the node is restarted. The fix moves the counter reset so it always runs when the lookup finishes, success or failure, and adds a test that triggers a failure and checks the counter returns to zero.
Treat this as a security fix and include it in the next maintenance release. Users running nodes with the DNS resolver feature enabled should upgrade. No immediate workaround is described; disabling the DNS resolver feature would avoid exposure if it is opt-in.
Security signals we found
Denial-of-service via resource exhaustion
Missing cleanup on error path
Counter leak in rate limiter
Attacker-controlled input (DNS query name from onion message)
Persistent DoS until process restart
Regression test added for failure-path cleanup
Evidence from the diff
OMDomainResolver uses pending_query_count, capped at MAX_PENDING_RESPONSES (1024), to rate-limit concurrent DNSSEC proof builds. Previously, in the async task spawned by handle_dnssec_query, pending_query_count.fetch_sub(1) was only executed inside the success branch after build_txt_proof_async returned Ok. On any Err path the slot leaked. The patch moves the decrement outside the if-let Ok block so it executes unconditionally after the proof build completes. A regression test binds the resolver to 127.0.0.1:1 (which refuses TCP), sends a DNSSECQuery onion message, and asserts pending_query_count returns to zero within 10 seconds.
Changed components
lightning-dns-resolver/src/lib.rsOMDomainResolver::handle_dnssec_querypending_query_count rate limiterInspect captured patch +92 / −1
diff --git a/lightning-dns-resolver/src/lib.rs b/lightning-dns-resolver/src/lib.rs
index c6d583a..90f1ac0 100644
--- a/lightning-dns-resolver/src/lib.rs
+++ b/lightning-dns-resolver/src/lib.rs
@@ -125,8 +125,8 @@ impl<PH: DNSResolverMessageHandler> DNSResolverMessageHandler for OMDomainResolv
let contents = DNSResolverMessage::DNSSECProof(DNSSECProof { name: q.0, proof });
let instructions = responder.respond().into_instructions();
us.pending_replies.lock().unwrap().push((contents, instructions));
- us.pending_query_count.fetch_sub(1, Ordering::Relaxed);
}
+ us.pending_query_count.fetch_sub(1, Ordering::Relaxed);
});
None
}
@@ -337,4 +337,95 @@ mod test {
assert_eq!(resolution.1, payment_id);
assert!(resolution.2[.."bitcoin:".len()].eq_ignore_ascii_case("bitcoin:"));
}
+
+ #[tokio::test]
+ async fn failed_query_does_not_leak_pending_counter() {
+ use std::sync::atomic::Ordering;
+
+ let secp_ctx = Secp256k1::new();
+
+ // Resolver points at a port that should refuse TCP, so build_txt_proof_async
+ // returns Err quickly.
+ let resolver_keys = Arc::new(KeysManager::new(&[99; 32], 42, 43, true));
+ let resolver_logger = TestLogger { node: "resolver" };
+ let resolver =
+ Arc::new(OMDomainResolver::<IgnoringMessageHandler>::ignoring_incoming_proofs(
+ "127.0.0.1:1".parse().unwrap(),
+ ));
+ let resolver_state = Arc::clone(&resolver.state);
+ let resolver_messenger = OnionMessenger::new(
+ Arc::clone(&resolver_keys),
+ Arc::clone(&resolver_keys),
+ resolver_logger,
+ DummyNodeLookup {},
+ DirectlyConnectedRouter {},
+ IgnoringMessageHandler {},
+ IgnoringMessageHandler {},
+ Arc::clone(&resolver),
+ IgnoringMessageHandler {},
+ );
+ let resolver_id = resolver_keys.get_node_id(Recipient::Node).unwrap();
+
+ let resolver_dest = Destination::Node(resolver_id);
+ let now = SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap().as_secs();
+
+ let payment_id = PaymentId([42; 32]);
+ let name = HumanReadableName::from_encoded("matt@mattcorallo.com").unwrap();
+
+ let payer_keys = Arc::new(KeysManager::new(&[2; 32], 42, 43, true));
+ let payer_logger = TestLogger { node: "payer" };
+ let payer_id = payer_keys.get_node_id(Recipient::Node).unwrap();
+ let payer = Arc::new(URIResolver {
+ resolved_uri: Mutex::new(None),
+ resolver: OMNameResolver::new(now as u32, 1),
+ pending_messages: Mutex::new(Vec::new()),
+ });
+ let payer_messenger = Arc::new(OnionMessenger::new(
+ Arc::clone(&payer_keys),
+ Arc::clone(&payer_keys),
+ payer_logger,
+ DummyNodeLookup {},
+ DirectlyConnectedRouter {},
+ IgnoringMessageHandler {},
+ IgnoringMessageHandler {},
+ Arc::clone(&payer),
+ IgnoringMessageHandler {},
+ ));
+
+ let init_msg = get_om_init();
+ payer_messenger.peer_connected(resolver_id, &init_msg, true).unwrap();
+ resolver_messenger.peer_connected(payer_id, &init_msg, false).unwrap();
+
+ let (msg, context) =
+ payer.resolver.resolve_name(payment_id, name.clone(), &*payer_keys).unwrap();
+ let query_context = MessageContext::DNSResolver(context);
+ let receive_key = payer_keys.get_receive_auth_key();
+ let reply_path = BlindedMessagePath::one_hop(
+ payer_id,
+ receive_key,
+ query_context,
+ false,
+ &*payer_keys,
+ &secp_ctx,
+ );
+ payer.pending_messages.lock().unwrap().push((
+ DNSResolverMessage::DNSSECQuery(msg),
+ MessageSendInstructions::WithSpecifiedReplyPath {
+ destination: resolver_dest,
+ reply_path,
+ },
+ ));
+
+ let query = payer_messenger.next_onion_message_for_peer(resolver_id).unwrap();
+ resolver_messenger.handle_onion_message(payer_id, &query);
+
+ let start = Instant::now();
+ while resolver_state.pending_query_count.load(Ordering::Relaxed) != 0 {
+ tokio::time::sleep(Duration::from_millis(50)).await;
+ assert!(
+ start.elapsed() < Duration::from_secs(10),
+ "pending_query_count not decremented after failed proof: counter leaks"
+ );
+ }
+ }
}
Why this scored 76/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.