fix: bound duplicate-packet dedup against alternating-payload floods

bdraco · bdraco · commit 303a453a6711 · 2026-05-20T08:11:38.000-05:00
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
diff --git a/src/zeroconf/_listener.pxd b/src/zeroconf/_listener.pxd
@@ -14,6 +14,7 @@ cdef bint TYPE_CHECKING
 
 cdef cython.uint _MAX_MSG_ABSOLUTE
 cdef cython.uint _DUPLICATE_PACKET_SUPPRESSION_INTERVAL
+cdef cython.uint _RECENT_PACKETS_MAX
 
 
 cdef class AsyncListener:
@@ -30,11 +31,12 @@ cdef class AsyncListener:
     cdef public cython.dict _deferred
     cdef public cython.dict _timers
     cdef public cython.dict _deferred_deadlines
+    cdef public cython.dict _recent_packets
 
     @cython.locals(now=double, debug=cython.bint)
     cpdef datagram_received(self, cython.bytes bytes, cython.tuple addrs)
 
-    @cython.locals(msg=DNSIncoming)
+    @cython.locals(msg=DNSIncoming, recent_time=double)
     cpdef _process_datagram_at_time(self, bint debug, cython.uint data_len, double now, bytes data, cython.tuple addrs)
 
     cdef _cancel_any_timers_for_addr(self, object addr)
diff --git a/src/zeroconf/_listener.py b/src/zeroconf/_listener.py
@@ -32,7 +32,7 @@
 from ._protocol.incoming import DNSIncoming
 from ._transport import _WrappedTransport, make_wrapped_transport
 from ._utils.time import current_time_millis, millis_to_seconds
-from .const import _DUPLICATE_PACKET_SUPPRESSION_INTERVAL, _MAX_MSG_ABSOLUTE
+from .const import _DUPLICATE_PACKET_SUPPRESSION_INTERVAL, _MAX_MSG_ABSOLUTE, _RECENT_PACKETS_MAX
 
 if TYPE_CHECKING:
     from ._core import Zeroconf
@@ -60,6 +60,7 @@ class AsyncListener:
         "_deferred",
         "_deferred_deadlines",
         "_query_handler",
+        "_recent_packets",
         "_record_manager",
         "_registry",
         "_timers",
@@ -84,6 +85,12 @@ def __init__(self, zc: Zeroconf) -> None:
         self._deferred: dict[str, list[DNSIncoming]] = {}
         self._timers: dict[str, asyncio.TimerHandle] = {}
         self._deferred_deadlines: dict[str, float] = {}
+        # Bounded recency window so an alternating (A, B, A, B, ...)
+        # flood cannot defeat single-slot dedup; relies on dict insertion
+        # order so the oldest entry is evicted first. Only payloads
+        # without a QU question are cached so unicast replies still go
+        # out on every receipt.
+        self._recent_packets: dict[bytes, float] = {}
         super().__init__()
 
     def datagram_received(self, data: _bytes, addrs: tuple[str, int] | tuple[str, int, int, int]) -> None:
@@ -130,6 +137,31 @@ def _process_datagram_at_time(
                 )
             return
 
+        # `get(data, -1e30)` keeps the suppression compare a single C
+        # double compare; the sentinel is far below any real `now -
+        # _DUPLICATE_PACKET_SUPPRESSION_INTERVAL` so a cache miss never
+        # triggers the branch even when `now` is small (time.monotonic
+        # is allowed to start near zero, leaving `now - INTERVAL`
+        # negative for the first ~1s after boot). Only non-QU payloads
+        # are cached, so any hit here is safe to suppress without re-
+        # checking has_qu_question.
+        recent_time = self._recent_packets.get(data, -1e30)
+        if (now - _DUPLICATE_PACKET_SUPPRESSION_INTERVAL) < recent_time:
+            # No timestamp refresh on hit so the suppression window
+            # ends at first-observation + interval; one parse-and-
+            # dispatch fires per payload per interval, capping the
+            # CPU cost under a sustained alternating flood.
+            if debug:
+                log.debug(
+                    "Ignoring duplicate message with no unicast questions"
+                    " received from %s [socket %s] (%d bytes) as [%r]",
+                    addrs,
+                    self.sock_description,
+                    data_len,
+                    data,
+                )
+            return
+
         if len(addrs) == 2:
             v6_flow_scope: tuple[()] | tuple[int, int] = ()
             # https://github.com/python/mypy/issues/1178
@@ -150,6 +182,15 @@ def _process_datagram_at_time(
         self.data = data
         self.last_time = now
         self.last_message = msg
+        if not msg.has_qu_question():
+            # Refresh LRU position when an entry exists but the
+            # suppression window has expired; otherwise evict the oldest
+            # entry once the window is full.
+            if data in self._recent_packets:
+                del self._recent_packets[data]
+            elif len(self._recent_packets) >= _RECENT_PACKETS_MAX:
+                del self._recent_packets[next(iter(self._recent_packets))]
+            self._recent_packets[data] = now
         if msg.valid is True:
             if debug:
                 log.debug(
diff --git a/src/zeroconf/const.py b/src/zeroconf/const.py
@@ -33,6 +33,7 @@
 _LISTENER_TIME = 200  # ms
 _BROWSER_TIME = 10000  # ms
 _DUPLICATE_PACKET_SUPPRESSION_INTERVAL = 1000  # ms
+_RECENT_PACKETS_MAX = 16  # Bounded recency window for per-listener dedup
 _DUPLICATE_QUESTION_INTERVAL = 999  # ms # Must be 1ms less than _DUPLICATE_PACKET_SUPPRESSION_INTERVAL
 _CACHE_CLEANUP_INTERVAL = 10  # s
 _LOADED_SYSTEM_TIMEOUT = 10  # s
diff --git a/tests/__init__.py b/tests/__init__.py
@@ -118,6 +118,14 @@ def has_working_ipv6():
 def _clear_cache(zc: Zeroconf) -> None:
     zc.cache.cache.clear()
     zc.question_history.clear()
+    # Reset per-listener dedup state so identical packets sent in the
+    # next phase of the test are not suppressed by the bounded recency
+    # window populated during the previous phase.
+    if zc.engine is not None:
+        for protocol in zc.engine.protocols:
+            protocol._recent_packets.clear()
+            protocol.data = None
+            protocol.last_time = 0
 
 
 def _backdate_cache(zc: Zeroconf, ms: int = 1100) -> None:
diff --git a/tests/test_handlers.py b/tests/test_handlers.py
@@ -1758,7 +1758,8 @@ async def test_response_aggregation_timings_multiple(run_isolated, disable_dupli
     with patch.object(aiozc.zeroconf, "async_send") as send_mock:
         send_mock.reset_mock()
         protocol.datagram_received(query2.packets()[0], ("127.0.0.1", const._MDNS_PORT))
-        protocol.last_time = 0  # manually reset the last time to avoid duplicate packet suppression
+        protocol.last_time = 0  # manually reset to avoid duplicate packet suppression
+        protocol._recent_packets.clear()
         await asyncio.sleep(0.2)
         calls = send_mock.mock_calls
         assert len(calls) == 1
@@ -1769,7 +1770,8 @@ async def test_response_aggregation_timings_multiple(run_isolated, disable_dupli
 
         send_mock.reset_mock()
         protocol.datagram_received(query2.packets()[0], ("127.0.0.1", const._MDNS_PORT))
-        protocol.last_time = 0  # manually reset the last time to avoid duplicate packet suppression
+        protocol.last_time = 0  # manually reset to avoid duplicate packet suppression
+        protocol._recent_packets.clear()
         await asyncio.sleep(1.2)
         calls = send_mock.mock_calls
         assert len(calls) == 1
@@ -1780,9 +1782,11 @@ async def test_response_aggregation_timings_multiple(run_isolated, disable_dupli
 
         send_mock.reset_mock()
         protocol.datagram_received(query2.packets()[0], ("127.0.0.1", const._MDNS_PORT))
-        protocol.last_time = 0  # manually reset the last time to avoid duplicate packet suppression
+        protocol.last_time = 0  # manually reset to avoid duplicate packet suppression
+        protocol._recent_packets.clear()
         protocol.datagram_received(query2.packets()[0], ("127.0.0.1", const._MDNS_PORT))
-        protocol.last_time = 0  # manually reset the last time to avoid duplicate packet suppression
+        protocol.last_time = 0  # manually reset to avoid duplicate packet suppression
+        protocol._recent_packets.clear()
         # The minimum protected send_after is 1000ms + 20ms random; sleep
         # well under that so coarse timers on slow runners cannot push the
         # send into this window and flake the assertion.
diff --git a/tests/test_listener.py b/tests/test_listener.py
@@ -222,15 +222,16 @@ def handle_query_or_defer(
         _handle_query_or_defer.assert_called_once()
         _handle_query_or_defer.reset_mock()
 
-        # Now call with the different packet and handle_query_or_defer should fire
+        # Replay the first packet — the recency window remembers more than
+        # just the most recent payload, so this is a duplicate.
         listener._process_datagram_at_time(
             False,
             len(packet_with_qm_question),
             new_time,
             packet_with_qm_question,
             addrs,
         )
-        _handle_query_or_defer.assert_called_once()
+        _handle_query_or_defer.assert_not_called()
         _handle_query_or_defer.reset_mock()
 
         # 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(
 
         log.setLevel(logging.WARNING)
 
-        # Call with the QM packet again
-        listener._process_datagram_at_time(
-            False,
-            len(packet_with_qm_question),
-            new_time,
-            packet_with_qm_question,
-            addrs,
-        )
-        _handle_query_or_defer.assert_called_once()
-        _handle_query_or_defer.reset_mock()
-
-        # Now call with the same packet again and handle_query_or_defer should not fire
+        # Replay the QM packet with debug disabled — suppression must hold
+        # off the debug-log path too.
         listener._process_datagram_at_time(
             False,
             len(packet_with_qm_question),
@@ -285,3 +276,164 @@ def handle_query_or_defer(
         _handle_query_or_defer.reset_mock()
 
     zc.close()
+
+
+def test_guard_against_alternating_duplicate_packets() -> None:
+    """Alternating two distinct payloads must not bypass duplicate suppression."""
+    zc = Zeroconf(interfaces=["127.0.0.1"])
+    zc.registry.async_add(
+        ServiceInfo(
+            "_http._tcp.local.",
+            "Test._http._tcp.local.",
+            server="Test._http._tcp.local.",
+            port=4,
+        )
+    )
+    zc.question_history = QuestionHistoryWithoutSuppression()
+
+    class SubListener(_listener.AsyncListener):
+        def handle_query_or_defer(
+            self,
+            msg: DNSIncoming,
+            addr: str,
+            port: int,
+            transport: _engine._WrappedTransport,
+            v6_flow_scope: tuple[()] | tuple[int, int] = (),
+        ) -> None:
+            super().handle_query_or_defer(msg, addr, port, transport, v6_flow_scope)
+
+    listener = SubListener(zc)
+    listener.transport = MagicMock()
+
+    query_a = r.DNSOutgoing(const._FLAGS_QR_QUERY, multicast=True)
+    query_a.add_question(r.DNSQuestion("a._http._tcp.local.", const._TYPE_PTR, const._CLASS_IN))
+    packet_a = query_a.packets()[0]
+
+    query_b = r.DNSOutgoing(const._FLAGS_QR_QUERY, multicast=True)
+    query_b.add_question(r.DNSQuestion("b._http._tcp.local.", const._TYPE_PTR, const._CLASS_IN))
+    packet_b = query_b.packets()[0]
+
+    assert packet_a != packet_b
+
+    addrs = ("1.2.3.4", 43)
+
+    with patch.object(listener, "handle_query_or_defer") as _handle_query_or_defer:
+        now = current_time_millis()
+
+        # Prime both payloads.
+        listener._process_datagram_at_time(False, len(packet_a), now, packet_a, addrs)
+        listener._process_datagram_at_time(False, len(packet_b), now, packet_b, addrs)
+        assert _handle_query_or_defer.call_count == 2
+        _handle_query_or_defer.reset_mock()
+
+        for _ in range(4):
+            listener._process_datagram_at_time(False, len(packet_a), now, packet_a, addrs)
+            listener._process_datagram_at_time(False, len(packet_b), now, packet_b, addrs)
+        _handle_query_or_defer.assert_not_called()
+
+    zc.close()
+
+
+def test_recent_packets_window_is_bounded() -> None:
+    """Distinct payloads beyond the recency window evict oldest entries."""
+    zc = Zeroconf(interfaces=["127.0.0.1"])
+    zc.registry.async_add(
+        ServiceInfo(
+            "_http._tcp.local.",
+            "Test._http._tcp.local.",
+            server="Test._http._tcp.local.",
+            port=4,
+        )
+    )
+    zc.question_history = QuestionHistoryWithoutSuppression()
+
+    class SubListener(_listener.AsyncListener):
+        def handle_query_or_defer(
+            self,
+            msg: DNSIncoming,
+            addr: str,
+            port: int,
+            transport: _engine._WrappedTransport,
+            v6_flow_scope: tuple[()] | tuple[int, int] = (),
+        ) -> None:
+            super().handle_query_or_defer(msg, addr, port, transport, v6_flow_scope)
+
+    listener = SubListener(zc)
+    listener.transport = MagicMock()
+
+    addrs = ("1.2.3.4", 43)
+    now = current_time_millis()
+
+    packets = []
+    for i in range(const._RECENT_PACKETS_MAX + 4):
+        query = r.DNSOutgoing(const._FLAGS_QR_QUERY, multicast=True)
+        query.add_question(r.DNSQuestion(f"n{i}._http._tcp.local.", const._TYPE_PTR, const._CLASS_IN))
+        packets.append(query.packets()[0])
+
+    with patch.object(listener, "handle_query_or_defer") as _handle_query_or_defer:
+        for packet in packets:
+            listener._process_datagram_at_time(False, len(packet), now, packet, addrs)
+        assert _handle_query_or_defer.call_count == len(packets)
+        _handle_query_or_defer.reset_mock()
+
+        # The newest _RECENT_PACKETS_MAX entries are still in the
+        # window; replaying them must be suppressed. Checked before
+        # replaying the evicted ones below since that would mutate the
+        # window and could mask an off-by-one in eviction.
+        kept = packets[-const._RECENT_PACKETS_MAX :]
+        for packet in kept:
+            listener._process_datagram_at_time(False, len(packet), now, packet, addrs)
+        _handle_query_or_defer.assert_not_called()
+
+        # The oldest packets should have been evicted and now replay.
+        evicted = packets[: len(packets) - const._RECENT_PACKETS_MAX]
+        for packet in evicted:
+            listener._process_datagram_at_time(False, len(packet), now, packet, addrs)
+        assert _handle_query_or_defer.call_count == len(evicted)
+
+    zc.close()
+
+
+def test_recent_packets_miss_with_small_now_is_not_suppressed() -> None:
+    """A cache miss must not trigger suppression when `now` is below the suppression interval."""
+    # time.monotonic() can start near zero on freshly booted systems, so
+    # `now - _DUPLICATE_PACKET_SUPPRESSION_INTERVAL` is negative for the
+    # first second of process lifetime. A 0.0 default on the recency
+    # dict would let any negative `now - INTERVAL` satisfy the compare
+    # and suppress legitimate traffic.
+    zc = Zeroconf(interfaces=["127.0.0.1"])
+    zc.registry.async_add(
+        ServiceInfo(
+            "_http._tcp.local.",
+            "Test._http._tcp.local.",
+            server="Test._http._tcp.local.",
+            port=4,
+        )
+    )
+    zc.question_history = QuestionHistoryWithoutSuppression()
+
+    class SubListener(_listener.AsyncListener):
+        def handle_query_or_defer(
+            self,
+            msg: DNSIncoming,
+            addr: str,
+            port: int,
+            transport: _engine._WrappedTransport,
+            v6_flow_scope: tuple[()] | tuple[int, int] = (),
+        ) -> None:
+            super().handle_query_or_defer(msg, addr, port, transport, v6_flow_scope)
+
+    listener = SubListener(zc)
+    listener.transport = MagicMock()
+
+    query = r.DNSOutgoing(const._FLAGS_QR_QUERY, multicast=True)
+    query.add_question(r.DNSQuestion("a._http._tcp.local.", const._TYPE_PTR, const._CLASS_IN))
+    packet = query.packets()[0]
+
+    addrs = ("1.2.3.4", 43)
+
+    with patch.object(listener, "handle_query_or_defer") as _handle_query_or_defer:
+        listener._process_datagram_at_time(False, len(packet), 0.0, packet, addrs)
+        _handle_query_or_defer.assert_called_once()
+
+    zc.close()
