sdp: bound DataElement parse recursion to prevent RecursionError DoS

[ibondarenko1]

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

[zxzxwu]

I personally don't suggest that we need much protection agaist attacks here, as Bumble is not designed to be a product providing safety, but a tooling stack for test and study, so additional protection might limit the readability and flexibility for our major purpose.

Maybe need opinion from @barbibulle

[ibondarenko1]

Reasonable point, but there's a direct precedent — the ATT_PDU IndexError fix in #912 (same DoS class, same "malformed remote peer PDU crashes parser") was merged recently. That finding was also surfaced by fuzzing and the fix shipped in production Bumble.

If #912 was in-scope as "hardening against malformed peer input," #914 applies the same reasoning to SDP's nested SEQUENCE parser — a 4.5KB PDU exhausts the Python stack. The depth cap is 32 (>10× realistic SDP records), so the legitimate flexibility for testing/research is unaffected.

Happy to defer to @barbibulle's read; just wanted to name the precedent.

[zxzxwu]

#912 is also debatable - if we decide fuzzing is not applicable to Bumble, we might revert it.

In many previous PRs, we actually minimized the validation of packets or parameters, so that users can send something invalid to controllers or remote devices to test edge cases.

If we are making products for innocent users like Android or BlueZ, that's definitely necessary to protect the stack, but all users of Bumble are (should be?) developers responsible for themselves.

[ibondarenko1]

Totally fair framing on target audience — developers responsible for their own robustness, I get the philosophy.

The specific angle I had in mind was narrower: CI / fuzz-rig hygiene. Bumble is used downstream in automated discovery against arbitrary remote devices (Hermes fuzzer, interop lab rigs, BLE research pipelines). One malicious peer nearby → RecursionError propagates uncatchably out of the SDP task → harness dies. The current fix just swaps it for a plain ValueError at a fixed depth, which a developer-user can catch + resume the fuzz loop. So even under the "no input validation" philosophy, it's a strict improvement for the developer-as-user experience (catchable vs. uncatchable).

If that narrower reading isn't compelling either, happy to close the PR — no strong feelings. Just wanted to put that use case on the table before @barbibulle weighs in.

[barbibulle]

I agree with @zxzxwu 's general statement that trying to make the stack robust against attacks would distract from its main goal. And it would take a lot of effort to actually do that, since there are so many code paths that process external data.
But given the context here, where the change is not for robustness per se, but rather to aid with use as a tool, which is one fo the goals of the project, I think it makes sense to include this change.

[ibondarenko1]

Thanks for the review @barbibulle — appreciate the framing, happy to land this strictly as a tooling-ergonomics fix.

Pushed c55eb15 to fix the black format check the 5 "Check Code" jobs were complaining about (missing blank line after import pytest inside the new regression test). Verified locally against black~=25.1, ruff==0.14.10, isort, mypy==1.12.0, pylint==3.3.1 on tests/sdp_test.py + bumble/sdp.py — all clean.

CI runs are currently queued as action_required (workflow-approval gate on external-contributor commits). When you have a moment, the button on the Actions tab should unblock them. No changes to the fix logic — recursion cap still 32, thread-local counter intact, both regression tests unchanged.

[zxzxwu]

import at top

[ibondarenko1]

Done in 16d0ed5 — moved import pytest to the top of the test module and removed the duplicate inside the test.

[zxzxwu]

Not prefer to use a global threading local, but we can leave it here and refactor later.

[ibondarenko1]

Acknowledged — leaving the module-level threading.local() as-is per your suggestion; happy to refactor it into a context-scoped helper in a follow-up PR if/when you'd like.

[zxzxwu]

core.InvalidPacketError

[ibondarenko1]

Done in 16d0ed5 — switched to core.InvalidPacketError for the depth-limit raise so it matches the rest of the parser's error path.

[ibondarenko1]

Thanks for the review @zxzxwu! Pushed 721e398 addressing the two nits:

• tests/sdp_test.py — removed the duplicate import pytest inside test_nested_sequence_recursion_guard; pytest is already imported at module top (line 23).
• bumble/sdp.py — swapped raise ValueError(...) for raise InvalidPacketError(...) in the depth guard. InvalidPacketError was already imported at line 34 and extends ValueError, so the existing regression-test pytest.raises(ValueError, ...) still matches.

Threading-local counter left as-is per your "leave it here and refactor later" comment.

Local verification: black/ruff/isort clean on both files; pytest tests/sdp_test.py -v -k 'recursion or nested' → 2 passed.

Ready for another look once CI re-runs.

[ibondarenko1]

@zxzxwu friendly ping — addressed all three review nits in 16d0ed5:

• bumble/sdp.py:297 — depth-limit raise now uses core.InvalidPacketError (matches the rest of the parser).
• tests/sdp_test.py — import pytest moved to module top, duplicate inside the test removed.
• bumble/sdp.py:52 — left the module-level threading.local() as-is per your suggestion; can refactor to a context-scoped helper in a follow-up.

CI is green. Ready for another look whenever you have a moment.