CommonCLI bounds replies at 160 bytes, but every caller gives it at most 158 #25

CommonCLI bounds several of its replies at 160 bytes. Every caller can only give it 157, because the caller first echoes an optional 3-byte xx| prefix into the reply buffer and passes the rest on. An admin command with a long argument therefore writes 1 to 3 bytes past the caller's stack buffer, over serial or over remote admin.

References:

• The code: upstream dev 2dbd463e.
• The measurement: a host transcription of the writers and the wrapper, with each caller buffer in a sentinel-filled arena. Its inputs are constructed (see "Evidence").

The buffers

• Serial: char reply[160], in simple_repeater/main.cpp L152, simple_room_server/main.cpp L130 and simple_sensor/main.cpp L147.
• Remote admin: &temp[5] of uint8_t temp[166], which leaves 161 bytes. See simple_repeater/MyMesh.cpp L733-L739.
• The prefix: each wrapper copies xx| into the reply and does reply += 3 before it calls _cli.handleCommand(). See simple_repeater/MyMesh.cpp L1235-L1239, simple_room_server/MyMesh.cpp L937-L941 and simple_sensor/SensorMesh.cpp L453-L457.

So CommonCLI gets 157 bytes from a serial caller and 158 from a remote one.

The writers that assume 160

All of these are in src/helpers/CommonCLI.cpp:

• L697: set with an unknown key echoes the key, StrHelper::strncpy(&reply[16], config, 160-17).
• L872-L881 and L897: the region def errors, snprintf(reply, 160, ...). The put-failed and unknown-jump errors echo the name the admin typed.
• L903, L910 and L1020: the region map exports, exportTo(reply, 160) and exportNamesTo(reply, 160, ...). These grow with the configured regions.
• L376-L380: gps diag, which bounds formatDiagnostics() at 160 - prefix_len.
• L272 (password), L294 (power) and L735 (get owner.info). Their content is too short to reach the bound today (a 16-byte password, a short status line, a 120-byte owner_info), but they state the same wrong limit.

Evidence

Measured with the transcription, for the two writers a command's own text can drive, and get owner.info as a control. Each command is at most 158 characters, the most simple_repeater/main.cpp's command[160] keeps:

The overrun is past a stack array, so what it overwrites depends on the compiler's frame layout.

Impact

Low to moderate. It needs an admin login (or the serial console), and it writes at most 3 bytes. The only content is the admin's own text, plus the terminating NUL. But it is a stack overrun reachable from the mesh by a logged-in admin.

Proposed fix

Define the size once in CommonCLI.h, CLI_REPLY_SIZE = 157 (the smaller of the two callers, with the prefix), with the derivation in a comment, and use it for every bound above. A reply without the prefix then loses at most its last 3 characters.

Not changed: the sensor list and neighbours loops, which stop adding entries at a fixed 134 bytes. Upstream PR meshcore-dev#3433 left the sensor list bound as it is, and the neighbours loop is not audited here.

ZephCore fixed the same overrun in liquidraver/ZephCore@ad23dcb. It passes the prefix length to its CLI and subtracts it from its remote bound, which is separate from its local one. MeshCore has a single bound, so one smaller constant covers both paths.