search

adafruit / Adafruit_GPS

An interrupt-based GPS Arduino library for no-parsing-required use
Other
475 stars 318 forks source link

Add warning about library usage #140

Closed devtty1er closed 2 years ago

devtty1er commented 2 years ago

I emailed support@adafruit.com with some details leading to this suggested warning to end users (as well as some additional recommendations)

ladyada commented 2 years ago

ideally we would not corrupt on invalid NMEAs - whats an example of one that causes corruption - we can then make a test suite?

devtty1er commented 2 years ago

There are a number of real/potential issues. I have not tracked all of them down in terms of reachability/exploitability.

For example, see my remediatory commit here: https://github.com/adafruit/Adafruit_GPS/compare/master...Cyber-by-the-Sea:Adafruit_GPS:fix-npd

It looks like there was an effort to prevent a null pointer dereference (npd) by checking against NULL, but that check won't actually be performed until after the pointer is dereferenced.

Even correcting this one conditional statement won't account for the many npd's that can occur by not checking the return value of strchr.

Unfortunately, there are quite a few places where assumptions about NMEA sentence formats were made.

Furthermore, just changing Adafruit_GPS::parse to exit early is insufficient. Although the docstring for parse indicates @return True if successfully parsed, false if fails check or parsing, parse updates instance variables all throughout parsing, regardless of the potential for early exit due to failed parsing.

Consider the following "fix:"

if (parseCoord(p, &latitudeDegrees, &latitude, &latitude_fixed, &lat))
    newDataValue(NMEA_LAT, latitudeDegrees);
p = strchr(p, ',') + 1;
+ if (p == (char*) 0x1) return false;  // exit early

The early exit would still come after Adafruit_GPS instance variables were updated in parseCoord.

There are also a number of instances where unsafe coding standards were used: tokenOnList (which is a likely extension point) seems to rely on a potential buffer overread, strcpy (unsafe) is used, and there quite a few instances of implicit conversion.

The best example of the dangers of implicit conversion is probably in parseStr. Although strncpy is used in place of strcpy, the len parameter has an implicit sign conversion from int to size_t (so a negative value as a result of the pointer math/min would be interpreted as a large unsigned int).

Due to the scope of required changes, I recommend adding the warning banner in this PR for the short term.

ladyada commented 2 years ago

@drak7 take a look! also might be good to buffer any changed vars till after the parsing succeeds

caternuson commented 2 years ago

Approved, so merging.