Add parse_uint function to improve integer overflow handling

Originally found by oss-fuzz (issue 525) in get_ansi_color using ubsan.
After a lot of analysis I'm 99% sure this isn't security relevant so
it's fine to handle this publicly.

The fix is mainly adding a function that does it right and use it
everywhere. This is harder than it seems because the strtol() family of
functions doesn't have the friendliest of interfaces.

Aside from get_ansi_color(), there were other pieces of code that used
the same (out*10+(*in-'0')) pattern, like the parse_size() and
parse_time_interval() functions, which are mostly used for settings.
Those are interesting cases, since they multiply the parsed number
(resulting in more overflows) and they write to a signed integer
parameter (which can accidentally make the uints negative without UB)

Thanks to Pascal Cuoq for enlightening me about the undefined behavior
of parse_size (and, in particular, the implementation-defined behavior
of one of the WIP versions of this commit, where something like signed
integer overflow happened, but it was legal). Also for writing
tis-interpreter, which is better than ubsan to verify these things.

1 files changed, 20 insertions, 8 deletions

diff --git a/src/fe-common/core/formats.c b/src/fe-common/core/formats.c
index 17c13a97..005e6fb7 100644
--- a/src/fe-common/core/formats.c
+++ b/src/fe-common/core/formats.c
@@ -33,6 +33,7 @@
 #include "themes.h"
 #include "recode.h"
 #include "utf8.h"
+#include "misc.h"
 
 static const char *format_backs = "04261537";
 static const char *format_fores = "kbgcrmyw";
@@ -870,8 +871,9 @@ static const char *get_ansi_color(THEME_REC *theme, const char *str,
 {
 	static char ansitab[8] = { 0, 4, 2, 6, 1, 5, 3, 7 };
 	const char *start;
-	int fg, bg, flags, num, i;
-	unsigned int num2;
+	char *endptr;
+	int fg, bg, flags, i;
+	guint num, num2;
 
 	if (*str != '[')
 		return str;
@@ -886,8 +888,10 @@ static const char *get_ansi_color(THEME_REC *theme, const char *str,
 		if (*str == '\0') return start;
 
 		if (i_isdigit(*str)) {
-			num = num*10 + (*str-'0');
-			continue;
+			if (!parse_uint(str, &endptr, 10, &num)) {
+				return start;
+			}
+			str = endptr;
 		}
 
 		if (*str != ';' && *str != 'm')
@@ -958,8 +962,12 @@ static const char *get_ansi_color(THEME_REC *theme, const char *str,
 			/* ANSI indexed color or RGB color */
 			if (*str != ';') break;
 			str++;
-			for (num2 = 0; i_isdigit(*str); str++)
-				num2 = num2*10 + (*str-'0');
+
+			if (!parse_uint(str, &endptr, 10, &num2)) {
+				return start;
+			}
+			str = endptr;
+
 			if (*str == '\0') return start;
 
 			switch (num2) {
@@ -1006,8 +1014,12 @@ static const char *get_ansi_color(THEME_REC *theme, const char *str,
 				/* indexed */
 				if (*str != ';') break;
 				str++;
-				for (num2 = 0; i_isdigit(*str); str++)
-					num2 = num2*10 + (*str-'0');
+
+				if (!parse_uint(str, &endptr, 10, &num2)) {
+					return start;
+				}
+				str = endptr;
+
 				if (*str == '\0') return start;
 
 				if (num == 38) {