From ff911e209d9e18990d4f70432c18f779e81a02a4 Mon Sep 17 00:00:00 2001 From: Mingjie Shen Date: Thu, 20 Apr 2023 22:48:04 -0400 Subject: [PATCH 1/3] Fix potential buffer overflows when calling sprintf 1. Replace sprintf with snprintf, which is safer 2. An unsigned int or unsigned long int requires 11 bytes to print (including the null terminator) 3. Use PRIu16 macro to print uint16_t 4. Format string "#%2d %2d:%02d:%02d.%02d\n" in StopWatch::stopLapBtnEventHandler() requires at least 17 bytes. The 16-byte buffer would clearly be overrun if sprintf were used. --- src/components/datetime/DateTimeController.cpp | 4 ++-- src/displayapp/screens/StopWatch.cpp | 6 +++--- src/displayapp/screens/SystemInfo.cpp | 8 ++++---- src/displayapp/screens/Twos.cpp | 2 +- src/displayapp/screens/settings/SettingDisplay.cpp | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/components/datetime/DateTimeController.cpp b/src/components/datetime/DateTimeController.cpp index 9e9fb6e4fe..8d4a834e06 100644 --- a/src/components/datetime/DateTimeController.cpp +++ b/src/components/datetime/DateTimeController.cpp @@ -140,9 +140,9 @@ std::string DateTime::FormattedTime() { hour12 = (hour == 12) ? 12 : hour - 12; amPmStr = "PM"; } - sprintf(buff, "%i:%02i %s", hour12, minute, amPmStr); + snprintf(buff, sizeof(buff), "%i:%02i %s", hour12, minute, amPmStr); } else { - sprintf(buff, "%02i:%02i", hour, minute); + snprintf(buff, sizeof(buff), "%02i:%02i", hour, minute); } return std::string(buff); } diff --git a/src/displayapp/screens/StopWatch.cpp b/src/displayapp/screens/StopWatch.cpp index 239ebe3934..d2b811542f 100644 --- a/src/displayapp/screens/StopWatch.cpp +++ b/src/displayapp/screens/StopWatch.cpp @@ -198,11 +198,11 @@ void StopWatch::stopLapBtnEventHandler() { continue; } TimeSeparated_t times = convertTicksToTimeSegments(laps[i]); - char buffer[16]; + char buffer[17]; if (times.hours == 0) { - sprintf(buffer, "#%2d %2d:%02d.%02d\n", i + 1, times.mins, times.secs, times.hundredths); + snprintf(buffer, sizeof(buffer), "#%2d %2d:%02d.%02d\n", i + 1, times.mins, times.secs, times.hundredths); } else { - sprintf(buffer, "#%2d %2d:%02d:%02d.%02d\n", i + 1, times.hours, times.mins, times.secs, times.hundredths); + snprintf(buffer, sizeof(buffer), "#%2d %2d:%02d:%02d.%02d\n", i + 1, times.hours, times.mins, times.secs, times.hundredths); } lv_label_ins_text(lapText, LV_LABEL_POS_LAST, buffer); } diff --git a/src/displayapp/screens/SystemInfo.cpp b/src/displayapp/screens/SystemInfo.cpp index eea7593800..18fb7ad208 100644 --- a/src/displayapp/screens/SystemInfo.cpp +++ b/src/displayapp/screens/SystemInfo.cpp @@ -236,9 +236,9 @@ std::unique_ptr SystemInfo::CreateScreen4() { auto nb = uxTaskGetSystemState(tasksStatus, maxTaskCount, nullptr); std::sort(tasksStatus, tasksStatus + nb, sortById); for (uint8_t i = 0; i < nb && i < maxTaskCount; i++) { - char buffer[7] = {0}; + char buffer[11] = {0}; - sprintf(buffer, "%lu", tasksStatus[i].xTaskNumber); + snprintf(buffer, sizeof(buffer), "%lu", tasksStatus[i].xTaskNumber); lv_table_set_cell_value(infoTask, i + 1, 0, buffer); switch (tasksStatus[i].eCurrentState) { case eReady: @@ -262,9 +262,9 @@ std::unique_ptr SystemInfo::CreateScreen4() { lv_table_set_cell_value(infoTask, i + 1, 1, buffer); lv_table_set_cell_value(infoTask, i + 1, 2, tasksStatus[i].pcTaskName); if (tasksStatus[i].usStackHighWaterMark < 20) { - sprintf(buffer, "%d low", tasksStatus[i].usStackHighWaterMark); + snprintf(buffer, sizeof(buffer), "%" PRIu16 " low", tasksStatus[i].usStackHighWaterMark); } else { - sprintf(buffer, "%d", tasksStatus[i].usStackHighWaterMark); + snprintf(buffer, sizeof(buffer), "%" PRIu16, tasksStatus[i].usStackHighWaterMark); } lv_table_set_cell_value(infoTask, i + 1, 3, buffer); } diff --git a/src/displayapp/screens/Twos.cpp b/src/displayapp/screens/Twos.cpp index 8157a160b9..6f2eff40c4 100644 --- a/src/displayapp/screens/Twos.cpp +++ b/src/displayapp/screens/Twos.cpp @@ -242,7 +242,7 @@ void Twos::updateGridDisplay() { const unsigned int col = i % nCols; if (grid[row][col].value > 0) { char buffer[7]; - sprintf(buffer, "%d", grid[row][col].value); + snprintf(buffer, sizeof(buffer), "%u", grid[row][col].value); lv_table_set_cell_value(gridDisplay, row, col, buffer); } else { lv_table_set_cell_value(gridDisplay, row, col, ""); diff --git a/src/displayapp/screens/settings/SettingDisplay.cpp b/src/displayapp/screens/settings/SettingDisplay.cpp index 91f4d5902f..9abb4c74cf 100644 --- a/src/displayapp/screens/settings/SettingDisplay.cpp +++ b/src/displayapp/screens/settings/SettingDisplay.cpp @@ -46,7 +46,7 @@ SettingDisplay::SettingDisplay(Pinetime::Applications::DisplayApp* app, Pinetime char buffer[12]; for (unsigned int i = 0; i < options.size(); i++) { cbOption[i] = lv_checkbox_create(container1, nullptr); - sprintf(buffer, "%2ds", options[i] / 1000); + snprintf(buffer, sizeof(buffer), "%2" PRIu16 "s", options[i] / 1000); lv_checkbox_set_text(cbOption[i], buffer); cbOption[i]->user_data = this; lv_obj_set_event_cb(cbOption[i], event_handler); From f94a0c0eabd5f151ad8a9a857c8d0817bec7ed24 Mon Sep 17 00:00:00 2001 From: Mingjie Shen Date: Wed, 26 Apr 2023 22:39:36 -0400 Subject: [PATCH 2/3] SettingDisplay: Reduce buffer size Given that 2^16 / 1000 is 65, we can make the buffer only 3 chars. --- src/displayapp/screens/settings/SettingDisplay.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/displayapp/screens/settings/SettingDisplay.cpp b/src/displayapp/screens/settings/SettingDisplay.cpp index 9abb4c74cf..bd533e675b 100644 --- a/src/displayapp/screens/settings/SettingDisplay.cpp +++ b/src/displayapp/screens/settings/SettingDisplay.cpp @@ -43,7 +43,7 @@ SettingDisplay::SettingDisplay(Pinetime::Applications::DisplayApp* app, Pinetime lv_label_set_align(icon, LV_LABEL_ALIGN_CENTER); lv_obj_align(icon, title, LV_ALIGN_OUT_LEFT_MID, -10, 0); - char buffer[12]; + char buffer[4]; for (unsigned int i = 0; i < options.size(); i++) { cbOption[i] = lv_checkbox_create(container1, nullptr); snprintf(buffer, sizeof(buffer), "%2" PRIu16 "s", options[i] / 1000); From 8e1794574a6412f9227ad4f00c5167447b87bf59 Mon Sep 17 00:00:00 2001 From: Mingjie Shen Date: Wed, 26 Apr 2023 22:43:22 -0400 Subject: [PATCH 3/3] StopWatch: Add an extra space to the string without the hour so that it's just as long as with the hour. --- src/displayapp/screens/StopWatch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/displayapp/screens/StopWatch.cpp b/src/displayapp/screens/StopWatch.cpp index d2b811542f..f0359da4d4 100644 --- a/src/displayapp/screens/StopWatch.cpp +++ b/src/displayapp/screens/StopWatch.cpp @@ -200,7 +200,7 @@ void StopWatch::stopLapBtnEventHandler() { TimeSeparated_t times = convertTicksToTimeSegments(laps[i]); char buffer[17]; if (times.hours == 0) { - snprintf(buffer, sizeof(buffer), "#%2d %2d:%02d.%02d\n", i + 1, times.mins, times.secs, times.hundredths); + snprintf(buffer, sizeof(buffer), "#%2d %2d:%02d.%02d\n", i + 1, times.mins, times.secs, times.hundredths); } else { snprintf(buffer, sizeof(buffer), "#%2d %2d:%02d:%02d.%02d\n", i + 1, times.hours, times.mins, times.secs, times.hundredths); }