Skip to content

GPS: keep MicroNMEA's over-reads inside the provider's NMEA buffer - #3624

Open
nullrouten0 wants to merge 2 commits into
meshcore-dev:devfrom
nullrouten0:fix/nmea-buffer-guard-tail
Open

nullrouten0 wants to merge 2 commits into
meshcore-dev:devfrom
nullrouten0:fix/nmea-buffer-guard-tail

Conversation

@nullrouten0

Copy link
Copy Markdown

What changes

  • MicroNMEALocationProvider keeps a zeroed 16-byte tail behind the 100-byte buffer it hands to MicroNMEA. The library still sees 100 bytes, so the accepted sentence length and the parse of every well-formed sentence are unchanged. Cost: 16 bytes of RAM per provider.
  • A new native_gps test environment compiles the real provider header against the real MicroNMEA 2.0.6 with the address and undefined-behaviour sanitizers on, and the unit-test workflow runs it. The shared mocks gain what the header needs to build on a host: yield(), the ctype.h functions, minimal digital I/O, mesh::RTCClock, and a DateTime stand-in. The suite has a shared helper header, main(), and a parsing smoke test; this fix adds test_buffer_guard.cpp.

Why

MicroNMEA never writes past the length it is given, but its field parsers use fixed-width skips (s += 2 after the status and hemisphere fields, s + 6 after the date) that do not check for the end of the sentence. Its checksum test also stops at the first *. So a 99-character sentence such as $GPRMC,1*56 followed by 85 arbitrary bytes and ,AB passes the checksum, and its tail is parsed as fields. The parser then reads up to 5 bytes past the terminating NUL. In the provider, those bytes belong to the MicroNMEA object that sits right after the buffer.

Nothing crashes, but the result depends on object layout, and the parse leaves isValid() true with an impossible latitude. The bytes come from the GPS UART, so this is a robustness fix, not a security one: the trigger is a misbehaving receiver or a damaged line.

What I checked

  • pio test -e native -e native_kiss_modem -e native_gps: 60 of 60 pass, the 54 existing tests with the changed mocks plus 6 new ones.
  • The guard size: with the address sanitizer on, a heap buffer of exactly 100 + 16 bytes survives the crafted sentences and 20,000 malformed ones. With a 4-byte guard the sanitizer reports a read 0 bytes past a 104-byte region, in skipField() after parseDate(); 5 bytes pass. So the furthest read is 5 bytes past the end, and 16 leaves margin.
  • Mutation check: 7 deliberate breaks of the fix all fail the suite. They are: full revert, guard 4, guard 0, passing the whole array size as the length, dropping the zero initialisation, and length 99 and 101. The test that compares against a NUL-padded reference builds the provider in memory pre-filled with 0xA5, so it does not depend on fresh zeroed memory.
  • Firmware builds, dev and this branch:
Build RAM (bytes) Flash (bytes)
Heltec_v3_repeater 62,512 → 62,528 1,174,497 → 1,174,529
RAK_4631_repeater 37,240 → 37,256 440,924 → 440,940

Not checked

  • No hardware run. The over-read is only observable with a sanitizer or a debugger.
  • The sanitizer flags need gcc or clang on Linux or macOS, so native_gps will not build with MinGW on Windows. The existing native and native_kiss_modem environments are unchanged.
  • MicroNMEA itself is not changed. Its parseFloat() also overflows a signed long on long digit runs, a separate library bug; the new tests keep numeric fields short so UBSan does not stop on it.

Related

This PR adds the native_gps environment as its first commit. Two follow-up GPS PRs carry the same commit and each adds only its own test file, so the three merge in any order without conflicts, and the shared commit drops out on rebase whichever lands first.

🤖 Generated with Claude Code

nullrouten0 and others added 2 commits October 18, 2026 00:33
Add a native_gps PlatformIO environment that compiles the real
MicroNMEALocationProvider header against the real MicroNMEA 2.0.6
library, with the address and undefined-behaviour sanitizers on, and
run it in the unit-test workflow.

The mocks gain what the provider header needs to compile on a host:
yield(), the C character classification functions, minimal digital
I/O, mesh::RTCClock and a DateTime stand-in. The suite has a shared
helper header and starts with a parsing smoke test on well-formed GGA
and RMC sentences; later changes add their own test files to it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MicroNMEA 2.0.6 never writes past the buffer length it is given, but its
field parsers use fixed-width skips that do not check for the end of the
sentence, and its checksum test stops at the first '*'. A 99-character
sentence whose checksum covers only its first few fields therefore makes
the parser read up to five bytes past the terminating NUL, into the
MicroNMEA object that follows the buffer in MicroNMEALocationProvider.
The result depends on object layout, and the parse can leave isValid()
true with a meaningless position.

Reserve a zeroed 16-byte tail behind the 100 bytes the library is given.
The accepted sentence length and the parse of every well-formed sentence
are unchanged; malformed sentences now parse as if followed by NULs.

Tests: a heap buffer of exactly the reserved size under the address
sanitizer (a 4-byte guard fails it, 5 passes), the tail is never written,
sentences of 100+ characters are still rejected, and a provider built in
non-zero memory parses a crafted sentence's position the same as a
NUL-padded reference.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mikecarper added a commit to mikecarper/MeshCore that referenced this pull request Oct 18, 2026
Port the guarded NMEA buffer, validated RMC clock samples and coordinate bounds from meshcore-dev#3624, meshcore-dev#3625 and meshcore-dev#3626. Adapt clock admission to retain this branch's signed date limits, bounded loop work, tracker acquisitions, sample consumption, diagnostics and periodic settings.

Upstream-PR: meshcore-dev#3624
Upstream-Commit: 1ac389b
Upstream-PR: meshcore-dev#3625
Upstream-Commit: 72ed4dc
Upstream-PR: meshcore-dev#3626
Upstream-Commit: 499727a

Co-authored-by: nullrouten0 <201262769+nullrouten0@users.noreply.github.com>
mikecarper added a commit to mikecarper/MeshCore that referenced this pull request Oct 22, 2026
Port the guarded NMEA buffer, validated RMC clock samples and coordinate bounds from meshcore-dev#3624, meshcore-dev#3625 and meshcore-dev#3626. Adapt clock admission to retain this branch's signed date limits, bounded loop work, tracker acquisitions, sample consumption, diagnostics and periodic settings.

Upstream-PR: meshcore-dev#3624
Upstream-Commit: 1ac389b
Upstream-PR: meshcore-dev#3625
Upstream-Commit: 72ed4dc
Upstream-PR: meshcore-dev#3626
Upstream-Commit: 499727a

Co-authored-by: nullrouten0 <201262769+nullrouten0@users.noreply.github.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant