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.
This commit is contained in:
dequis 2017-05-17 06:18:49 -03:00
commit 632b0ce5e6
4 changed files with 130 additions and 27 deletions

View file

@ -275,6 +275,8 @@ static char *get_special_value(char **cmd, SERVER_REC *server, void *item,
static int get_alignment_args(char **data, int *align, int *flags, char *pad)
{
char *str;
char *endptr;
guint align_;
*align = 0;
*flags = ALIGN_CUT|ALIGN_PAD;
@ -295,10 +297,11 @@ static int get_alignment_args(char **data, int *align, int *flags, char *pad)
return FALSE; /* expecting number */
/* get the alignment size */
while (i_isdigit(*str)) {
*align = (*align) * 10 + (*str-'0');
str++;
if (!parse_uint(str, &endptr, 10, &align_)) {
return FALSE;
}
str = endptr;
*align = align_;
/* get the pad character */
while (*str != '\0' && *str != ']') {