ArduPilot / ArduPilot/ardupilot

Off by one error in GPS parser

Open Beginner friendly
#34,136 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C++
Stars
15.9k
Forks
21.4k
Avg merge
3d 17h
Merged PRs (30d)
119

Description

### Issue details
The NMEA KSXT parser has an off-by-one error that can cause an out-of-bounds write.

In libraries/AP_GPS/AP_GPS_NMEA.h, the KSXT field buffer contains 21 elements:

struct {
double fields[21];
} _ksxt;

Its valid indices are therefore 0 through 20. However, AP_GPS_NMEA.cpp accepts KSXT terms 1 through 22 inclusively:

case _GPS_SENTENCE_KSXT + 1 ... _GPS_SENTENCE_KSXT + 22:
_ksxt.fields[_term_number-1] = atof(_term);
break;

When _term_number is 22, the assignment writes to _ksxt.fields[21], which is one element beyond the end of the array. Because each element is a double, this normally results in an eight-byte out-of-bounds write.

The issue is reachable by supplying a KSXT sentence containing a nonempty 22nd field. For example, the KSXT sentence shown in the source can be extended as follows:

$KSXT,20211016083433.00,116.31296102,39.95817066,49.4911,223.57,-11.32,330.19,0.024,,1,3,28,27,,,,-0.012,0.021,0.020,,,1*00

The final 1 is parsed as field 22 and written to _ksxt.fields[21]. A valid checksum is not required to reach the write: _decode() calls _term_complete() for the data field when it encounters *, before marking the following term as the checksum and validating it.

This can corrupt the memory following _ksxt, resulting in undefined behavior, corrupted GPS state, or instability. The exact affected member depends on the build configuration and object layout.

The upper bound should be changed from 22 to 21:

case _GPS_SENTENCE_KSXT + 1 ... _GPS_SENTENCE_KSXT + 21:
_ksxt.fields[_term_number-1] = atof(_term);
break;

An explicit bounds check based on the array length would provide stronger protection against similar errors.
Please describe the problem

**Version**
ArduPilot revision 4fe7ad.

The same mismatch also appears to be present in the current master branch.

**Platform**
[ *] All
[ ] AntennaTracker
[ ] Copter
[ ] Plane
[ ] Rover
[ ] Submarine

**Airframe type**
Not airframe-specific.

**Hardware type**
Not hardware-specific.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the KSXT field definition in libraries/AP_GPS/AP_GPS_NMEA.h and the KSXT term handling in libraries/AP_GPS/AP_GPS_NMEA.cpp. Reproduce the issue with the extended KSXT sentence from the report, then verify that a nonempty 22nd field no longer writes past the buffer and that valid fields 1 through 21 still parse correctly.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
embedded-iot
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
86/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.