NMEAParser : fix possible buffer overflow

brunotl · Apr 9, 2024 · 81dc498 · 81dc498
1 parent 60c06b1
commit 81dc498
Show file tree

Hide file tree

Showing 7 changed files with 34 additions and 23 deletions.
diff --git a/Common/Header/Parser.h b/Common/Header/Parser.h
@@ -7,6 +7,7 @@
 #include "Geographic/GeoPoint.h"
 #include "Util/ScopeExit.hxx"
 #include "Time/PeriodClock.hpp"
+#include "Sizes.h"
 
 #if defined(PNA) && defined(UNDER_CE)
 #include "lkgpsapi.h"
@@ -62,8 +63,10 @@ class NMEAParser {
   // these routines can be used by other parsers.
 
   static double ParseAltitude(const char* value, const char* format);
-  static size_t ValidateAndExtract(const char* src, char* dst, size_t dstsz, char** arr, size_t arrsz);
-  static size_t ExtractParameters(const char* src, char* dst, char** arr, size_t sz);
+
+  static size_t ValidateAndExtract(const char* src, char (&dst)[MAX_NMEA_LEN], char* (&arr)[MAX_NMEA_PARAMS]);
+  static size_t ExtractParameters(const char* src, char (&dst)[MAX_NMEA_LEN], char* (&arr)[MAX_NMEA_PARAMS]);
+
   static void ExtractParameter(const char* Source, char* Destination, int DesiredFieldNumber);
   static BOOL NMEAChecksum(const char* String);
 

diff --git a/Common/Source/Comm/Parser.cpp b/Common/Source/Comm/Parser.cpp
@@ -206,20 +206,23 @@ BOOL NMEAParser::ParseGPS_POSITION_internal(const GPS_POSITION& loc, NMEA_INFO&
 #endif
 
 BOOL NMEAParser::ParseNMEAString_Internal(DeviceDescriptor_t& d, const char* String, NMEA_INFO* pGPS) {
+  if (!String) {
+    return FALSE;
+  }
 
   auto wait_ack = d.lock_wait_ack();
   if (wait_ack && wait_ack->check(String)) {
     return TRUE;
   }
 
-  char ctemp[MAX_NMEA_LEN];
-  char* params[MAX_NMEA_PARAMS];
-
   if (String[0] !='$') {
     return FALSE;
   }
 
-  size_t n_params = ValidateAndExtract(String, ctemp, MAX_NMEA_LEN, params, MAX_NMEA_PARAMS);
+  char ctemp[MAX_NMEA_LEN];
+  char* params[MAX_NMEA_PARAMS];
+
+  size_t n_params = ValidateAndExtract(String, ctemp, params);
   if (n_params < 1 || params[0][0] != '$') {
     return FALSE;
   }
@@ -874,7 +877,7 @@ BOOL NMEAParser::RMZ(DeviceDescriptor_t& d, const char* String, char **params, s
 
 
 // TASMAN instruments support for Tasman Flight Pack model Fp10
-BOOL NMEAParser::PTAS1(DeviceDescriptor_t& d, const char* String, char **params, size_t nparams, NMEA_INFO *pGPS)
+BOOL NMEAParser::PTAS1(DeviceDescriptor_t& d, const char* String, char **params, size_t nparams, NMEA_INFO *pGPS) 
 {
   if(nparams < 4) {
     TESTBENCH_DO_ONLY(10,StartupStore(_T(". NMEAParser invalid PTAS1 sentence, nparams=%u%s"),(unsigned)nparams,NEWLINE));

diff --git a/Common/Source/Comm/UtilsParser.cpp b/Common/Source/Comm/UtilsParser.cpp
@@ -52,32 +52,35 @@ void NMEAParser::ExtractParameter(const char* Source, char* Destination, int Des
  * Same as ExtractParameters, but also validate the length of
  * the string and the NMEA checksum.
  */
-size_t NMEAParser::ValidateAndExtract(const char* src, char* dst, size_t dstsz, char** arr, size_t arrsz) {
+size_t NMEAParser::ValidateAndExtract(const char* src, char (&dst)[MAX_NMEA_LEN], char* (&arr)[MAX_NMEA_PARAMS]) {
   if (!NMEAChecksum(src))
     return 0;
 
-  return ExtractParameters(src, dst, arr, arrsz);
+  return ExtractParameters(src, dst, arr);
 }
 
-size_t NMEAParser::ExtractParameters(const char* src, char* dst, char** arr, size_t sz) {
+size_t NMEAParser::ExtractParameters(const char* src, char (&dst)[MAX_NMEA_LEN], char* (&arr)[MAX_NMEA_PARAMS]) {
   if (!src || !(*src)) {
     return 0;
   }
 
+  auto out = std::begin(dst);
+  const auto out_end = std::prev(std::end(dst)); // reserve placeholder for trailing zero
+
   size_t i = 0;
-  arr[i++] = dst;
+  arr[i++] = out;
 
-  while (*src && *src != '*') {
+  while (*src && *src != '*' && out != out_end && i < MAX_NMEA_PARAMS) {
     if (*src == ',') {
       *dst = '\0';
       arr[i++] = dst + 1;
     } else {
       *dst = *src;
     }
     ++src;
-    ++dst;
+    ++out;
   }
-  *dst = '\0';
+  *out = '\0';
   return i;
 }
 
@@ -100,15 +103,15 @@ BOOL NMEAParser::NMEAChecksum(const char *String) {
     return TRUE;
   }
 
-  uint8_t CalcCheckSum = 0;
-  uint8_t ReadCheckSum = 0;
+  uint8_t CalcCheckSum = 0U;
+  uint8_t ReadCheckSum = 0U;
 
   if (*String && *String == '$') {
     ++String;
   }
 
   while (*String && *String != '*') {
-    CalcCheckSum = (unsigned char)(CalcCheckSum ^ *(String++));
+    CalcCheckSum ^= static_cast<uint8_t>(*(String++));
   }
 
   if (*String == '*') {

diff --git a/Common/Source/Devices/devEWMicroRecorder.cpp b/Common/Source/Devices/devEWMicroRecorder.cpp
@@ -55,8 +55,10 @@ BOOL ExpectStringWait(DeviceDescriptor_t* d, const TCHAR *token) {
 
 
 BOOL EWMicroRecorderParseNMEA(DeviceDescriptor_t* d, const char *String, NMEA_INFO *pGPS){
-  char ctemp[80], *params[5];
-  int nparams = NMEAParser::ValidateAndExtract(String, ctemp, 80, params, 5);
+  char ctemp[MAX_NMEA_LEN];
+  char* params[MAX_NMEA_PARAMS];
+
+  int nparams = NMEAParser::ValidateAndExtract(String, ctemp, params);
   if (nparams < 1)
     return FALSE;
 

diff --git a/Common/Source/Devices/devLXNano3.cpp b/Common/Source/Devices/devLXNano3.cpp
@@ -1130,8 +1130,8 @@ BOOL DevLXNanoIII::PLXVC(DeviceDescriptor_t* d, const char* sentence, NMEA_INFO*
   bool bCRCok = NMEAParser::NMEAChecksum(sentence);
 
   char ctemp[MAX_NMEA_LEN];
-  char* params[10];
-  size_t n_params = NMEAParser::ExtractParameters(sentence, ctemp, params, 10);
+  char* params[MAX_NMEA_PARAMS];
+  size_t n_params = NMEAParser::ExtractParameters(sentence, ctemp, params);
   if(n_params < 4) {
     return FALSE;
   }

diff --git a/Common/Source/Devices/devXCTracer.cpp b/Common/Source/Devices/devXCTracer.cpp
@@ -227,7 +227,7 @@ static BOOL XCTracerParseNMEA(DeviceDescriptor_t* d, const char *String, NMEA_IN
     char ctemp[MAX_NMEA_LEN];
     char * params[MAX_NMEA_PARAMS];
 
-    size_t n_params = NMEAParser::ValidateAndExtract(String, ctemp, MAX_NMEA_LEN, params, MAX_NMEA_PARAMS);
+    size_t n_params = NMEAParser::ValidateAndExtract(String, ctemp, params);
     if (n_params>0) {
         if(strcmp(params[0], "$XCTRC") == 0) {
             return XTRC(d, params, n_params, _INFO);

diff --git a/Common/Source/Devices/devXCVario.cpp b/Common/Source/Devices/devXCVario.cpp
@@ -161,7 +161,7 @@ BOOL ParseNMEA(DeviceDescriptor_t* d, const char* String, NMEA_INFO* pGPS) {
   char ctemp[MAX_NMEA_LEN];
   char* params[MAX_NMEA_PARAMS];
 
-  size_t n_params = NMEAParser::ValidateAndExtract(String, ctemp, MAX_NMEA_LEN, params, MAX_NMEA_PARAMS);
+  size_t n_params = NMEAParser::ValidateAndExtract(String, ctemp, params);
   if (n_params > 0) {
     if (params[0] == "$PXCV"sv) {
       return PXCV(d, params, n_params, pGPS);