From c02634c161ce3af373d4ea4728668170fbcbcaf7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= <hrydgard@gmail.com>
Date: Fri, 28 Apr 2023 20:26:08 +0200
Subject: [PATCH] Add some paranoid format string checking in popup slider
 dialogs

---
 Common/Buffer.cpp          |  2 +-
 Common/UI/PopupScreens.cpp | 54 ++++++++++++++++++++++++++++----------
 Common/UI/PopupScreens.h   |  4 +--
 3 files changed, 43 insertions(+), 17 deletions(-)

diff --git a/Common/Buffer.cpp b/Common/Buffer.cpp
index 58a485593c..25af9607d1 100644
--- a/Common/Buffer.cpp
+++ b/Common/Buffer.cpp
@@ -42,7 +42,7 @@ void Buffer::Append(const Buffer &other) {
 void Buffer::AppendValue(int value) {
 	char buf[16];
 	// This is slow.
-	sprintf(buf, "%i", value);
+	snprintf(buf, sizeof(buf), "%i", value);
 	Append(buf);
 }
 
diff --git a/Common/UI/PopupScreens.cpp b/Common/UI/PopupScreens.cpp
index 6ee96b9bda..1c4b2277bc 100644
--- a/Common/UI/PopupScreens.cpp
+++ b/Common/UI/PopupScreens.cpp
@@ -146,13 +146,13 @@ std::string PopupMultiChoice::ValueText() const {
 
 PopupSliderChoice::PopupSliderChoice(int *value, int minValue, int maxValue, int defaultValue, const std::string &text, ScreenManager *screenManager, const std::string &units, LayoutParams *layoutParams)
 	: AbstractChoiceWithValueDisplay(text, layoutParams), value_(value), minValue_(minValue), maxValue_(maxValue), defaultValue_(defaultValue), step_(1), units_(units), screenManager_(screenManager) {
-	fmt_ = "%i";
+	fmt_ = "%d";
 	OnClick.Handle(this, &PopupSliderChoice::HandleClick);
 }
 
 PopupSliderChoice::PopupSliderChoice(int *value, int minValue, int maxValue, int defaultValue, const std::string &text, int step, ScreenManager *screenManager, const std::string &units, LayoutParams *layoutParams)
 	: AbstractChoiceWithValueDisplay(text, layoutParams), value_(value), minValue_(minValue), maxValue_(maxValue), defaultValue_(defaultValue), step_(step), units_(units), screenManager_(screenManager) {
-	fmt_ = "%i";
+	fmt_ = "%d";
 	OnClick.Handle(this, &PopupSliderChoice::HandleClick);
 }
 
@@ -191,18 +191,41 @@ EventReturn PopupSliderChoice::HandleChange(EventParams &e) {
 	return EVENT_DONE;
 }
 
+static bool IsValidNumberFormatString(const std::string &s) {
+	if (s.empty())
+		return false;
+	size_t percentCount = 0;
+	for (int i = 0; i < (int)s.size(); i++) {
+		if (s[i] == '%') {
+			percentCount++;
+			if (i < s.size() - 1) {
+				if (s[i] == 's')
+					return false;
+			}
+		}
+	}
+	return percentCount == 1;
+}
+
 std::string PopupSliderChoice::ValueText() const {
 	// Always good to have space for Unicode.
 	char temp[256];
+	temp[0] = '\0';
 	if (zeroLabel_.size() && *value_ == 0) {
-		strcpy(temp, zeroLabel_.c_str());
+		truncate_cpy(temp, zeroLabel_.c_str());
 	} else if (negativeLabel_.size() && *value_ < 0) {
-		strcpy(temp, negativeLabel_.c_str());
+		truncate_cpy(temp, negativeLabel_.c_str());
 	} else {
-		sprintf(temp, fmt_, *value_);
+		// Would normally be dangerous to have user-controlled format strings!
+		// However, let's check that there's only one % sign, and that it's not followed by an S.
+		// Also, these strings are from translations, which are kinda-fixed (though can be modified in theory).
+		if (IsValidNumberFormatString(fmt_)) {
+			snprintf(temp, sizeof(temp), fmt_.c_str(), *value_);
+		} else {
+			truncate_cpy(temp, "(translation error)");
+		}
 	}
-
-	return temp;
+	return std::string(temp);
 }
 
 EventReturn PopupSliderChoiceFloat::HandleClick(EventParams &e) {
@@ -229,12 +252,14 @@ EventReturn PopupSliderChoiceFloat::HandleChange(EventParams &e) {
 
 std::string PopupSliderChoiceFloat::ValueText() const {
 	char temp[256];
+	temp[0] = '\0';
 	if (zeroLabel_.size() && *value_ == 0.0f) {
-		strcpy(temp, zeroLabel_.c_str());
+		truncate_cpy(temp, zeroLabel_.c_str());
+	} else if (IsValidNumberFormatString(fmt_.c_str())) {
+		snprintf(temp, sizeof(temp), fmt_.c_str(), *value_);
 	} else {
-		sprintf(temp, fmt_, *value_);
+		snprintf(temp, sizeof(temp), "%0.2f", *value_);
 	}
-
 	return temp;
 }
 
@@ -282,8 +307,8 @@ EventReturn SliderPopupScreen::OnTextChange(EventParams &params) {
 }
 
 void SliderPopupScreen::UpdateTextBox() {
-	char temp[64];
-	sprintf(temp, "%d", sliderValue_);
+	char temp[128];
+	snprintf(temp, sizeof(temp), "%d", sliderValue_);
 	edit_->SetText(temp);
 }
 
@@ -295,6 +320,7 @@ void SliderPopupScreen::CreatePopupContents(UI::ViewGroup *parent) {
 	sliderValue_ = *value_;
 	if (disabled_ && sliderValue_ < 0)
 		sliderValue_ = 0;
+
 	LinearLayout *vert = parent->Add(new LinearLayout(ORIENT_VERTICAL, new LinearLayoutParams(UI::Margins(10, 10))));
 	slider_ = new Slider(&sliderValue_, minValue_, maxValue_, new LinearLayoutParams(UI::Margins(10, 10)));
 	slider_->OnChange.Handle(this, &SliderPopupScreen::OnSliderChange);
@@ -417,8 +443,8 @@ EventReturn SliderFloatPopupScreen::OnSliderChange(EventParams &params) {
 }
 
 void SliderFloatPopupScreen::UpdateTextBox() {
-	char temp[64];
-	sprintf(temp, "%0.3f", sliderValue_);
+	char temp[128];
+	snprintf(temp, sizeof(temp), "%0.3f", sliderValue_);
 	edit_->SetText(temp);
 }
 
diff --git a/Common/UI/PopupScreens.h b/Common/UI/PopupScreens.h
index 2cfbb23979..acb37a030a 100644
--- a/Common/UI/PopupScreens.h
+++ b/Common/UI/PopupScreens.h
@@ -309,7 +309,7 @@ private:
 	int maxValue_;
 	int defaultValue_;
 	int step_;
-	const char *fmt_;
+	std::string fmt_;
 	std::string zeroLabel_;
 	std::string negativeLabel_;
 	std::string units_;
@@ -348,7 +348,7 @@ private:
 	float maxValue_;
 	float defaultValue_;
 	float step_;
-	const char *fmt_;
+	std::string fmt_;
 	std::string zeroLabel_;
 	std::string units_;
 	ScreenManager *screenManager_;