From 632b0ce5e68ce32ade90382cb64fbb8d1e75090d Mon Sep 17 00:00:00 2001 From: dequis Date: Wed, 17 May 2017 06:18:49 -0300 Subject: [PATCH] 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/core/misc.c | 119 ++++++++++++++++++++++++++++++----- src/core/misc.h | 1 + src/core/special-vars.c | 9 ++- src/fe-common/core/formats.c | 28 ++++++--- 4 files changed, 130 insertions(+), 27 deletions(-) diff --git a/src/core/misc.c b/src/core/misc.c index d8437430..0f038cbb 100644 --- a/src/core/misc.c +++ b/src/core/misc.c @@ -750,10 +750,42 @@ int nearest_power(int num) return n; } -int parse_time_interval(const char *time, int *msecs) +/* Parses unsigned integers from strings with decent error checking. + * Returns true on success, false otherwise (overflow, no valid number, etc) + * There's a 31 bit limit so the output can be assigned to signed positive ints */ +int parse_uint(const char *nptr, char **endptr, int base, guint *number) +{ + char *endptr_; + gulong parsed; + + /* strtoul accepts whitespace and plus/minus signs, for some reason */ + if (!i_isdigit(*nptr)) { + return FALSE; + } + + errno = 0; + parsed = strtoul(nptr, &endptr_, base); + + if (errno || endptr_ == nptr || parsed >= (1U << 31)) { + return FALSE; + } + + if (endptr) { + *endptr = endptr_; + } + + if (number) { + *number = (guint) parsed; + } + + return TRUE; +} + +static int parse_time_interval_uint(const char *time, guint *msecs) { const char *desc; - int number, sign, len, ret, digits; + guint number; + int sign, len, ret, digits; *msecs = 0; @@ -769,8 +801,11 @@ int parse_time_interval(const char *time, int *msecs) } for (;;) { if (i_isdigit(*time)) { - number = number*10 + (*time - '0'); - time++; + char *endptr; + if (!parse_uint(time, &endptr, 10, &number)) { + return FALSE; + } + time = endptr; digits = TRUE; continue; } @@ -835,10 +870,11 @@ int parse_time_interval(const char *time, int *msecs) return ret; } -int parse_size(const char *size, int *bytes) +static int parse_size_uint(const char *size, guint *bytes) { const char *desc; - int number, len; + guint number, multiplier, limit; + int len; *bytes = 0; @@ -846,8 +882,11 @@ int parse_size(const char *size, int *bytes) number = 0; while (*size != '\0') { if (i_isdigit(*size)) { - number = number*10 + (*size - '0'); - size++; + char *endptr; + if (!parse_uint(size, &endptr, 10, &number)) { + return FALSE; + } + size = endptr; continue; } @@ -869,14 +908,31 @@ int parse_size(const char *size, int *bytes) return FALSE; } - if (g_ascii_strncasecmp(desc, "gbytes", len) == 0) - *bytes += number * 1024*1024*1024; - if (g_ascii_strncasecmp(desc, "mbytes", len) == 0) - *bytes += number * 1024*1024; - if (g_ascii_strncasecmp(desc, "kbytes", len) == 0) - *bytes += number * 1024; - if (g_ascii_strncasecmp(desc, "bytes", len) == 0) - *bytes += number; + multiplier = 0; + limit = 0; + + if (g_ascii_strncasecmp(desc, "gbytes", len) == 0) { + multiplier = 1U << 30; + limit = 2U << 0; + } + if (g_ascii_strncasecmp(desc, "mbytes", len) == 0) { + multiplier = 1U << 20; + limit = 2U << 10; + } + if (g_ascii_strncasecmp(desc, "kbytes", len) == 0) { + multiplier = 1U << 10; + limit = 2U << 20; + } + if (g_ascii_strncasecmp(desc, "bytes", len) == 0) { + multiplier = 1; + limit = 2U << 30; + } + + if (limit && number > limit) { + return FALSE; + } + + *bytes += number * multiplier; /* skip punctuation */ while (*size != '\0' && i_ispunct(*size)) @@ -886,6 +942,37 @@ int parse_size(const char *size, int *bytes) return TRUE; } +int parse_size(const char *size, int *bytes) +{ + guint bytes_; + int ret; + + ret = parse_size_uint(size, &bytes_); + + if (bytes_ > (1U << 31)) { + return FALSE; + } + + *bytes = bytes_; + return ret; +} + +int parse_time_interval(const char *time, int *msecs) +{ + guint msecs_; + int ret; + + ret = parse_time_interval_uint(time, &msecs_); + + if (msecs_ > (1U << 31)) { + return FALSE; + } + + *msecs = msecs_; + return ret; +} + + char *ascii_strup(char *str) { char *s; diff --git a/src/core/misc.h b/src/core/misc.h index 00637da0..375744db 100644 --- a/src/core/misc.h +++ b/src/core/misc.h @@ -71,6 +71,7 @@ int expand_escape(const char **data); int nearest_power(int num); /* Returns TRUE / FALSE */ +int parse_uint(const char *nptr, char **endptr, int base, guint *number); int parse_time_interval(const char *time, int *msecs); int parse_size(const char *size, int *bytes); diff --git a/src/core/special-vars.c b/src/core/special-vars.c index 6ca080fc..aaf8da8f 100644 --- a/src/core/special-vars.c +++ b/src/core/special-vars.c @@ -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 != ']') { 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) {