From: Timo Sirainen Date: Tue, 17 Oct 2017 14:39:32 +0000 (+0300) Subject: lib: printf_format_fix*() - Be over-strict in what format strings are allowed X-Git-Tag: 2.3.0.rc1~815 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=c398eca6b0fc6583687bd6fe2ee2dbcca2ae9387;p=thirdparty%2Fdovecot%2Fcore.git lib: printf_format_fix*() - Be over-strict in what format strings are allowed The checks could have been bypassed by some invalid format strings that were handled differently by the printf_format_fix*() code and libc. For example "%**%n" was passed through as ok, but glibc handled the %n in it. Found by cPanel Security Team. --- diff --git a/src/lib/printf-format-fix.c b/src/lib/printf-format-fix.c index 55fea4e45d..3e863105cb 100644 --- a/src/lib/printf-format-fix.c +++ b/src/lib/printf-format-fix.c @@ -35,23 +35,108 @@ fix_format_real(const char *fmt, const char *p, size_t *len_r) static const char * printf_format_fix_noalloc(const char *format, size_t *len_r) { - static const char *printf_skip_chars = "# -+'I.*0123456789hlLjzt"; - /* as a tiny optimization keep the most commonly used conversion - modifiers first, so strchr() stops early. */ - static const char *printf_allowed_conversions = "sudcioxXp%eEfFgGaA"; + /* NOTE: This function is overly strict in what it accepts. Some + format strings that are valid (and safe) in C99 will cause a panic + here. This is because we don't really need to support the weirdest + special cases, and we're also being extra careful not to pass + anything to the underlying libc printf, which might treat the string + differently than us and unexpectedly handling it as %n. For example + "%**%n" with glibc. */ + + /* Allow only the standard C99 flags. There are also <'> and flags, + but we don't really need them. And at worst if they're not supported + by the underlying printf, they could potentially be used to work + around our restrictions. */ + const char printf_flags[] = "#0- +"; + /* As a tiny optimization keep the most commonly used conversion + specifiers first, so strchr() stops early. */ + static const char *printf_specifiers = "sudcixXpoeEfFgGaA"; const char *ret, *p, *p2; + char *flag; p = ret = format; while ((p2 = strchr(p, '%')) != NULL) { + const unsigned int start_pos = p2 - format; + p = p2+1; - while (*p != '\0' && strchr(printf_skip_chars, *p) != NULL) + if (*p == '%') { + /* we'll be strict and allow %% only when there are no + optinal flags or modifiers. */ + p++; + continue; + } + /* 1) zero or more flags. We'll add a further restriction that + each flag can be used only once, since there's no need to + use them more than once, and some implementations might + add their own limits. */ + bool printf_flags_seen[N_ELEMENTS(printf_flags)] = { FALSE, }; + while (*p != '\0' && + (flag = strchr(printf_flags, *p)) != NULL) { + unsigned int flag_idx = flag - printf_flags; + + if (printf_flags_seen[flag_idx]) { + i_panic("Duplicate %% flag '%c' starting at #%u in '%s'", + *p, start_pos, format); + } + printf_flags_seen[flag_idx] = TRUE; p++; + } + + /* 2) Optional minimum field width */ + if (*p == '*') { + /* We don't bother supporting "*m$" - it's not used + anywhere and seems a bit dangerous. */ + p++; + } else if (*p >= '1' && *p <= '9') { + /* Limit to 4 digits - we'll never want more than that. + Some implementations might not handle long digits + correctly, or maybe even could be used for DoS due + to using too much CPU. */ + unsigned int i = 0; + do { + p++; + if (++i > 4) { + i_panic("Too large minimum field width starting at #%u in '%s'", + start_pos, format); + } + } while (*p >= '0' && *p <= '9'); + } + + /* 3) Optional precision */ + if (*p == '.') { + /* We don't bother supporting anything but numbers + here. 999 should be long enough precision. */ + unsigned int i = 0; + p++; + while (*p >= '0' && *p <= '9') { + if (++i > 3) { + i_panic("Too large precision starting at #%u in '%s'", + start_pos, format); + } + p++; + } + } + + /* 4) Optional length modifier */ + switch (*p) { + case 'h': + if (*++p == 'h') + p++; + break; + case 'l': + if (*++p == 'l') + p++; + break; + case 'L': + case 'j': + case 'z': + case 't': + p++; + break; + } - if (*p == '\0') { - i_panic("%% modifier missing in '%s'", format); - } else if (strchr(printf_allowed_conversions, *p) != NULL) { - /* allow & ignore */ - } else { + /* 5) conversion specifier */ + if (*p == '\0' || strchr(printf_specifiers, *p) == NULL) { switch (*p) { case 'n': i_panic("%%n modifier used"); @@ -60,8 +145,12 @@ printf_format_fix_noalloc(const char *format, size_t *len_r) i_panic("%%m used twice"); ret = fix_format_real(format, p-1, len_r); break; + case '\0': + i_panic("Missing %% specifier starting at #%u in '%s'", + start_pos, format); default: - i_panic("Unsupported %%%c modifier", *p); + i_panic("Unsupported 0x%02x specifier starting at #%u in '%s'", + *p, start_pos, format); } } p++; diff --git a/src/lib/test-printf-format-fix.c b/src/lib/test-printf-format-fix.c index d75639b723..0e9b8caab6 100644 --- a/src/lib/test-printf-format-fix.c +++ b/src/lib/test-printf-format-fix.c @@ -17,7 +17,14 @@ static void test_unchanged() { static const char *tests[] = { "Hello world", - "Embedded %%, %u, %f, etc. are OK", + "Embedded %%, %u, %f, %s, etc. are OK", + "Allow %#0- +s flags", + "duplicate flags in different args %0-123s %0-123s", + "Minimum length %9999s", + "Precision %.999s", + "Precision %1.999s", + "Length modifiers %hd %hhd %ld %lld %Lg %jd %zd %td", + "Specifiers %s %u %d %c %i %x %X %p %o %e %E %f %F %g %G %a %A", "%%doesn't cause confusion in %%m and %%n", }; unsigned int i; @@ -103,6 +110,16 @@ enum fatal_test_state fatal_printf_format_fix(unsigned int stage) "%m allowed once, but not twice: %m", "%m must not obscure a later %n", "definitely can't have a tailing %", + "Evil %**%n", + "Evil %*#%99999$s", + "No weird %% with %0%", + "No duplicate modifiers %00s", + "Minimum length can't be too long %10000s", + "Minimum length doesn't support %*1$s", + "Precision can't be too long %.1000s", + "Precision can't be too long %1.1000s", + "Precision doesn't support %1.-1s", + "Precision doesn't support %1.*s", }; if(stage >= N_ELEMENTS(fatals)) {