Skip to content

Security: validate BLE-supplied filenames before any SD access - #34

Merged
TheAngryRaven merged 1 commit into
masterfrom
feat/ble-filename-sanitization
May 21, 2026
Merged

Security: validate BLE-supplied filenames before any SD access#34
TheAngryRaven merged 1 commit into
masterfrom
feat/ble-filename-sanitization

Conversation

@TheAngryRaven

Copy link
Copy Markdown
Owner

PR #3 on the path to 8/10. Closes the path-traversal / FAT-unsafe filename gap flagged in the code review.

The problem

BLE delivered raw command bytes straight into the filesystem with no sanitization:

  • GET:<name>SD.open(name, FILE_READ) at root
  • DELETE:<name>SD.remove(name) at root
  • TGET: / TPUT: / TDEL:snprintf(path, "/TRACKS/%s", name)

So anyone in BLE range (the service is SECMODE_OPEN) could:

  • read/delete files outside the intended scope with an absolute path or ../
  • escape /TRACKS/ via TPUT:../FOO/TRACKS/../FOO/FOO on FAT
  • wedge the card with reserved characters (:, *, wildcards, control bytes)

The fix

New BirdsEye/filename_validator.{h,cpp} — a pure, host-testable function:

bool isValidFilename(const char* name, size_t max_len);

A name is valid iff all of:

  • non-null, non-empty
  • length ≤ max_len (production limit kMaxBleFilenameLen = 24, which fits the smallest on-device filename buffer — trackUploadFilename[25])
  • first char is not . (rejects ., .., and dotfiles)
  • every char is in the FAT-safe set [A-Za-z0-9._-]

Because / (and \) are rejected outright, no path components can form, so ../ traversal is structurally impossible — not just pattern-matched away.

Wired into all five filename-bearing BLE commands in bluetooth.ino, each validating before any SD call:

Command Reject response Rationale
GET: ERROR matches existing transfer-failure code
DELETE: NOT_FOUND matches existing missing-file code; doesn't advertise that traversal was detected
TGET: / TPUT: / TDEL: TERR:BAD_NAME track commands already use the TERR: prefix convention

Tests

13 new host cases (built on the PR #2 doctest harness), 85 total, all green:

  • real DOVEX log names (20240115_1430.dovex) and track names (OKC.json) accepted
  • length boundary — exactly max_len passes, max_len+1 fails; caller-supplied smaller limits respected
  • leading-dot rejection (., .., .hidden)
  • traversal rejection (../secret, ../../etc/passwd)
  • separator rejection (/absolute, sub/dir, back\slash)
  • Windows-reserved + wildcard rejection (:, *, ?, |, ", <, >)
  • control-byte, high-bit-byte, and DEL rejection
  • spaces rejected
  • embedded (non-leading) dots accepted (v1.0.dovex, foo..bar)
cmake -S tests -B tests/build && cmake --build tests/build
ctest --test-dir tests/build --output-on-failure

Scope notes

  • Pin Lock is deferred per the earlier decision — it needs coordinated companion-app work and is its own effort. This PR is purely about not trusting the filename string.
  • Behavior for valid filenames is unchanged — this only adds rejections for inputs that were never legitimate.
  • No firmware-visible behavior change beyond the new rejection paths, so the compile-sketch job is the regression net for the build; the BLE reject paths are covered by the unit tests.

Where we are

# Branch Status
1 refactor/drop-legacy-module-headers ✅ merged (#32)
2 tests/host-doctest-harness ✅ merged (#33)
3 feat/ble-filename-sanitization ← this PR
4 ci/clang-tidy-and-size-budget next
5 release/semver-and-uf2-artifacts
6 docs/contributor-setup final

🤖 Generated with Claude Code


Generated by Claude Code

Closes the path-traversal / FAT-unsafe-filename gap from the code
review. BLE delivered raw command bytes straight into SD.open() /
SD.remove() / '/TRACKS/%s' path building, so anyone in radio range
could read or delete files outside the intended scope (../, absolute
paths) or wedge FAT with reserved characters.

New BirdsEye/filename_validator.{h,cpp}: pure isValidFilename(name,
max_len) — non-empty, <= 24 chars (fits the smallest on-device
filename buffer), no leading dot (kills '.', '..', dotfiles), and a
strict [A-Za-z0-9._-] allow-list (kills '/', '\', ':', wildcards,
spaces, control + high-bit bytes). No slashes means path components
can't form, so '../' traversal is impossible.

Wired into all five filename-bearing BLE commands in bluetooth.ino —
GET, DELETE, TGET, TPUT, TDEL — each rejecting before it touches the
card. GET/DELETE reply ERROR / NOT_FOUND (consistent with existing
failure codes, and doesn't advertise that traversal was detected);
track commands reply TERR:BAD_NAME.

13 host test cases (now 85 total, all green): real DOVEX/track names
accepted, length boundary at max_len, leading-dot + traversal +
slash rejection, Windows-reserved + wildcard rejection, control and
high-bit byte rejection.

Pin Lock deferred per earlier decision — needs coordinated phone-app
work.

Defers nothing else; behavior for valid filenames is unchanged.
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