Skip to content

fix(ble): reject negative and overlapping offsets in inbound write buffer - #1666

Open
kappaone-eth wants to merge 1 commit into
permissionlesstech:mainfrom
kappaone-eth:fix/ble-inbound-write-buffer-offset-validation
Open

fix(ble): reject negative and overlapping offsets in inbound write buffer#1666
kappaone-eth wants to merge 1 commit into
permissionlesstech:mainfrom
kappaone-eth:fix/ble-inbound-write-buffer-offset-validation

Conversation

@kappaone-eth

Copy link
Copy Markdown

Summary

  • Fixes a remotely triggerable crash: a BLE central can send ATT writes with a negative offset, which flows through BLEInboundWriteBuffer.append(chunks:for:capBytes:) into Data.replaceSubrange and traps at runtime, crashing any BitChat peripheral that receives the write (denial of service).
  • BLEInboundWriteBuffer now validates each chunk before mutating the buffer: rejects offset < 0, non-monotonic sequences, and overlapping chunks, and drops the offending per-central buffer (AppendResult.invalid).
  • The peripheral-role write handler logs and discards malformed writes instead of crashing.

Details

In didReceiveWrite, CBATTRequest.offset is attacker-controlled by the remote central. Requests are sorted ascending, but nothing rejects a negative offset:

combined.replaceSubrange(chunk.offset..<end, with: chunk.data)

With chunk.offset < 0, the range chunk.offset..<end is invalid (and if end lands below the buffer start, replaceSubrange also traps), so a single crafted write crashes the app. Duplicate or overlapping offsets additionally let a sender silently rewrite previously buffered bytes.

The fix validates each non-empty chunk against a running lastEnd cursor before touching the buffer. On violation it clears the per-central buffer and returns a new .invalid(metadata:) result; the caller logs the offending offsets and keeps operating. Legitimate CoreBluetooth long writes (ascending, non-overlapping offsets, including zero-padded gaps) are unaffected.

Tests

Added to BLEInboundWriteBufferTests:

  • negative offset is rejected and the cleared buffer still decodes a later clean frame
  • overlapping chunks are rejected
  • non-monotonic chunks are rejected

Note: tests were authored on a machine without an Xcode toolchain, so they have not been executed locally — relying on CI.

Supersedes #1665 (same change; branch history was rewritten for authorship).

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a46c898e59

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread bitchat/Services/BLE/BLEInboundWriteBuffer.swift Outdated
…ffer

A malicious BLE central can send ATT writes with a negative offset, which
reaches Data.replaceSubrange via BLEInboundWriteBuffer.append and traps at
runtime, crashing any peripheral that receives the write.

Validate each chunk before mutating the buffer: reject negative offsets,
non-monotonic sequences, and overlapping chunks, and drop the offending
per-central buffer. Add an AppendResult.invalid case, handle it in the
peripheral-role write handler, and cover the rejection paths with tests.
@kappaone-eth
kappaone-eth force-pushed the fix/ble-inbound-write-buffer-offset-validation branch from a46c898 to fd131a9 Compare August 12, 2026 14:48

@Chessing234 Chessing234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the overlap/rewrite hardening is worth having, but two things first.

the negative offset: CBATTRequest.offset comes straight off the ATT wire and that field is 16-bit unsigned, so i don't think a central can produce offset < 0 at all. can you show a repro or drop the DoS framing?

bigger one: our central role writes with .withoutResponse (BLEService.swift:5073, 5126, 5167), so legit inbound writes land at offset 0. with a stale partial buffer pending, the next offset-0 write now trips chunk.offset >= lastEnd, goes .invalid, and its bytes are dropped — main applied them in place. can you treat offset 0 with a non-empty buffer as a restart (clear, then apply) instead of a reject?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants