Conversation
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.
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.
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.
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.
|
Build size and comparison to main:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2485.
The BLE write handlers in FSService, DfuService and SimpleWeatherService cast their header onto
om_dataand then trust the length fields inside it.AlertNotificationService::OnAlertalready measures everything againstOS_MBUF_PKTLEN, so this follows the same idea and checks against the length that actually arrived before anything is read.One commit per service so they can be taken separately.
What changes:
maxpathlenarrays rather than arrays sized from the announced length, and the read chunk is capped at 200 bytes. A chunk larger than that never fit into one notification anyway, so nothing that used to work stops working.WRITE_DATAclampsdataSizeto the bytes that arrived, instead of handing it toFileWriteas given.plen > maxpathlenbecomes a check that leaves room for the terminator, soplen == 256no longer writesfilepath[256]intofileSize.nbPacketsToNotifyis checked before the modulo that uses it as a divisor.nbDaysinstead of always five.prepareReadDataRespis removed. Nothing calls it, and it read a file into the flexible array member of aReadResponse. Say the word if you would rather keep it.breakand fell through todefault.Testing: