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; } 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) { 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); }; } } 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(),