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 55fea4e45d05ba0a786c205f762e84650b1f2ff9..3e863105cbd1772f4fa2ba42e24137c4c9184a93 100644 (file)
--- 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 <I> 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 d75639b723d74bdacd44f5a5d74a023a98ec6aba..0e9b8caab6353cdb2776b3268723221f6323f02d 100644 (file)
--- 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)) {