Skip to content

Commit 642efe7

Browse files
bdracobdraco
committed
fix: bound duplicate-packet dedup against alternating-payload floods
Replace the single-slot `self.data` remembered last packet with a bounded recency window keyed on payload bytes; an attacker alternating two byte-distinct payloads (A, B, A, B, ...) previously slipped past dedup on every packet, forcing every datagram through DNSIncoming parse and the deferred-queue dedup scan. State is kept on each AsyncListener (one per reader socket / interface) so a duplicate seen on one interface does not suppress a legitimate QU or unicast reply that arrives on another. Only non-QU payloads are cached so QU questions still get unicast replies on every receipt. The single-slot fast path is retained ahead of the dict check so the steady-state dedup hit stays a pure-C compare and matches the prior performance on `test_dedup_hit_same_payload`. Fixes #1724
1 parent a7cefe9 commit 642efe7

6 files changed

Lines changed: 168 additions & 20 deletions

File tree

‎src/zeroconf/_listener.pxd‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ cdef bint TYPE_CHECKING
1414

1515
cdef cython.uint _MAX_MSG_ABSOLUTE
1616
cdef cython.uint _DUPLICATE_PACKET_SUPPRESSION_INTERVAL
17+
cdef cython.uint _RECENT_PACKETS_MAX
1718

1819

1920
cdef class AsyncListener:
@@ -30,11 +31,12 @@ cdef class AsyncListener:
3031
cdef public cython.dict _deferred
3132
cdef public cython.dict _timers
3233
cdef public cython.dict _deferred_deadlines
34+
cdef public cython.dict _recent_packets
3335

3436
@cython.locals(now=double, debug=cython.bint)
3537
cpdef datagram_received(self, cython.bytes bytes, cython.tuple addrs)
3638

37-
@cython.locals(msg=DNSIncoming)
39+
@cython.locals(msg=DNSIncoming, recent_packets=cython.dict, recent_time=double)
3840
cpdef _process_datagram_at_time(self, bint debug, cython.uint data_len, double now, bytes data, cython.tuple addrs)
3941

4042
cdef _cancel_any_timers_for_addr(self, object addr)

‎src/zeroconf/_listener.py‎

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@
3232
from ._protocol.incoming import DNSIncoming
3333
from ._transport import _WrappedTransport, make_wrapped_transport
3434
from ._utils.time import current_time_millis, millis_to_seconds
35-
from .const import _DUPLICATE_PACKET_SUPPRESSION_INTERVAL, _MAX_MSG_ABSOLUTE
35+
from .const import _DUPLICATE_PACKET_SUPPRESSION_INTERVAL, _MAX_MSG_ABSOLUTE, _RECENT_PACKETS_MAX
3636

3737
if TYPE_CHECKING:
3838
from ._core import Zeroconf
@@ -60,6 +60,7 @@ class AsyncListener:
6060
"_deferred",
6161
"_deferred_deadlines",
6262
"_query_handler",
63+
"_recent_packets",
6364
"_record_manager",
6465
"_registry",
6566
"_timers",
@@ -84,6 +85,12 @@ def __init__(self, zc: Zeroconf) -> None:
8485
self._deferred: dict[str, list[DNSIncoming]] = {}
8586
self._timers: dict[str, asyncio.TimerHandle] = {}
8687
self._deferred_deadlines: dict[str, float] = {}
88+
# Bounded recency window so an alternating (A, B, A, B, ...)
89+
# flood cannot defeat single-slot dedup; relies on dict insertion
90+
# order so the oldest entry is evicted first. Only payloads
91+
# without a QU question are cached so unicast replies still go
92+
# out on every receipt.
93+
self._recent_packets: dict[bytes, float] = {}
8794
super().__init__()
8895

8996
def datagram_received(self, data: _bytes, addrs: tuple[str, int] | tuple[str, int, int, int]) -> None:
@@ -130,6 +137,25 @@ def _process_datagram_at_time(
130137
)
131138
return
132139

140+
recent_packets = self._recent_packets
141+
# `get(data, 0.0)` so the suppression compare stays a single C
142+
# double compare and skips the `is None` branch; stored values
143+
# are millisecond timestamps so 0.0 never collides with a real
144+
# entry. Only non-QU payloads are cached, so any hit here is
145+
# safe to suppress without re-checking has_qu_question.
146+
recent_time = recent_packets.get(data, 0.0)
147+
if (now - _DUPLICATE_PACKET_SUPPRESSION_INTERVAL) < recent_time:
148+
if debug:
149+
log.debug(
150+
"Ignoring duplicate message with no unicast questions"
151+
" received from %s [socket %s] (%d bytes) as [%r]",
152+
addrs,
153+
self.sock_description,
154+
data_len,
155+
data,
156+
)
157+
return
158+
133159
if len(addrs) == 2:
134160
v6_flow_scope: tuple[()] | tuple[int, int] = ()
135161
# https://github.com/python/mypy/issues/1178
@@ -150,6 +176,15 @@ def _process_datagram_at_time(
150176
self.data = data
151177
self.last_time = now
152178
self.last_message = msg
179+
if not msg.has_qu_question():
180+
# Refresh LRU position when an entry exists but the
181+
# suppression window has expired; otherwise evict the oldest
182+
# entry once the window is full.
183+
if data in recent_packets:
184+
del recent_packets[data]
185+
elif len(recent_packets) >= _RECENT_PACKETS_MAX:
186+
del recent_packets[next(iter(recent_packets))]
187+
recent_packets[data] = now
153188
if msg.valid is True:
154189
if debug:
155190
log.debug(

‎src/zeroconf/const.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
_LISTENER_TIME = 200 # ms
3434
_BROWSER_TIME = 10000 # ms
3535
_DUPLICATE_PACKET_SUPPRESSION_INTERVAL = 1000 # ms
36+
_RECENT_PACKETS_MAX = 16 # Bounded recency window for per-listener dedup
3637
_DUPLICATE_QUESTION_INTERVAL = 999 # ms # Must be 1ms less than _DUPLICATE_PACKET_SUPPRESSION_INTERVAL
3738
_CACHE_CLEANUP_INTERVAL = 10 # s
3839
_LOADED_SYSTEM_TIMEOUT = 10 # s

‎tests/__init__.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,14 @@ def has_working_ipv6():
118118
def _clear_cache(zc: Zeroconf) -> None:
119119
zc.cache.cache.clear()
120120
zc.question_history.clear()
121+
# Reset per-listener dedup state so identical packets sent in the
122+
# next phase of the test are not suppressed by the bounded recency
123+
# window populated during the previous phase.
124+
if zc.engine is not None:
125+
for protocol in zc.engine.protocols:
126+
protocol._recent_packets.clear()
127+
protocol.data = None
128+
protocol.last_time = 0
121129

122130

123131
def _backdate_cache(zc: Zeroconf, ms: int = 1100) -> None:

‎tests/test_handlers.py‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1758,7 +1758,8 @@ async def test_response_aggregation_timings_multiple(run_isolated, disable_dupli
17581758
with patch.object(aiozc.zeroconf, "async_send") as send_mock:
17591759
send_mock.reset_mock()
17601760
protocol.datagram_received(query2.packets()[0], ("127.0.0.1", const._MDNS_PORT))
1761-
protocol.last_time = 0 # manually reset the last time to avoid duplicate packet suppression
1761+
protocol.last_time = 0 # manually reset to avoid duplicate packet suppression
1762+
protocol._recent_packets.clear()
17621763
await asyncio.sleep(0.2)
17631764
calls = send_mock.mock_calls
17641765
assert len(calls) == 1
@@ -1769,7 +1770,8 @@ async def test_response_aggregation_timings_multiple(run_isolated, disable_dupli
17691770

17701771
send_mock.reset_mock()
17711772
protocol.datagram_received(query2.packets()[0], ("127.0.0.1", const._MDNS_PORT))
1772-
protocol.last_time = 0 # manually reset the last time to avoid duplicate packet suppression
1773+
protocol.last_time = 0 # manually reset to avoid duplicate packet suppression
1774+
protocol._recent_packets.clear()
17731775
await asyncio.sleep(1.2)
17741776
calls = send_mock.mock_calls
17751777
assert len(calls) == 1
@@ -1780,9 +1782,11 @@ async def test_response_aggregation_timings_multiple(run_isolated, disable_dupli
17801782

17811783
send_mock.reset_mock()
17821784
protocol.datagram_received(query2.packets()[0], ("127.0.0.1", const._MDNS_PORT))
1783-
protocol.last_time = 0 # manually reset the last time to avoid duplicate packet suppression
1785+
protocol.last_time = 0 # manually reset to avoid duplicate packet suppression
1786+
protocol._recent_packets.clear()
17841787
protocol.datagram_received(query2.packets()[0], ("127.0.0.1", const._MDNS_PORT))
1785-
protocol.last_time = 0 # manually reset the last time to avoid duplicate packet suppression
1788+
protocol.last_time = 0 # manually reset to avoid duplicate packet suppression
1789+
protocol._recent_packets.clear()
17861790
# The minimum protected send_after is 1000ms + 20ms random; sleep
17871791
# well under that so coarse timers on slow runners cannot push the
17881792
# send into this window and flake the assertion.

‎tests/test_listener.py‎

Lines changed: 112 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -222,15 +222,16 @@ def handle_query_or_defer(
222222
_handle_query_or_defer.assert_called_once()
223223
_handle_query_or_defer.reset_mock()
224224

225-
# Now call with the different packet and handle_query_or_defer should fire
225+
# Replay the first packet — the recency window remembers more than
226+
# just the most recent payload, so this is a duplicate.
226227
listener._process_datagram_at_time(
227228
False,
228229
len(packet_with_qm_question),
229230
new_time,
230231
packet_with_qm_question,
231232
addrs,
232233
)
233-
_handle_query_or_defer.assert_called_once()
234+
_handle_query_or_defer.assert_not_called()
234235
_handle_query_or_defer.reset_mock()
235236

236237
# Now call with the different packet with qu question and handle_query_or_defer should fire
@@ -257,18 +258,8 @@ def handle_query_or_defer(
257258

258259
log.setLevel(logging.WARNING)
259260

260-
# Call with the QM packet again
261-
listener._process_datagram_at_time(
262-
False,
263-
len(packet_with_qm_question),
264-
new_time,
265-
packet_with_qm_question,
266-
addrs,
267-
)
268-
_handle_query_or_defer.assert_called_once()
269-
_handle_query_or_defer.reset_mock()
270-
271-
# Now call with the same packet again and handle_query_or_defer should not fire
261+
# Replay the QM packet with debug disabled — suppression must hold
262+
# off the debug-log path too.
272263
listener._process_datagram_at_time(
273264
False,
274265
len(packet_with_qm_question),
@@ -285,3 +276,110 @@ def handle_query_or_defer(
285276
_handle_query_or_defer.reset_mock()
286277

287278
zc.close()
279+
280+
281+
def test_guard_against_alternating_duplicate_packets() -> None:
282+
"""Alternating two distinct payloads must not bypass duplicate suppression."""
283+
zc = Zeroconf(interfaces=["127.0.0.1"])
284+
zc.registry.async_add(
285+
ServiceInfo(
286+
"_http._tcp.local.",
287+
"Test._http._tcp.local.",
288+
server="Test._http._tcp.local.",
289+
port=4,
290+
)
291+
)
292+
zc.question_history = QuestionHistoryWithoutSuppression()
293+
294+
class SubListener(_listener.AsyncListener):
295+
def handle_query_or_defer(
296+
self,
297+
msg: DNSIncoming,
298+
addr: str,
299+
port: int,
300+
transport: _engine._WrappedTransport,
301+
v6_flow_scope: tuple[()] | tuple[int, int] = (),
302+
) -> None:
303+
super().handle_query_or_defer(msg, addr, port, transport, v6_flow_scope)
304+
305+
listener = SubListener(zc)
306+
listener.transport = MagicMock()
307+
308+
query_a = r.DNSOutgoing(const._FLAGS_QR_QUERY, multicast=True)
309+
query_a.add_question(r.DNSQuestion("a._http._tcp.local.", const._TYPE_PTR, const._CLASS_IN))
310+
packet_a = query_a.packets()[0]
311+
312+
query_b = r.DNSOutgoing(const._FLAGS_QR_QUERY, multicast=True)
313+
query_b.add_question(r.DNSQuestion("b._http._tcp.local.", const._TYPE_PTR, const._CLASS_IN))
314+
packet_b = query_b.packets()[0]
315+
316+
assert packet_a != packet_b
317+
318+
addrs = ("1.2.3.4", 43)
319+
320+
with patch.object(listener, "handle_query_or_defer") as _handle_query_or_defer:
321+
now = current_time_millis()
322+
323+
# Prime both payloads.
324+
listener._process_datagram_at_time(False, len(packet_a), now, packet_a, addrs)
325+
listener._process_datagram_at_time(False, len(packet_b), now, packet_b, addrs)
326+
assert _handle_query_or_defer.call_count == 2
327+
_handle_query_or_defer.reset_mock()
328+
329+
for _ in range(4):
330+
listener._process_datagram_at_time(False, len(packet_a), now, packet_a, addrs)
331+
listener._process_datagram_at_time(False, len(packet_b), now, packet_b, addrs)
332+
_handle_query_or_defer.assert_not_called()
333+
334+
zc.close()
335+
336+
337+
def test_recent_packets_window_is_bounded() -> None:
338+
"""Distinct payloads beyond the recency window evict oldest entries."""
339+
zc = Zeroconf(interfaces=["127.0.0.1"])
340+
zc.registry.async_add(
341+
ServiceInfo(
342+
"_http._tcp.local.",
343+
"Test._http._tcp.local.",
344+
server="Test._http._tcp.local.",
345+
port=4,
346+
)
347+
)
348+
zc.question_history = QuestionHistoryWithoutSuppression()
349+
350+
class SubListener(_listener.AsyncListener):
351+
def handle_query_or_defer(
352+
self,
353+
msg: DNSIncoming,
354+
addr: str,
355+
port: int,
356+
transport: _engine._WrappedTransport,
357+
v6_flow_scope: tuple[()] | tuple[int, int] = (),
358+
) -> None:
359+
super().handle_query_or_defer(msg, addr, port, transport, v6_flow_scope)
360+
361+
listener = SubListener(zc)
362+
listener.transport = MagicMock()
363+
364+
addrs = ("1.2.3.4", 43)
365+
now = current_time_millis()
366+
367+
packets = []
368+
for i in range(const._RECENT_PACKETS_MAX + 4):
369+
query = r.DNSOutgoing(const._FLAGS_QR_QUERY, multicast=True)
370+
query.add_question(r.DNSQuestion(f"n{i}._http._tcp.local.", const._TYPE_PTR, const._CLASS_IN))
371+
packets.append(query.packets()[0])
372+
373+
with patch.object(listener, "handle_query_or_defer") as _handle_query_or_defer:
374+
for packet in packets:
375+
listener._process_datagram_at_time(False, len(packet), now, packet, addrs)
376+
assert _handle_query_or_defer.call_count == len(packets)
377+
_handle_query_or_defer.reset_mock()
378+
379+
# The oldest packets should have been evicted and now replay.
380+
evicted = packets[: len(packets) - const._RECENT_PACKETS_MAX]
381+
for packet in evicted:
382+
listener._process_datagram_at_time(False, len(packet), now, packet, addrs)
383+
assert _handle_query_or_defer.call_count == len(evicted)
384+
385+
zc.close()

0 commit comments

Comments
 (0)