mirror of
https://github.com/liquidraver/ZephCore.git
synced 2026-09-09 17:25:39 +00:00
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.
This commit is contained in:
@@ -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);
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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; }
|
||||
|
||||
@@ -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);
|
||||
|
||||
+11
-4
@@ -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 {
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user