From 632b0ce5e68ce32ade90382cb64fbb8d1e75090d Mon Sep 17 00:00:00 2001 From: dequis Date: Wed, 17 May 2017 06:18:49 -0300 Subject: 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. --- src/fe-common/core/formats.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) (limited to 'src/fe-common') 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) { -- cgit v1.2.3