From ad63afeda0546887ef7dfc3766d59be808e96b81 Mon Sep 17 00:00:00 2001 From: DustinHab Date: Sat, 12 Sep 2026 22:35:16 +0200 Subject: [PATCH 1/4] FSService: check the received length before reading headers and paths Every command casts its header onto om_data and then uses length fields out of it without looking at how much data actually arrived. MOVE terminated the old path in place with header->pathstr[plen] = 0, where plen is a uint16 from the packet, so the write landed at an offset the peer picked. It now copies both paths out instead. DELETE, MKDIR, LISTDIR and MOVE sized a stack array from the announced path length, and READ sized one from the requested chunk size. On a part with 64 KB of RAM that let a peer ask for far more stack than exists. Paths now go into fixed maxpathlen buffers and chunks are capped at what a single notification can carry anyway. WRITE_DATA passed header->dataSize straight to FileWrite, so a short packet with a large dataSize wrote memory past the packet into the file. It is now clamped to what arrived. READ and WRITE rejected a path only when plen > maxpathlen, so plen == 256 got through and filepath[256] wrote into the next member, fileSize. The comment on that line said "counts for null term", so >= was the intent. MOVE also fell through to default because its break was missing. prepareReadDataResp is removed. Nothing calls it, and it read a file straight into the flexible array member of a ReadResponse the caller would have had on the stack. Boundary cases of the new check were exercised on the host under asan and ubsan: path length 255 and 256, packet one byte short, empty path, and 65535. --- src/components/ble/FSService.cpp | 146 +++++++++++++++++++------------ src/components/ble/FSService.h | 5 +- 2 files changed, 95 insertions(+), 56 deletions(-) diff --git a/src/components/ble/FSService.cpp b/src/components/ble/FSService.cpp index 721ed297c5..45f6e5beb0 100644 --- a/src/components/ble/FSService.cpp +++ b/src/components/ble/FSService.cpp @@ -74,7 +74,31 @@ int FSService::OnFSServiceRequested(uint16_t connectionHandle, uint16_t attribut return 0; } +namespace { + // The path follows its header in the same packet. Both the length field and + // the buffer it is copied into have to be respected, so check against the + // bytes that actually arrived and against the destination size. + bool CopyPath(const char* pathstr, uint16_t pathlen, size_t pathOffset, size_t packetLen, char* out, size_t outSize) { + if (static_cast(pathlen) + 1 > outSize) { + return false; + } + if (pathOffset + static_cast(pathlen) > packetLen) { + return false; + } + memcpy(out, pathstr, pathlen); + out[pathlen] = '\0'; + return true; + } +} + int FSService::FSCommandHandler(uint16_t connectionHandle, os_mbuf* om) { + // Every case below reads its header straight out of the received buffer. + // om_len is how much is really there, so header and path lengths are + // measured against it before they are used. + const size_t packetLen = om->om_len; + if (packetLen < 1) { + return 0; + } auto command = static_cast(om->om_data[0]); NRF_LOG_INFO("[FS_S] -> FSCommandHandler Command %d", command); // Just always make sure we are awake... @@ -89,13 +113,13 @@ int FSService::FSCommandHandler(uint16_t connectionHandle, os_mbuf* om) { switch (command) { case commands::READ: { NRF_LOG_INFO("[FS_S] -> Read"); + if (packetLen < sizeof(ReadHeader)) { + return 0; + } auto* header = (ReadHeader*) om->om_data; - uint16_t plen = header->pathlen; - if (plen > maxpathlen) { //> counts for null term - return -1; + if (!CopyPath(header->pathstr, header->pathlen, sizeof(ReadHeader), packetLen, filepath, sizeof(filepath))) { + return 0; } - memcpy(filepath, header->pathstr, plen); - filepath[plen] = 0; // Copy and null terminate string ReadResponse resp; os_mbuf* om; resp.command = commands::READ_DATA; @@ -108,11 +132,11 @@ int FSService::FSCommandHandler(uint16_t connectionHandle, os_mbuf* om) { resp.totallen = 0; om = ble_hs_mbuf_from_flat(&resp, sizeof(ReadResponse)); } else { - resp.chunklen = std::min(header->chunksize, info.size); // TODO add mtu somehow + resp.chunklen = std::min(std::min(header->chunksize, info.size), maxChunkLen); resp.totallen = info.size; fs.FileOpen(&f, filepath, LFS_O_RDONLY); fs.FileSeek(&f, header->chunkoff); - uint8_t fileData[resp.chunklen] = {0}; + uint8_t fileData[maxChunkLen] = {0}; resp.chunklen = fs.FileRead(&f, fileData, resp.chunklen); om = ble_hs_mbuf_from_flat(&resp, sizeof(ReadResponse)); os_mbuf_append(om, fileData, resp.chunklen); @@ -124,6 +148,9 @@ int FSService::FSCommandHandler(uint16_t connectionHandle, os_mbuf* om) { } case commands::READ_PACING: { NRF_LOG_INFO("[FS_S] -> Readpacing"); + if (packetLen < sizeof(ReadHeader)) { + return 0; + } auto* header = (ReadHeader*) om->om_data; ReadResponse resp; resp.command = commands::READ_DATA; @@ -135,14 +162,14 @@ int FSService::FSCommandHandler(uint16_t connectionHandle, os_mbuf* om) { resp.chunklen = 0; resp.totallen = 0; } else { - resp.chunklen = std::min(header->chunksize, info.size); // TODO add mtu somehow + resp.chunklen = std::min(std::min(header->chunksize, info.size), maxChunkLen); resp.totallen = info.size; fs.FileOpen(&f, filepath, LFS_O_RDONLY); fs.FileSeek(&f, header->chunkoff); } os_mbuf* om; if (resp.chunklen > 0) { - uint8_t fileData[resp.chunklen] = {0}; + uint8_t fileData[maxChunkLen] = {0}; resp.chunklen = fs.FileRead(&f, fileData, resp.chunklen); om = ble_hs_mbuf_from_flat(&resp, sizeof(ReadResponse)); os_mbuf_append(om, fileData, resp.chunklen); @@ -156,13 +183,13 @@ int FSService::FSCommandHandler(uint16_t connectionHandle, os_mbuf* om) { } case commands::WRITE: { NRF_LOG_INFO("[FS_S] -> Write"); + if (packetLen < sizeof(WriteHeader)) { + return 0; + } auto* header = (WriteHeader*) om->om_data; - uint16_t plen = header->pathlen; - if (plen > maxpathlen) { //> counts for null term - return -1; // TODO make this actually return a BLE notif + if (!CopyPath(header->pathstr, header->pathlen, sizeof(WriteHeader), packetLen, filepath, sizeof(filepath))) { + return 0; // TODO make this actually return a BLE notif } - memcpy(filepath, header->pathstr, plen); - filepath[plen] = 0; // Copy and null terminate string fileSize = header->totalSize; WriteResponse resp; resp.command = commands::WRITE_PACING; @@ -181,7 +208,13 @@ int FSService::FSCommandHandler(uint16_t connectionHandle, os_mbuf* om) { } case commands::WRITE_DATA: { NRF_LOG_INFO("[FS_S] -> WriteData"); + if (packetLen < sizeof(WritePacing)) { + return 0; + } auto* header = (WritePacing*) om->om_data; + // dataSize is announced by the peer. Only what arrived may be written, + // otherwise the write runs off the end of the packet and into the file. + const uint32_t dataSize = std::min(header->dataSize, packetLen - sizeof(WritePacing)); WriteResponse resp; resp.command = commands::WRITE_PACING; resp.offset = header->offset; @@ -189,7 +222,7 @@ int FSService::FSCommandHandler(uint16_t connectionHandle, os_mbuf* om) { if (!(res = fs.FileOpen(&f, filepath, LFS_O_RDWR | LFS_O_CREAT))) { if ((res = fs.FileSeek(&f, header->offset)) >= 0) { - res = fs.FileWrite(&f, header->data, header->dataSize); + res = fs.FileWrite(&f, header->data, dataSize); } fs.FileClose(&f); } @@ -203,11 +236,14 @@ int FSService::FSCommandHandler(uint16_t connectionHandle, os_mbuf* om) { } case commands::DELETE: { NRF_LOG_INFO("[FS_S] -> Delete"); + if (packetLen < sizeof(DelHeader)) { + return 0; + } auto* header = (DelHeader*) om->om_data; - uint16_t plen = header->pathlen; - char path[plen + 1] = {0}; - memcpy(path, header->pathstr, plen); - path[plen] = 0; // Copy and null terminate string + char path[maxpathlen]; + if (!CopyPath(header->pathstr, header->pathlen, sizeof(DelHeader), packetLen, path, sizeof(path))) { + return 0; + } DelResponse resp {}; resp.command = commands::DELETE_STATUS; int res = fs.FileDelete(path); @@ -218,11 +254,14 @@ int FSService::FSCommandHandler(uint16_t connectionHandle, os_mbuf* om) { } case commands::MKDIR: { NRF_LOG_INFO("[FS_S] -> MKDir"); + if (packetLen < sizeof(MKDirHeader)) { + return 0; + } auto* header = (MKDirHeader*) om->om_data; - uint16_t plen = header->pathlen; - char path[plen + 1] = {0}; - memcpy(path, header->pathstr, plen); - path[plen] = 0; // Copy and null terminate string + char path[maxpathlen]; + if (!CopyPath(header->pathstr, header->pathlen, sizeof(MKDirHeader), packetLen, path, sizeof(path))) { + return 0; + } MKDirResponse resp {}; resp.command = commands::MKDIR_STATUS; resp.modification_time = 0; @@ -234,11 +273,14 @@ int FSService::FSCommandHandler(uint16_t connectionHandle, os_mbuf* om) { } case commands::LISTDIR: { NRF_LOG_INFO("[FS_S] -> ListDir"); + if (packetLen < sizeof(ListDirHeader)) { + return 0; + } ListDirHeader* header = (ListDirHeader*) om->om_data; - uint16_t plen = header->pathlen; - char path[plen + 1] = {0}; - path[plen] = 0; // Copy and null terminate string - memcpy(path, header->pathstr, plen); + char path[maxpathlen]; + if (!CopyPath(header->pathstr, header->pathlen, sizeof(ListDirHeader), packetLen, path, sizeof(path))) { + return 0; + } ListDirResponse resp {}; @@ -298,19 +340,34 @@ int FSService::FSCommandHandler(uint16_t connectionHandle, os_mbuf* om) { } case commands::MOVE: { NRF_LOG_INFO("[FS_S] -> Move"); + if (packetLen < sizeof(MoveHeader)) { + return 0; + } MoveHeader* header = (MoveHeader*) om->om_data; - uint16_t plen = header->OldPathLength; - // Null Terminate string - header->pathstr[plen] = 0; - char path[header->NewPathLength + 1] = {0}; - memcpy(path, &header->pathstr[plen + 1], header->NewPathLength); - path[header->NewPathLength] = 0; // Copy and null terminate string + // Both paths sit behind the header, separated by a terminator. The old + // one used to be terminated in place, which wrote into the received + // packet at an offset the peer chose. + const size_t oldPathLen = header->OldPathLength; + char oldPath[maxpathlen]; + char newPath[maxpathlen]; + if (!CopyPath(header->pathstr, header->OldPathLength, sizeof(MoveHeader), packetLen, oldPath, sizeof(oldPath))) { + return 0; + } + if (!CopyPath(&header->pathstr[oldPathLen + 1], + header->NewPathLength, + sizeof(MoveHeader) + oldPathLen + 1, + packetLen, + newPath, + sizeof(newPath))) { + return 0; + } MoveResponse resp {}; resp.command = commands::MOVE_STATUS; - int8_t res = (int8_t) fs.Rename(header->pathstr, path); + int8_t res = (int8_t) fs.Rename(oldPath, newPath); resp.status = (res == 0) ? 1 : res; auto* om = ble_hs_mbuf_from_flat(&resp, sizeof(MoveResponse)); ble_gattc_notify_custom(connectionHandle, transferCharacteristicHandle, om); + break; } default: break; @@ -321,24 +378,3 @@ int FSService::FSCommandHandler(uint16_t connectionHandle, os_mbuf* om) { } // Loads resp with file data given a valid filepath header and resp -void FSService::prepareReadDataResp(ReadHeader* header, ReadResponse* resp) { - // uint16_t plen = header->pathlen; - resp->command = commands::READ_DATA; - resp->chunkoff = header->chunkoff; - resp->status = 0x01; - struct lfs_info info = {}; - int res = fs.Stat(filepath, &info); - if (res == LFS_ERR_NOENT && info.type != LFS_TYPE_DIR) { - resp->status = 0x03; - resp->chunklen = 0; - resp->totallen = 0; - } else { - lfs_file f; - resp->chunklen = std::min(header->chunksize, info.size); - resp->totallen = info.size; - fs.FileOpen(&f, filepath, LFS_O_RDONLY); - fs.FileSeek(&f, header->chunkoff); - resp->chunklen = fs.FileRead(&f, resp->chunk, resp->chunklen); - fs.FileClose(&f); - } -} diff --git a/src/components/ble/FSService.h b/src/components/ble/FSService.h index b43bc9e479..e7c57e350b 100644 --- a/src/components/ble/FSService.h +++ b/src/components/ble/FSService.h @@ -37,6 +37,10 @@ namespace Pinetime { static constexpr uint16_t fsTransferId {0x0200}; uint16_t fsVersion = {0x0004}; static constexpr uint16_t maxpathlen = 256; + // A chunk has to fit into a single notification, so there is no point in + // honouring a larger request. Sizing a buffer from the request instead + // put the peer in charge of how much stack was used. + static constexpr uint32_t maxChunkLen = 200; static constexpr ble_uuid16_t fsServiceUuid { .u {.type = BLE_UUID_TYPE_16}, .value = {0xFEBB}}; // {0x72, 0x65, 0x66, 0x73, 0x6e, 0x61, 0x72, 0x54, 0x65, 0x6c, 0x69, 0x46, 0xBB, 0xFE, 0xAF, 0xAD}}; @@ -197,7 +201,6 @@ namespace Pinetime { }; int FSCommandHandler(uint16_t connectionHandle, os_mbuf* om); - void prepareReadDataResp(ReadHeader* header, ReadResponse* resp); }; } } From 037af45a1a6034e8a4623301df324f37e74af1c2 Mon Sep 17 00:00:00 2001 From: DustinHab Date: Sat, 12 Sep 2026 22:35:16 +0200 Subject: [PATCH 2/4] DfuService: check the received length before parsing packets The init packet parser read a softdevice count out of the packet and sized a stack array from it, up to 128 KB on a part with 64 KB of RAM, then walked past the end of the buffer to find the CRC behind it. Only the CRC is used, so the list is skipped rather than copied, and both the fixed part and the CRC offset are checked against the length that arrived. The start packet handler read twelve bytes and the control point read one or two without checking for them. nbPacketsToNotify is zero until the peer asks for packet receipt notifications. A peer that skips that request and sends data reached nbPacketReceived % nbPacketsToNotify with a zero divisor. --- src/components/ble/DfuService.cpp | 52 +++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/src/components/ble/DfuService.cpp b/src/components/ble/DfuService.cpp index ad9c99e9e0..616ce0bd9c 100644 --- a/src/components/ble/DfuService.cpp +++ b/src/components/ble/DfuService.cpp @@ -129,6 +129,11 @@ int DfuService::SendDfuRevision(os_mbuf* om) const { int DfuService::WritePacketHandler(uint16_t connectionHandle, os_mbuf* om) { switch (state) { case States::Start: { + // Three 32 bit sizes are expected here. + if (om->om_len < 12) { + NRF_LOG_INFO("[DFU] -> Start data too short"); + return 0; + } softdeviceSize = om->om_data[0] + (om->om_data[1] << 8) + (om->om_data[2] << 16) + (om->om_data[3] << 24); bootloaderSize = om->om_data[4] + (om->om_data[5] << 8) + (om->om_data[6] << 16) + (om->om_data[7] << 24); applicationSize = om->om_data[8] + (om->om_data[9] << 8) + (om->om_data[10] << 16) + (om->om_data[11] << 24); @@ -153,24 +158,33 @@ int DfuService::WritePacketHandler(uint16_t connectionHandle, os_mbuf* om) { } return 0; case States::Init: { + // The softdevice list is variable length and its size comes from the + // packet, so it decides where the CRC sits. Only the CRC is used, so the + // list is skipped rather than copied; sizing an array from that field + // asked for up to 128 KB of stack on a part that has 64 KB of RAM. + constexpr size_t headerSize = 10; + if (om->om_len < headerSize + 2) { + NRF_LOG_INFO("[DFU] -> Init data too short"); + return 0; + } uint16_t deviceType = om->om_data[0] + (om->om_data[1] << 8); uint16_t deviceRevision = om->om_data[2] + (om->om_data[3] << 8); uint32_t applicationVersion = om->om_data[4] + (om->om_data[5] << 8) + (om->om_data[6] << 16) + (om->om_data[7] << 24); uint16_t softdeviceArrayLength = om->om_data[8] + (om->om_data[9] << 8); - uint16_t sd[softdeviceArrayLength]; - for (int i = 0; i < softdeviceArrayLength; i++) { - sd[i] = om->om_data[10 + (i * 2)] + (om->om_data[10 + (i * 2) + 1] << 8); + + const size_t crcOffset = headerSize + (static_cast(softdeviceArrayLength) * 2); + if (crcOffset + 2 > om->om_len) { + NRF_LOG_INFO("[DFU] -> Init data announces %d softdevices but is too short for them", softdeviceArrayLength); + return 0; } - expectedCrc = om->om_data[10 + (softdeviceArrayLength * 2)] + (om->om_data[10 + (softdeviceArrayLength * 2) + 1] << 8); + expectedCrc = om->om_data[crcOffset] + (om->om_data[crcOffset + 1] << 8); - NRF_LOG_INFO( - "[DFU] -> Init data received : deviceType = %d, deviceRevision = %d, applicationVersion = %d, nb SD = %d, First SD = %d, CRC = %u", - deviceType, - deviceRevision, - applicationVersion, - softdeviceArrayLength, - sd[0], - expectedCrc); + NRF_LOG_INFO("[DFU] -> Init data received : deviceType = %d, deviceRevision = %d, applicationVersion = %d, nb SD = %d, CRC = %u", + deviceType, + deviceRevision, + applicationVersion, + softdeviceArrayLength, + expectedCrc); return 0; } @@ -181,7 +195,9 @@ int DfuService::WritePacketHandler(uint16_t connectionHandle, os_mbuf* om) { bytesReceived += om->om_len; bleController.FirmwareUpdateCurrentBytes(bytesReceived); - if ((nbPacketReceived % nbPacketsToNotify) == 0 && bytesReceived != applicationSize) { + // A peer that skips the packet receipt notification request leaves + // nbPacketsToNotify at zero, and the modulo below would divide by it. + if (nbPacketsToNotify > 0 && (nbPacketReceived % nbPacketsToNotify) == 0 && bytesReceived != applicationSize) { uint8_t data[5] {static_cast(Opcodes::PacketReceiptNotification), static_cast(bytesReceived & 0x000000FFu), static_cast(bytesReceived >> 8u), @@ -208,9 +224,19 @@ int DfuService::WritePacketHandler(uint16_t connectionHandle, os_mbuf* om) { } int DfuService::ControlPointHandler(uint16_t connectionHandle, os_mbuf* om) { + if (om->om_len < 1) { + return 0; + } auto opcode = static_cast(om->om_data[0]); NRF_LOG_INFO("[DFU] -> ControlPointHandler"); + // StartDFU, InitDFUParameters and PacketReceiptNotificationRequest all read + // a second byte. + if ((opcode == Opcodes::StartDFU || opcode == Opcodes::InitDFUParameters || opcode == Opcodes::PacketReceiptNotificationRequest) && + om->om_len < 2) { + return 0; + } + switch (opcode) { case Opcodes::StartDFU: { if (state != States::Idle && state != States::Start) { From 3c38bb6a9c0c038eaf762b39b3bfb34cbc4c5f97 Mon Sep 17 00:00:00 2001 From: DustinHab Date: Sat, 12 Sep 2026 22:35:16 +0200 Subject: [PATCH 3/4] SimpleWeatherService: check the received length before parsing CreateCurrentWeather reads up to offset 52 and copies 32 bytes from offset 16 into the location string, and CreateForecast reads up to offset 35, none of it checked against the packet. A short write left whatever followed the buffer in the city name shown on the watch face. This service has no DfuAndFsMode gate in front of it. The forecast log loop also went over all five days regardless of how many were present, dereferencing empty optionals in a debug build. --- src/components/ble/SimpleWeatherService.cpp | 22 ++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/components/ble/SimpleWeatherService.cpp b/src/components/ble/SimpleWeatherService.cpp index c2da93055e..f608786fe6 100644 --- a/src/components/ble/SimpleWeatherService.cpp +++ b/src/components/ble/SimpleWeatherService.cpp @@ -128,9 +128,21 @@ int SimpleWeatherService::OnCommand(struct ble_gatt_access_ctxt* ctxt) { const auto* buffer = ctxt->om; const auto* dataBuffer = buffer->om_data; + // The parsers below read fixed offsets out of the packet, so there has to be + // a packet that long. Message type and version are the first two bytes. + const size_t packetLen = buffer->om_len; + if (packetLen < 2) { + return 0; + } + switch (GetMessageType(dataBuffer)) { case MessageType::CurrentWeather: if (GetVersion(dataBuffer) <= 1) { + // Version 0 ends after the icon at offset 48, version 1 adds sunrise + // and sunset up to offset 52. + if (packetLen < (GetVersion(dataBuffer) == 1 ? 53u : 49u)) { + return 0; + } currentWeather = CreateCurrentWeather(dataBuffer); NRF_LOG_INFO("Current weather :\n\tTimestamp : %d\n\tTemperature:%d\n\tMin:%d\n\tMax:%d\n\tIcon:%d\n\tLocation:%s", currentWeather->timestamp, @@ -146,9 +158,17 @@ int SimpleWeatherService::OnCommand(struct ble_gatt_access_ctxt* ctxt) { break; case MessageType::Forecast: if (GetVersion(dataBuffer) == 0) { + // The day count sits at offset 10 and each day takes five bytes. + if (packetLen < 11) { + return 0; + } + const uint8_t nbDaysInBuffer = std::min(MaxNbForecastDays, dataBuffer[10]); + if (packetLen < 11u + (nbDaysInBuffer * 5u)) { + return 0; + } forecast = CreateForecast(dataBuffer); NRF_LOG_INFO("Forecast : Timestamp : %d", forecast->timestamp); - for (int i = 0; i < 5; i++) { + for (int i = 0; i < forecast->nbDays; i++) { NRF_LOG_INFO("\t[%d] Min: %d - Max : %d - Icon : %d", i, forecast->days[i]->minTemperature.PreciseCelsius(), From aab4d2d708a44602837b675ab6c0e269c8b0b76a Mon Sep 17 00:00:00 2001 From: DustinHab Date: Sat, 12 Sep 2026 22:35:16 +0200 Subject: [PATCH 4/4] AlertNotificationClient: don't dereference a null characteristic Every branch above guards against characteristic being null, the final else did not. Only reachable in a debug build, since the dereference is inside NRF_LOG_INFO. --- src/components/ble/AlertNotificationClient.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/components/ble/AlertNotificationClient.cpp b/src/components/ble/AlertNotificationClient.cpp index e3bc924230..acb1ce8f36 100644 --- a/src/components/ble/AlertNotificationClient.cpp +++ b/src/components/ble/AlertNotificationClient.cpp @@ -101,8 +101,9 @@ int AlertNotificationClient::OnCharacteristicsDiscoveryEvent(uint16_t connection } else if (characteristic != nullptr && ble_uuid_cmp(&controlPointUuid.u, &characteristic->uuid.u) == 0) { NRF_LOG_INFO("ANS Characteristic discovered : controlPointUuid"); controlPointHandle = characteristic->val_handle; - } else + } else if (characteristic != nullptr) { NRF_LOG_INFO("ANS Characteristic discovered : 0x%x", characteristic->val_handle); + } } return 0; }