From bd1e022e889bd9b78d0e1125fb6524bcc1b75d7f Mon Sep 17 00:00:00 2001 From: liquidraver <504870+liquidraver@users.noreply.github.com> Date: Wed, 20 May 2026 11:52:39 +0200 Subject: [PATCH] fix(security): close OOB read in path-decoding callers (BLE + LoRa-anon) Both mesh::Packet::writePath and ::copyPath did a raw memcpy of the decoded hash_count*hash_size bytes from src to dest with no bound on src. Two call sites used phone-supplied or LoRa-anon-supplied buffers where the path_len byte was attacker-controlled: - CompanionMesh CMD_SEND_CHANNEL_DATA accepted len>=4 and called writePath with no src bound; a paired phone could leak up to ~65 bytes of syswq stack into the outgoing LoRa channel-data frame. - RepeaterMesh handleAnonRegionsReq / handleAnonOwnerReq / handleAnonClockReq read reply_path_len from an unauthenticated LoRa anon-request payload and called copyPath without any src bound. Any LoRa neighbor could leak repeater stack into the reply path. Hardened the API: both functions now require an explicit src_len and reject (return 0) when the decoded byte count exceeds it. Updated all 14 call sites across Packet/Mesh/Dispatcher/BaseChatMesh/ CompanionMesh/RepeaterMesh. Trusted callers (internal MAX_PATH_SIZE buffers) pass MAX_PATH_SIZE; untrusted callers pass real remaining length. Added len-5 plumbing through the anon-handler signatures. CMD_SEND_CHANNEL_DATA also gained a local len>=5 + path_bytes sanity check for early rejection. --- zephcore/app/CompanionMesh.cpp | 29 +++++++++++++++++++++++++---- zephcore/app/RepeaterMesh.cpp | 31 +++++++++++++++++++++---------- zephcore/app/RepeaterMesh.h | 6 +++--- zephcore/helpers/BaseChatMesh.cpp | 5 ++++- zephcore/include/mesh/Packet.h | 13 +++++++++++-- zephcore/src/Dispatcher.cpp | 3 ++- zephcore/src/Mesh.cpp | 15 +++++++++++---- zephcore/src/Packet.cpp | 14 +++++++++----- 8 files changed, 86 insertions(+), 30 deletions(-) diff --git a/zephcore/app/CompanionMesh.cpp b/zephcore/app/CompanionMesh.cpp index e3945ab..bc3cf87 100644 --- a/zephcore/app/CompanionMesh.cpp +++ b/zephcore/app/CompanionMesh.cpp @@ -630,7 +630,9 @@ void CompanionMesh::onDiscoveredContact(ContactInfo &contact, bool is_new, uint8 memcpy(ap->pubkey_prefix, contact.id.pub_key, 7); memcpy(ap->name, contact.name, sizeof(ap->name)); ap->recv_timestamp = (uint32_t)getRTCClock()->getCurrentTime(); - ap->path_len = mesh::Packet::copyPath(ap->path, path, path_len); + /* path source is from inbound advert; upstream parser bounds it within + * the packet payload. AdvertPath::path is MAX_PATH_SIZE-sized. */ + ap->path_len = mesh::Packet::copyPath(ap->path, path, MAX_PATH_SIZE, path_len); _next_advert_path_idx = (_next_advert_path_idx + 1) % ADVERT_PATH_TABLE_SIZE; } @@ -2597,7 +2599,8 @@ bool CompanionMesh::handleProtocolFrame(const uint8_t *data, size_t len) rsp[i++] = PACKET_ADVERT_PATH; put_le32(&rsp[i], ap->recv_timestamp); i += 4; rsp[i++] = ap->path_len; - i += mesh::Packet::writePath(&rsp[i], ap->path, ap->path_len); + /* Trusted source: AdvertPath::path is MAX_PATH_SIZE-sized. */ + i += mesh::Packet::writePath(&rsp[i], ap->path, MAX_PATH_SIZE, ap->path_len); writeFrame(rsp, i); } else { sendPacketError(ERR_NOT_FOUND); @@ -2796,7 +2799,9 @@ bool CompanionMesh::handleProtocolFrame(const uint8_t *data, size_t len) return true; case CMD_SEND_CHANNEL_DATA: { - if (len < 4) { + /* Minimum frame: cmd(1) + channel_idx(1) + path_len(1) + path(0..) + + * data_type(2) + payload(0..) = 5 bytes when path is empty. */ + if (len < 5) { sendPacketError(ERR_ILLEGAL_ARG); return true; } @@ -2810,9 +2815,25 @@ bool CompanionMesh::handleProtocolFrame(const uint8_t *data, size_t len) return true; } + /* Compute decoded path byte count and ensure source frame has room + * for path + data_type. (path_len is the 6-bit hash_count + 2-bit + * hash_size encoding; writePath itself will reject if src_len is + * too small, but failing here also rejects truncated data_type.) */ + uint8_t hash_count = path_len & 63; + uint8_t hash_size = (path_len >> 6) + 1; + size_t path_bytes = (path_len == OUT_PATH_UNKNOWN) ? 0 + : (size_t)hash_count * hash_size; + if ((size_t)i + path_bytes + 2 > len) { + LOG_WRN("CMD_SEND_CHANNEL_DATA short frame: len=%u need >=%u", + (unsigned)len, (unsigned)(i + path_bytes + 2)); + sendPacketError(ERR_ILLEGAL_ARG); + return true; + } + uint8_t path[MAX_PATH_SIZE]; if (path_len != OUT_PATH_UNKNOWN) { - i += mesh::Packet::writePath(path, &data[i], path_len); + /* src_len = remaining bytes from data[i] onward. */ + i += mesh::Packet::writePath(path, &data[i], len - i, path_len); } uint16_t data_type = ((uint16_t)data[i]) | (((uint16_t)data[i + 1]) << 8); diff --git a/zephcore/app/RepeaterMesh.cpp b/zephcore/app/RepeaterMesh.cpp index 0aeb177..b8cb3e3 100644 --- a/zephcore/app/RepeaterMesh.cpp +++ b/zephcore/app/RepeaterMesh.cpp @@ -173,10 +173,13 @@ uint8_t RepeaterMesh::handleLoginReq(const mesh::Identity& sender, const uint8_t return 13; } -uint8_t RepeaterMesh::handleAnonRegionsReq(const mesh::Identity& sender, uint32_t sender_timestamp, const uint8_t* data) { +uint8_t RepeaterMesh::handleAnonRegionsReq(const mesh::Identity& sender, uint32_t sender_timestamp, const uint8_t* data, size_t data_len) { if (anon_limiter.allow(getRTCClock()->getCurrentTime())) { + if (data_len < 1) return 0; reply_path_len = *data++; - mesh::Packet::copyPath(reply_path, data, reply_path_len); + data_len--; + /* data is anon-req-supplied; bound copy with remaining data_len. */ + mesh::Packet::copyPath(reply_path, data, data_len, reply_path_len); memcpy(reply_data, &sender_timestamp, 4); uint32_t now = getRTCClock()->getCurrentTime(); @@ -187,10 +190,13 @@ uint8_t RepeaterMesh::handleAnonRegionsReq(const mesh::Identity& sender, uint32_ return 0; } -uint8_t RepeaterMesh::handleAnonOwnerReq(const mesh::Identity& sender, uint32_t sender_timestamp, const uint8_t* data) { +uint8_t RepeaterMesh::handleAnonOwnerReq(const mesh::Identity& sender, uint32_t sender_timestamp, const uint8_t* data, size_t data_len) { if (anon_limiter.allow(getRTCClock()->getCurrentTime())) { + if (data_len < 1) return 0; reply_path_len = *data++; - mesh::Packet::copyPath(reply_path, data, reply_path_len); + data_len--; + /* data is anon-req-supplied; bound copy with remaining data_len. */ + mesh::Packet::copyPath(reply_path, data, data_len, reply_path_len); memcpy(reply_data, &sender_timestamp, 4); uint32_t now = getRTCClock()->getCurrentTime(); @@ -202,10 +208,13 @@ uint8_t RepeaterMesh::handleAnonOwnerReq(const mesh::Identity& sender, uint32_t return 0; } -uint8_t RepeaterMesh::handleAnonClockReq(const mesh::Identity& sender, uint32_t sender_timestamp, const uint8_t* data) { +uint8_t RepeaterMesh::handleAnonClockReq(const mesh::Identity& sender, uint32_t sender_timestamp, const uint8_t* data, size_t data_len) { if (anon_limiter.allow(getRTCClock()->getCurrentTime())) { + if (data_len < 1) return 0; reply_path_len = *data++; - mesh::Packet::copyPath(reply_path, data, reply_path_len); + data_len--; + /* data is anon-req-supplied; bound copy with remaining data_len. */ + mesh::Packet::copyPath(reply_path, data, data_len, reply_path_len); memcpy(reply_data, &sender_timestamp, 4); uint32_t now = getRTCClock()->getCurrentTime(); @@ -609,11 +618,11 @@ void RepeaterMesh::onAnonDataRecv(mesh::Packet* packet, const uint8_t* secret, c if (data[4] == 0 || data[4] >= ' ') { reply_len = handleLoginReq(sender, secret, timestamp, &data[4], packet->isRouteFlood()); } else if (data[4] == ANON_REQ_TYPE_REGIONS && packet->isRouteDirect()) { - reply_len = handleAnonRegionsReq(sender, timestamp, &data[5]); + reply_len = handleAnonRegionsReq(sender, timestamp, &data[5], (len > 5) ? (len - 5) : 0); } else if (data[4] == ANON_REQ_TYPE_OWNER && packet->isRouteDirect()) { - reply_len = handleAnonOwnerReq(sender, timestamp, &data[5]); + reply_len = handleAnonOwnerReq(sender, timestamp, &data[5], (len > 5) ? (len - 5) : 0); } else if (data[4] == ANON_REQ_TYPE_BASIC && packet->isRouteDirect()) { - reply_len = handleAnonClockReq(sender, timestamp, &data[5]); + reply_len = handleAnonClockReq(sender, timestamp, &data[5], (len > 5) ? (len - 5) : 0); } else { reply_len = 0; } @@ -773,7 +782,9 @@ bool RepeaterMesh::onPeerPathRecv(mesh::Packet* packet, int sender_idx, const ui if (i >= 0 && i < acl.getNumClients()) { LOG_DBG("PATH to client, path_len=%d", path_len); auto client = acl.getClientByIdx(i); - client->out_path_len = mesh::Packet::copyPath(client->out_path, path, path_len); + /* path source bounded by upstream packet parser; client->out_path + * is MAX_PATH_SIZE-sized. */ + client->out_path_len = mesh::Packet::copyPath(client->out_path, path, MAX_PATH_SIZE, path_len); client->last_activity = getRTCClock()->getCurrentTime(); } return false; diff --git a/zephcore/app/RepeaterMesh.h b/zephcore/app/RepeaterMesh.h index e8b2abc..bd1a77d 100644 --- a/zephcore/app/RepeaterMesh.h +++ b/zephcore/app/RepeaterMesh.h @@ -124,9 +124,9 @@ class RepeaterMesh : public mesh::Mesh, public CommonCLICallbacks { void putNeighbour(const mesh::Identity& id, uint32_t timestamp, float snr); uint8_t handleLoginReq(const mesh::Identity& sender, const uint8_t* secret, uint32_t sender_timestamp, const uint8_t* data, bool is_flood); - uint8_t handleAnonRegionsReq(const mesh::Identity& sender, uint32_t sender_timestamp, const uint8_t* data); - uint8_t handleAnonOwnerReq(const mesh::Identity& sender, uint32_t sender_timestamp, const uint8_t* data); - uint8_t handleAnonClockReq(const mesh::Identity& sender, uint32_t sender_timestamp, const uint8_t* data); + uint8_t handleAnonRegionsReq(const mesh::Identity& sender, uint32_t sender_timestamp, const uint8_t* data, size_t data_len); + uint8_t handleAnonOwnerReq(const mesh::Identity& sender, uint32_t sender_timestamp, const uint8_t* data, size_t data_len); + uint8_t handleAnonClockReq(const mesh::Identity& sender, uint32_t sender_timestamp, const uint8_t* data, size_t data_len); int handleRequest(ClientInfo* sender, uint32_t sender_timestamp, uint8_t* payload, size_t payload_len); mesh::Packet* createSelfAdvert(); void sendFloodScoped(const TransportKey& scope, mesh::Packet* pkt, uint32_t delay_millis, uint8_t path_hash_size); diff --git a/zephcore/helpers/BaseChatMesh.cpp b/zephcore/helpers/BaseChatMesh.cpp index 25c9b78..530e379 100644 --- a/zephcore/helpers/BaseChatMesh.cpp +++ b/zephcore/helpers/BaseChatMesh.cpp @@ -335,7 +335,10 @@ bool BaseChatMesh::onPeerPathRecv(mesh::Packet *packet, int sender_idx, const ui bool BaseChatMesh::onContactPathRecv(ContactInfo &from, uint8_t *in_path, uint8_t in_path_len, uint8_t *out_path, uint8_t out_path_len, uint8_t extra_type, uint8_t *extra, uint8_t extra_len) { - from.out_path_len = mesh::Packet::copyPath(from.out_path, out_path, out_path_len); + /* out_path is from the inner payload of a validated LoRa packet (caller path). + * Existing contract is that the upstream parser bounds the available bytes; + * pass MAX_PATH_SIZE to preserve behavior while making the bound explicit. */ + from.out_path_len = mesh::Packet::copyPath(from.out_path, out_path, MAX_PATH_SIZE, out_path_len); from.lastmod = getRTCClock()->getCurrentTime(); onContactPathUpdated(from); diff --git a/zephcore/include/mesh/Packet.h b/zephcore/include/mesh/Packet.h index 11c0d64..f6f0e31 100644 --- a/zephcore/include/mesh/Packet.h +++ b/zephcore/include/mesh/Packet.h @@ -68,8 +68,17 @@ public: void setPathHashCount(uint8_t n) { path_len &= ~0x3F; path_len |= n; } void setPathHashSizeAndCount(uint8_t sz, uint8_t n) { path_len = ((sz - 1) << 6) | (n & 0x3F); } - static uint8_t copyPath(uint8_t *dest, const uint8_t *src, uint8_t path_len); - static size_t writePath(uint8_t *dest, const uint8_t *src, uint8_t path_len); + /* writePath / copyPath decode the 6-bit hash_count + 2-bit hash_size + * encoding in path_len, then copy hash_count*hash_size bytes from src + * to dest. src_len bounds the source: if the decoded byte count would + * read past src_len, the call returns 0 (no copy). + * + * Trusted callers writing from internal MAX_PATH_SIZE buffers must + * pass MAX_PATH_SIZE. Callers handling untrusted input (BLE phone + * frames, LoRa anon-request payloads) must pass the real remaining + * length so a malformed path_len cannot trigger an OOB read. */ + static size_t writePath(uint8_t *dest, const uint8_t *src, size_t src_len, uint8_t path_len); + static uint8_t copyPath(uint8_t *dest, const uint8_t *src, size_t src_len, uint8_t path_len); static bool isValidPathLen(uint8_t path_len); void markDoNotRetransmit() { header = 0xFF; } diff --git a/zephcore/src/Dispatcher.cpp b/zephcore/src/Dispatcher.cpp index d9f2b1f..304f329 100644 --- a/zephcore/src/Dispatcher.cpp +++ b/zephcore/src/Dispatcher.cpp @@ -437,7 +437,8 @@ void Dispatcher::checkSend() memcpy(&raw[len], &outbound->transport_codes[1], 2); len += 2; } raw[len++] = outbound->path_len; - len += Packet::writePath(&raw[len], outbound->path, outbound->path_len); + /* Trusted source: outbound->path is MAX_PATH_SIZE-sized. */ + len += Packet::writePath(&raw[len], outbound->path, MAX_PATH_SIZE, outbound->path_len); if (len + outbound->payload_len > MAX_TRANS_UNIT) { LOG_ERR("checkSend: packet too large len=%d+%d > %d", len, outbound->payload_len, MAX_TRANS_UNIT); diff --git a/zephcore/src/Mesh.cpp b/zephcore/src/Mesh.cpp index a5ff8e6..2fda297 100644 --- a/zephcore/src/Mesh.cpp +++ b/zephcore/src/Mesh.cpp @@ -112,7 +112,8 @@ DispatcherAction Mesh::forwardMultipartDirect(Packet *pkt) if (type == PAYLOAD_TYPE_ACK && pkt->payload_len >= 5) { Packet tmp; tmp.header = pkt->header; - tmp.path_len = Packet::copyPath(tmp.path, pkt->path, pkt->path_len); + /* Trusted source: pkt->path is MAX_PATH_SIZE-sized. */ + tmp.path_len = Packet::copyPath(tmp.path, pkt->path, MAX_PATH_SIZE, pkt->path_len); tmp.payload_len = pkt->payload_len - 1; memcpy(tmp.payload, &pkt->payload[1], tmp.payload_len); if (!_tables->hasSeen(&tmp)) { @@ -130,7 +131,8 @@ void Mesh::routeDirectRecvAcks(Packet *packet, uint32_t delay_millis) memcpy(&crc, packet->payload, 4); Packet *a2 = createAck(crc); if (a2) { - a2->path_len = Packet::copyPath(a2->path, packet->path, packet->path_len); + /* Trusted source: packet->path is MAX_PATH_SIZE-sized. */ + a2->path_len = Packet::copyPath(a2->path, packet->path, MAX_PATH_SIZE, packet->path_len); a2->header &= ~PH_ROUTE_MASK; a2->header |= ROUTE_TYPE_DIRECT; sendPacket(a2, 0, delay_millis); @@ -385,7 +387,8 @@ DispatcherAction Mesh::onRecvPacket(Packet *pkt) if (type == PAYLOAD_TYPE_ACK && pkt->payload_len >= 5) { Packet tmp; tmp.header = pkt->header; - tmp.path_len = Packet::copyPath(tmp.path, pkt->path, pkt->path_len); + /* Trusted source: pkt->path is MAX_PATH_SIZE-sized. */ + tmp.path_len = Packet::copyPath(tmp.path, pkt->path, MAX_PATH_SIZE, pkt->path_len); tmp.payload_len = pkt->payload_len - 1; memcpy(tmp.payload, &pkt->payload[1], tmp.payload_len); @@ -549,7 +552,11 @@ void Mesh::sendDirect(Packet *packet, const uint8_t *path, uint8_t path_len, uin packet->path_len = 0; pri = 5; } else { - packet->path_len = Packet::copyPath(packet->path, path, path_len); + /* path is caller-supplied; existing contract is that the caller has + * ensured at least the decoded path-byte-count is readable. Pass + * MAX_PATH_SIZE as the upper bound — this preserves existing + * behavior while making the API explicit. */ + packet->path_len = Packet::copyPath(packet->path, path, MAX_PATH_SIZE, path_len); if (packet->getPayloadType() == PAYLOAD_TYPE_PATH) { pri = 1; } else { diff --git a/zephcore/src/Packet.cpp b/zephcore/src/Packet.cpp index fae759d..8e75eae 100644 --- a/zephcore/src/Packet.cpp +++ b/zephcore/src/Packet.cpp @@ -24,21 +24,24 @@ bool Packet::isValidPathLen(uint8_t path_len) return hash_count * hash_size <= MAX_PATH_SIZE; } -size_t Packet::writePath(uint8_t *dest, const uint8_t *src, uint8_t path_len) +size_t Packet::writePath(uint8_t *dest, const uint8_t *src, size_t src_len, uint8_t path_len) { uint8_t hash_count = path_len & 63; uint8_t hash_size = (path_len >> 6) + 1; size_t len = hash_count * hash_size; if (len > MAX_PATH_SIZE) { - return 0; // Error + return 0; // Decoded path exceeds max + } + if (len > src_len) { + return 0; // Would read past source buffer (caller-supplied bound) } memcpy(dest, src, len); return len; } -uint8_t Packet::copyPath(uint8_t *dest, const uint8_t *src, uint8_t path_len) +uint8_t Packet::copyPath(uint8_t *dest, const uint8_t *src, size_t src_len, uint8_t path_len) { - size_t written = writePath(dest, src, path_len); + size_t written = writePath(dest, src, src_len, path_len); return written > 0 ? path_len : 0; } @@ -73,7 +76,8 @@ uint8_t Packet::writeTo(uint8_t dest[]) const memcpy(&dest[i], &transport_codes[1], 2); i += 2; } dest[i++] = path_len; - i += writePath(&dest[i], path, path_len); + /* Trusted source: Packet::path is MAX_PATH_SIZE-sized. */ + i += writePath(&dest[i], path, MAX_PATH_SIZE, path_len); memcpy(&dest[i], payload, payload_len); i += payload_len; return i; }