BLE handlers use length fields from the packet without checking how much data arrived

[DustinHab]

I searched open and closed issues and PRs and did not find these.

Several BLE write handlers cast a header struct directly onto om->om_data and then use length fields out of that header without checking how much data actually arrived. AlertNotificationService::OnAlert does it properly with OS_MBUF_PKTLEN and os_mbuf_copydata, so this looks like an oversight in the other services rather than a deliberate convention.

FSService and DfuService sit behind GetDfuAndFsMode(), which is off by default, so reaching those needs the user to have enabled firmware updates or file access first. SimpleWeatherService has no such check. None of them require bonding.

Variable length arrays sized by the peer

FSService.cpp, in DELETE, MKDIR, LISTDIR and MOVE:

uint16_t plen = header->pathlen;
char path[plen + 1] = {0};

DfuService.cpp, in the init packet parser:

uint16_t softdeviceArrayLength = om->om_data[8] + (om->om_data[9] << 8);
uint16_t sd[softdeviceArrayLength];

Both lengths come straight from the packet, so a peer can ask for 64 KB or 128 KB of stack on a part that has 64 KB of RAM in total.

Write past the end of the received buffer

FSService.cpp:304, MOVE:

uint16_t plen = header->OldPathLength;
header->pathstr[plen] = 0;

The offset comes from the packet and nothing limits it to the bytes that were received.

Off by one on filepath

FSService.cpp:98 and :165, READ and WRITE:

static constexpr uint16_t maxpathlen = 256;
char filepath[maxpathlen];
...
if (plen > maxpathlen) { ... }
memcpy(filepath, header->pathstr, plen);
filepath[plen] = 0;

plen == 256 passes the check, so filepath[256] writes into the next member, int fileSize. The comment on that line reads "counts for null term", so >= looks like the intent.

cppcheck reports this one as arrayIndexOutOfBoundsCond.

Reads past the end in the weather parser

SimpleWeatherService.cpp reads up to dataBuffer[52] in CreateCurrentWeather and up to dataBuffer[35] in CreateForecast, and copies 32 bytes from offset 16 into the location string, none of it checked against the packet length. On a short write the city name shown on the watch face contains whatever was next in memory. nbDays is clamped, so the forecast loop itself stays in bounds.

Unrelated, same file

The MOVE case in FSService::OnFSServiceRequested has no break before default:. Harmless today because default only breaks, but a case added in between would run silently.

Preferred solution

Check OS_MBUF_PKTLEN once at the top of each handler, reject anything shorter than the header it is about to read, and clamp path lengths against both the fixed buffer and the remaining packet bytes. Replace the variable length arrays with fixed buffers of maxpathlen. Change plen > maxpathlen to >=.

Found by reading the code. I have not written a proof of concept.

I can send a PR for this if you want it fixed along those lines.

Version

main, 6c119eb