Skip to content

SimplrSSH - Bug Hunter - 2026-10-18 - #28

Merged
bluenevus merged 1 commit into
integrationfrom
bug-hunt/20261018-0908
Oct 18, 2026
Merged

bluenevus merged 1 commit into
integrationfrom
bug-hunt/20261018-0908

Conversation

@bluenevus

Copy link
Copy Markdown
Contributor

Findings

  1. Backup restore accepts u32 values above 4294967295 — main/settings_backup.cpp:304-308 (parse_settings, type u). Root cause: the check allowed any ≤10-digit decimal string, but 10 digits can exceed UINT32_MAX; strtoul then clamped (32-bit) or truncated (64-bit) the value before nvs_set_u32. Impact: Low/Medium. A corrupt or tampered SD backup is accepted as valid, with a silently altered setting, where the rule is that the whole backup is validated before anything changes.

Reviewed with no defect found: vterm.cpp, tab5_keymap.cpp, command_redact.cpp, ui_page.cpp, known_hosts.cpp, and the URL/entity parsing in portal.cpp.

Plan

Reject u records above UINT32_MAX in parse_settings. Add a regression case to restore_rejects_untrusted_backups. Verify with the host tests.

Final gap analysis

  • Fix: done.
  • Regression test: done. It fails without the fix (restore succeeded and wiped the network) and passes with it.

Deferred (not fixed here)

  • ssh / sshkey commands split on single spaces, so a double space shifts the arguments and gives port 0. Port range validation in ssh_begin is already on origin/code-scan/20261018-0000 (b411112), so I left it to avoid a conflict.
  • connect "SSID" "" (open network) fails with a usage message, because an empty quoted password is dropped. Needs a product decision.
  • hosts forget prints "Forgot ..." even if nvs_erase_key fails (known_hosts.cpp). Not host-testable.
  • Profile ports loaded from NVS use atoi with no range check. Covered by the connect-time check above.

Verification

  • scripts/check.sh with SKIP_SAST=1: guard tests passed (97 OK). Host unit and persistence tests pass: unit_tests 114/114, persistence_tests 24/24.
  • SAST not run locally (it needs docker and ECR login). CI's security scan still runs it.
  • Firmware build and idf.py size not run (no ESP-IDF here). The change is host-only, parser logic with no RAM impact. Hardware behavior is unconfirmed on a real T-Deck or Tab5.

Generated by Claude Code

A 10-digit 'u' record passed the digit-count check but exceeds UINT32_MAX;
strtoul then clamped or truncated it before nvs_set_u32.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KhZ1xbmvwNKARGdpyRmryh
@bluenevus
bluenevus merged commit 3464d18 into integration Oct 18, 2026
29 checks passed
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