Skip to content

Add firmware settings validation reference#111

Open
thebentern wants to merge 1 commit into
masterfrom
audits-firmware
Open

Add firmware settings validation reference#111
thebentern wants to merge 1 commit into
masterfrom
audits-firmware

Conversation

@thebentern
Copy link
Copy Markdown
Contributor

Add a comprehensive firmware-side settings validation document at standards/audits/data/settings-validation-firmware.md describing defaults, per-field validation rules, module/channel behaviors, role-based overrides, authorization/session passkey rules, and inbound rate limits. Also add a .gitignore entry to ignore .DS_Store. This documents the device-side validation and defaulting logic to complement client-side references and aid audits and developer understanding.

Add a comprehensive firmware-side settings validation document at standards/audits/data/settings-validation-firmware.md describing defaults, per-field validation rules, module/channel behaviors, role-based overrides, authorization/session passkey rules, and inbound rate limits. Also add a .gitignore entry to ignore .DS_Store. This documents the device-side validation and defaulting logic to complement client-side references and aid audits and developer understanding.
@thebentern thebentern requested review from garthvh and jamesarich May 22, 2026 21:18
Copy link
Copy Markdown
Member

@garthvh garthvh left a comment

Choose a reason for hiding this comment

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

Good job clanker

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a firmware-side settings validation/reference document to complement the existing Android/iOS settings validation docs, improving auditability and developer understanding of which defaults/validation are applied on-device.

Changes:

  • Add settings-validation-firmware.md documenting firmware defaults, validation behaviors, authorization/session-passkey rules, and inbound rate limits.
  • Add .DS_Store to .gitignore.

Reviewed changes

Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.

File Description
standards/audits/data/settings-validation-firmware.md New comprehensive reference for firmware defaulting/validation and related admin/security behaviors.
.gitignore Ignores macOS Finder metadata files (.DS_Store).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +263 to +264
| `username` | `"meshdev"` | **none** | |
| `password` | `"large4cats"` | **none** | nanopb limit 32 bytes. |
Comment on lines +435 to +436
| `team` | `Unspecifed_Color` (firmware renders Cyan) | **none** (no-op) | `handleSetModuleConfig()` has **no `case` for `tak`** — a `set_module_config` carrying the `tak` variant is silently a no-op (nothing stored, no error returned). TAK config cannot be set via `set_module_config` in this firmware revision. |
| `role` | `Unspecifed` (firmware renders TeamMember) | **none** (no-op) | Same. |
Copy link
Copy Markdown
Collaborator

@jamesarich jamesarich left a comment

Choose a reason for hiding this comment

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

Thorough and well-sourced document — cross-checked against the cited firmware commit 91f930d5c. One material bug in the NeighborInfo section (dead-code clamp documented as working), plus a few precision improvements for the session passkey section. Everything else verified correct against source.

| Field | Default | Validation | Notes |
|-------|---------|------------|-------|
| `enabled` | `false` | **none** | |
| `update_interval` | `0` | **clamp** | If below `14400` (`min_neighbor_info_broadcast_secs`) it is reset to `21600` (`default_neighbor_info_broadcast_secs`). |
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Bug: This clamp is dead code in the firmware

The AdminModule.cpp handler (lines 1013–1020) checks and clamps the old stored value, then immediately overwrites the entire struct with the inbound payload:

case meshtastic_ModuleConfig_neighbor_info_tag:
    moduleConfig.has_neighbor_info = true;
    if (moduleConfig.neighbor_info.update_interval < min_neighbor_info_broadcast_secs) {
        moduleConfig.neighbor_info.update_interval = default_neighbor_info_broadcast_secs;
    }
    moduleConfig.neighbor_info = c.payload_variant.neighbor_info; // ← overwrites the clamped value
    break;

The guard never takes effect — any inbound update_interval (including values below 14400) is stored verbatim.

Suggested change
| `update_interval` | `0` | **clamp** | If below `14400` (`min_neighbor_info_broadcast_secs`) it is reset to `21600` (`default_neighbor_info_broadcast_secs`). |
| `update_interval` | `0` | **none** (intended clamp is dead code) | Firmware checks the *previous* stored value then overwrites with the inbound struct — the intended floor of `14400` (`min_neighbor_info_broadcast_secs`) never takes effect. See `AdminModule.cpp` lines 1016–1020. |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@thebentern - this is the call-out i was most concerned over


**Session passkey** — a state-changing message from a *remote* node must echo
back an 8-byte `session_passkey` issued on a prior `get_*` response. It is valid
for 300 s; a mismatch or timeout ⇒ `ADMIN_BAD_SESSION_KEY`. Locally-connected
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Two precision improvements here:

  1. The passkey check condition is mp.from != 0 && !messageIsRequest(r) && !messageIsResponse(r) — so remote get_* requests (classified as "requests" by messageIsRequest()) also skip the check, not just locally-connected clients.

  2. The passkey is included in every admin get_* response (owner, config, module_config, channel, metadata, connection_status, remote_hardware_pins, plus module API responses), not just a single prior response. It regenerates only if 150+ seconds have elapsed since last generation, creating an overlapping validity window (regenerates at 150s, valid until 300s).

Suggested change
for 300 s; a mismatch or timeout ⇒ `ADMIN_BAD_SESSION_KEY`. Locally-connected
for 300 s; a mismatch or timeout ⇒ `ADMIN_BAD_SESSION_KEY`. Locally-connected
clients skip this check, as do remote `get_*` requests (only non-request,
non-response admin messages are gated). The passkey is regenerated after 150 s
but remains valid for 300 s, creating an overlapping validity window.


| Field | Default | Validation | Notes |
|-------|---------|------------|-------|
| `team` | `Unspecifed_Color` (firmware renders Cyan) | **none** (no-op) | `handleSetModuleConfig()` has **no `case` for `tak`** — a `set_module_config` carrying the `tak` variant is silently a no-op (nothing stored, no error returned). TAK config cannot be set via `set_module_config` in this firmware revision. |
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Confirmed accurate — no meshtastic_ModuleConfig_tak_tag case exists in the switch. Worth noting that get_module_config_request for TAK does respond correctly (the response switch at line 1236 is missing it too actually — let me double-check... actually the response handler has no TAK_CONFIG case either in the get_module_config_request handler). So TAK config is fully inert in both directions via AdminModule. Good as documented.

Minor nit: Consider noting that the ModuleConfigType enum does define TAK_CONFIG = 15 in the proto, so clients can send the request — it just does nothing firmware-side.

| `index` | `0` | **reject** | Must be `0 ≤ index < 8`; otherwise `BAD_REQUEST`. `get_channel_request` is rejected the same way. |
| `role` | `PRIMARY` | **sanitize** | Setting a channel to `PRIMARY` demotes every other `PRIMARY` channel to `SECONDARY`. |
| `name` | `""` | **none** | nanopb limit 12 bytes. Empty ⇒ rendered as the modem-preset name. |
| `psk` | `{0x01}` (default-key shorthand) | **none** | Not validated at write time. `getKey()` later interprets it: `0` = off, `1`–`10` = built-in keys, 16 / 32 bytes = AES-128 / AES-256. A short key (2–15 or 17–31 bytes) is silently **zero-padded** to 16 / 32 — not rejected. |
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Accurate but could note the timing distinction for SDK implementors:

Suggested change
| `psk` | `{0x01}` (default-key shorthand) | **none** | Not validated at write time. `getKey()` later interprets it: `0` = off, `1``10` = built-in keys, 16 / 32 bytes = AES-128 / AES-256. A short key (2–15 or 17–31 bytes) is silently **zero-padded** to 16 / 32 — not rejected. |
| `psk` | `{0x01}` (default-key shorthand) | **none** | Not validated at write time; stored verbatim. `getKey()` at *read time* interprets it: `0` = off, `1``10` = built-in keys, 16 / 32 bytes = AES-128 / AES-256. A short key (2–15 or 17–31 bytes) is silently **zero-padded** to 16 / 32 at use time — not rejected or transformed on write. |

@jamesarich
Copy link
Copy Markdown
Collaborator

Clanker came up with a couple of potential corrections/nits


| Field | Default | Validation | Notes |
|-------|---------|------------|-------|
| `public_key` | empty | **sanitize** | Regenerated from the private key when a valid region is set. |
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why sanitize public key? It's PUBLIC

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.

4 participants