fix: add input validation to prevent remote crash from empty/malforme…#912
fix: add input validation to prevent remote crash from empty/malforme…#912zxzxwu merged 2 commits intogoogle:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
…d PDUs Add length checks in from_bytes() for ATT and SMP protocol parsers to prevent IndexError crashes from empty PDUs sent by remote Bluetooth devices. Also add buffer size limit and UTF-8 error handling in HFP protocol to prevent memory exhaustion and decode crashes. - bumble/att.py: validate PDU is non-empty before accessing pdu[0] - bumble/smp.py: validate PDU is non-empty before accessing pdu[0] - bumble/hfp.py: limit buffer to 64KB, handle invalid UTF-8 gracefully These issues can be triggered by a remote Bluetooth device sending malformed packets, causing denial of service on the host.
ibondarenko1
force-pushed
the
fix/empty-pdu-crash
branch
from
9ca9d12 to
0a78e75
Compare
There was a problem hiding this comment.
Thanks for your PR, but I am wondering when do we need such handling?
For single packet parsing, we won't crash because there is a try-except umbrella at the packet sink root.
For HFP, despite the issue is possible, "normal" products qualified by SIG and authenticated by users should not send such invalid packets, and users can still define their own handling.
| @classmethod | ||
| def from_bytes(cls, pdu: bytes) -> ATT_PDU: | ||
| if not pdu: | ||
| raise ValueError("Empty ATT PDU") |
| @classmethod | ||
| def from_bytes(cls, pdu: bytes) -> SMP_Command: | ||
| if not pdu: | ||
| raise ValueError("Empty SMP PDU") |
| if len(self.buffer) > 65536: | ||
| logger.warning("HFP buffer overflow, truncating") | ||
| self.buffer = self.buffer[-65536:] |
There was a problem hiding this comment.
Maybe we can just abort new packets after overflow?
| if len(self.buffer) > 65536: | |
| logger.warning("HFP buffer overflow, truncating") | |
| self.buffer = self.buffer[-65536:] | |
| MAX_LINE_LENGTH = 65536 | |
| ... | |
| if len(self.buffer) > MAX_LINE_LENGTH: | |
| logger.error("HFP buffer overflow, abort") | |
| return |
…ffer overflow - att.py: raise core.InvalidPacketError instead of generic ValueError - smp.py: raise core.InvalidPacketError instead of generic ValueError - hfp.py: add MAX_BUFFER_SIZE class constant (64KB) - hfp.py: drop incoming data when it would overflow buffer instead of truncating, preserving existing partial-packet state Per review comments on PR google#912 by @zxzxwu.
|
Thanks for the review @zxzxwu — pushed follow-up commit addressing both points:
Happy to adjust naming or buffer size if preferred. |
|
@googlebot I signed it! |
DataElement.from_bytes -> list_from_bytes -> (SEQUENCE/ALTERNATIVE constructor dispatches back to list_from_bytes) had no depth limit. A malicious SDP peer could send a PDU of a few kilobytes containing ~1000 nested SEQUENCE tags and exhaust the Python recursion stack, crashing the host with an unhandled RecursionError propagating out of the SDP handler. Reachable via: any remote Bluetooth device that Bumble performs SDP service discovery against (default during Classic connection setup). Same family as PR #912 (ATT_PDU.from_bytes empty PDU IndexError) - remote unchecked-input parser crash in the Bluetooth stack. Fix: thread-local depth counter, cap nesting at 32 (well above anything a legitimate service record uses). Added two regression tests covering the deep-nesting reject path and normal 16-level-nested SEQUENCE parsing. Reproducer (4.5 KB payload, deterministic crash on 0.0.228): from bumble.sdp import DataElement inner = b"\x35\x00" for _ in range(1500): size = len(inner) if size < 65535: inner = bytes([0x36, (size >> 8) & 0xFF, size & 0xFF]) + inner DataElement.from_bytes(inner) # RecursionError before fix Signed-off-by: ibondarenko1 <ibondarenko1@users.noreply.github.com>
A remote peer can send an AVDTP frame shorter than the assembler expects. The current MessageAssembler.on_pdu() unconditionally accesses pdu[0], pdu[1], and (for START packets) pdu[2], so a 0-, 1-, or 2-byte frame raises IndexError. The exception propagates up through L2CAP's read loop and tears down the channel — same DoS class as #912 (empty ATT PDU) and #914 (unbounded SDP recursion). Fix: validate length before each access. Empty PDUs and packets shorter than the type-specific minimum are logged and dropped; the assembler stays alive so the L2CAP channel is not torn down. - bumble/avdtp.py: length guards in MessageAssembler.on_pdu before accessing pdu[0], pdu[1], pdu[2]. - tests/avdtp_test.py: regression test covering empty PDU, 1-byte SINGLE, 1-byte START, 2-byte START — all four would have raised IndexError pre-fix; assembler now drops without raising.
3 participants
…d PDUs
Add length checks in from_bytes() for ATT and SMP protocol parsers to prevent IndexError crashes from empty PDUs sent by remote Bluetooth devices. Also add buffer size limit and UTF-8 error handling in HFP protocol to prevent memory exhaustion and decode crashes.
These issues can be triggered by a remote Bluetooth device sending malformed packets, causing denial of service on the host.