From 1e01488d413fcb1cbf0520ff7cbb0611a64166e1 Mon Sep 17 00:00:00 2001 From: Marcel Verdult Date: Thu, 12 Nov 2026 13:06:50 +0100 Subject: [PATCH] fix(companion): state the minimum frame length of 14 more commands handleCmdFrame() derives every field offset from the command code. 35 of its branches test the length they need, 14 do not, or test cmd_frame[1] before the length. Neither cmd_frame nor the serial interface's receive buffer is cleared between frames, and checkRecvFrame() copies only the received bytes, so everything above len is the tail of an earlier frame. A short frame therefore acts on the previous command's bytes. No read leaves cmd_frame (the largest unconditional offset is 135, the buffer holds MAX_FRAME_SIZE + 1 bytes); the damage is acting on stale data. Twelve branches gain "&& len >= N", so a short frame falls through to the existing else and is answered like the guarded ones (ERR_CODE_UNSUPPORTED_CMD). CMD_SET_PATH_HASH_MODE and CMD_SEND_PATH_DISCOVERY_REQ only have the length test moved in front of the cmd_frame[1] test. CMD_SEND_CHANNEL_DATA gains a second check for the 2 byte data_type behind the path, whose offset is known only after path_len is parsed, like CMD_SEND_RAW_DATA. CMD_SEND_CHANNEL_DATA 3 + path + 2 CMD_ADD_UPDATE_CONTACT 136 (was 36); only lat, lon and lastmod are optional CMD_REMOVE_CONTACT 33 CMD_SHARE_CONTACT 33 CMD_GET_CONTACT_BY_KEY 33 CMD_SET_RADIO_PARAMS 11 (repeat flag stays optional) CMD_SET_RADIO_TX_POWER 2 CMD_SET_TUNING_PARAMS 9 CMD_SET_OTHER_PARAMS 2 (later fields stay optional) CMD_SET_PATH_HASH_MODE 3 CMD_REBOOT 7 CMD_FACTORY_RESET 6 CMD_SET_AUTOADD_CONFIG 2 (max hops stays optional) CMD_SEND_PATH_DISCOVERY_REQ 34 CMD_ADD_UPDATE_CONTACT is the one existing guard that changes: it asked for 36 bytes while updateContactFromFrame() reads type, flags, out_path_len, 64 bytes of path, 32 bytes of name and last_advert_timestamp without testing len. The response frame (writeContactRespFrame) writes the same fields in the same order. Measured on a T-Echo: after a long CLI frame, a short SET_TUNING_PARAMS, SET_RADIO_TX_POWER, SET_OTHER_PARAMS or SET_AUTOADD_CONFIG frame changed the setting to the stale bytes, a 1 byte GET_CONTACT_BY_KEY returned a full record, a 36 byte ADD_UPDATE_CONTACT created a contact from 100 bytes of the earlier frame, and a short REBOOT after a "reboot" CLI frame rebooted the node. --- examples/companion_radio/MyMesh.cpp | 35 ++++++++++++++++++----------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/examples/companion_radio/MyMesh.cpp b/examples/companion_radio/MyMesh.cpp index 1a1bcbe767..94b1d049a8 100644 --- a/examples/companion_radio/MyMesh.cpp +++ b/examples/companion_radio/MyMesh.cpp @@ -1221,6 +1221,12 @@ void MyMesh::handleCmdFrame(size_t len) { i += mesh::Packet::writePath(path, &cmd_frame[i], path_len); } + if ((size_t)(i + 2) > len) { // the 2 byte data_type must be inside the frame + MESH_DEBUG_PRINTLN("CMD_SEND_CHANNEL_DATA frame too short for data_type: %d", (uint32_t)len); + writeErrFrame(ERR_CODE_ILLEGAL_ARG); + return; + } + uint16_t data_type = ((uint16_t)cmd_frame[i]) | (((uint16_t)cmd_frame[i + 1]) << 8); i += 2; const uint8_t *payload = &cmd_frame[i]; @@ -1329,7 +1335,10 @@ void MyMesh::handleCmdFrame(size_t len) { } else { writeErrFrame(ERR_CODE_NOT_FOUND); // unknown contact } - } else if (cmd_frame[0] == CMD_ADD_UPDATE_CONTACT && len >= 1 + 32 + 2 + 1) { + } else if (cmd_frame[0] == CMD_ADD_UPDATE_CONTACT && len >= 1 + PUB_KEY_SIZE + 3 + MAX_PATH_SIZE + 32 + 4) { + // updateContactFromFrame() reads every field up to last_advert_timestamp without + // testing len; only the trailing lat/lon and lastmod are optional. The reverse + // direction (writeContactRespFrame) writes the same fields in the same order. uint8_t *pub_key = &cmd_frame[1]; ContactInfo *recipient = lookupContactByPubKey(pub_key, PUB_KEY_SIZE); uint32_t last_mod = getRTCClock()->getCurrentTime(); // fallback value if not present in cmd_frame @@ -1350,7 +1359,7 @@ void MyMesh::handleCmdFrame(size_t len) { writeErrFrame(ERR_CODE_TABLE_FULL); } } - } else if (cmd_frame[0] == CMD_REMOVE_CONTACT) { + } else if (cmd_frame[0] == CMD_REMOVE_CONTACT && len >= 1 + PUB_KEY_SIZE) { uint8_t *pub_key = &cmd_frame[1]; ContactInfo *recipient = lookupContactByPubKey(pub_key, PUB_KEY_SIZE); if (recipient && removeContact(*recipient)) { @@ -1360,7 +1369,7 @@ void MyMesh::handleCmdFrame(size_t len) { } else { writeErrFrame(ERR_CODE_NOT_FOUND); // not found, or unable to remove } - } else if (cmd_frame[0] == CMD_SHARE_CONTACT) { + } else if (cmd_frame[0] == CMD_SHARE_CONTACT && len >= 1 + PUB_KEY_SIZE) { uint8_t *pub_key = &cmd_frame[1]; ContactInfo *recipient = lookupContactByPubKey(pub_key, PUB_KEY_SIZE); if (recipient) { @@ -1372,7 +1381,7 @@ void MyMesh::handleCmdFrame(size_t len) { } else { writeErrFrame(ERR_CODE_NOT_FOUND); } - } else if (cmd_frame[0] == CMD_GET_CONTACT_BY_KEY) { + } else if (cmd_frame[0] == CMD_GET_CONTACT_BY_KEY && len >= 1 + PUB_KEY_SIZE) { uint8_t *pub_key = &cmd_frame[1]; ContactInfo *contact = lookupContactByPubKey(pub_key, PUB_KEY_SIZE); if (contact) { @@ -1425,7 +1434,7 @@ void MyMesh::handleCmdFrame(size_t len) { out_frame[0] = RESP_CODE_NO_MORE_MESSAGES; _serial->writeFrame(out_frame, 1); } - } else if (cmd_frame[0] == CMD_SET_RADIO_PARAMS) { + } else if (cmd_frame[0] == CMD_SET_RADIO_PARAMS && len >= 11) { // freq, bw, sf, cr; the repeat flag stays optional int i = 1; uint32_t freq; memcpy(&freq, &cmd_frame[i], 4); @@ -1461,7 +1470,7 @@ void MyMesh::handleCmdFrame(size_t len) { (uint32_t)cr); writeErrFrame(ERR_CODE_ILLEGAL_ARG); } - } else if (cmd_frame[0] == CMD_SET_RADIO_TX_POWER) { + } else if (cmd_frame[0] == CMD_SET_RADIO_TX_POWER && len >= 2) { int8_t power = (int8_t)cmd_frame[1]; if (power < -9 || power > MAX_LORA_TX_POWER) { writeErrFrame(ERR_CODE_ILLEGAL_ARG); @@ -1471,7 +1480,7 @@ void MyMesh::handleCmdFrame(size_t len) { radio_driver.setTxPower(_prefs.tx_power_dbm); writeOKFrame(); } - } else if (cmd_frame[0] == CMD_SET_TUNING_PARAMS) { + } else if (cmd_frame[0] == CMD_SET_TUNING_PARAMS && len >= 9) { // two 32 bit values, neither range checked int i = 1; uint32_t rx, af; memcpy(&rx, &cmd_frame[i], 4); @@ -1489,7 +1498,7 @@ void MyMesh::handleCmdFrame(size_t len) { memcpy(&out_frame[i], &rx, 4); i += 4; memcpy(&out_frame[i], &af, 4); i += 4; _serial->writeFrame(out_frame, i); - } else if (cmd_frame[0] == CMD_SET_OTHER_PARAMS) { + } else if (cmd_frame[0] == CMD_SET_OTHER_PARAMS && len >= 2) { // the fields after manual_add stay optional _prefs.manual_add_contacts = cmd_frame[1]; if (len >= 3) { _prefs.telemetry_mode_base = cmd_frame[2] & 0x03; // v5+ @@ -1505,7 +1514,7 @@ void MyMesh::handleCmdFrame(size_t len) { } savePrefs(); writeOKFrame(); - } else if (cmd_frame[0] == CMD_SET_PATH_HASH_MODE && cmd_frame[1] == 0 && len >= 3) { + } else if (cmd_frame[0] == CMD_SET_PATH_HASH_MODE && len >= 3 && cmd_frame[1] == 0) { if (cmd_frame[2] >= 3) { writeErrFrame(ERR_CODE_ILLEGAL_ARG); } else { @@ -1513,7 +1522,7 @@ void MyMesh::handleCmdFrame(size_t len) { savePrefs(); writeOKFrame(); } - } else if (cmd_frame[0] == CMD_REBOOT && memcmp(&cmd_frame[1], "reboot", 6) == 0) { + } else if (cmd_frame[0] == CMD_REBOOT && len >= 7 && memcmp(&cmd_frame[1], "reboot", 6) == 0) { if (dirty_contacts_expiry) { // is there are pending dirty contacts write needed? saveContacts(); } @@ -1652,7 +1661,7 @@ void MyMesh::handleCmdFrame(size_t len) { } else { writeErrFrame(ERR_CODE_NOT_FOUND); // contact not found } - } else if (cmd_frame[0] == CMD_SEND_PATH_DISCOVERY_REQ && cmd_frame[1] == 0 && len >= 2 + PUB_KEY_SIZE) { + } else if (cmd_frame[0] == CMD_SEND_PATH_DISCOVERY_REQ && len >= 2 + PUB_KEY_SIZE && cmd_frame[1] == 0) { uint8_t *pub_key = &cmd_frame[2]; ContactInfo *recipient = lookupContactByPubKey(pub_key, PUB_KEY_SIZE); if (recipient) { @@ -1966,7 +1975,7 @@ void MyMesh::handleCmdFrame(size_t len) { } else { writeErrFrame(ERR_CODE_ILLEGAL_ARG); // invalid stats sub-type } - } else if (cmd_frame[0] == CMD_FACTORY_RESET && memcmp(&cmd_frame[1], "reset", 5) == 0) { + } else if (cmd_frame[0] == CMD_FACTORY_RESET && len >= 6 && memcmp(&cmd_frame[1], "reset", 5) == 0) { if (_serial) { MESH_DEBUG_PRINTLN("Factory reset: disabling serial interface to prevent reconnects (BLE/WiFi)"); _serial->disable(); // Phone app disconnects before we can send OK frame so it's safe here @@ -2024,7 +2033,7 @@ void MyMesh::handleCmdFrame(size_t len) { } else { writeErrFrame(ERR_CODE_TABLE_FULL); } - } else if (cmd_frame[0] == CMD_SET_AUTOADD_CONFIG) { + } else if (cmd_frame[0] == CMD_SET_AUTOADD_CONFIG && len >= 2) { // autoadd_max_hops stays optional _prefs.autoadd_config = cmd_frame[1]; if (len >= 3) { _prefs.autoadd_max_hops = min(cmd_frame[2], (uint8_t)64);