[SECURITY PATCH 114/117] kern/misc: Add function to check printf() format against expected format

[Daniel Kiper]

From: Thomas Frauendorfer | Miray Software <tf@miray.de>

The grub_printf_fmt_check() function parses the arguments of an untrusted
printf() format and an expected printf() format and then compares the
arguments counts and arguments types. The arguments count in the untrusted
format string must be less or equal to the arguments count in the expected
format string and both arguments types must match.

To do this the parse_printf_arg_fmt() helper function is extended in the
following way:

  1. Add a return value to report errors to the grub_printf_fmt_check().

  2. Add the fmt_check argument to enable stricter format verification:
     - the function expects that arguments definitions are always
       terminated by a supported conversion specifier.
     - positional parameters, "$", are not allowed, as they cannot be
       validated correctly with the current implementation. For example
       "%s%1$d" would assign the first args entry twice while leaving the
       second one unchanged.
     - Return an error if preallocated space in args is too small and
       allocation fails for the needed size. The grub_printf_fmt_check()
       should verify all arguments. So, if validation is not possible for
       any reason it should return an error.
     This also adds a case entry to handle "%%", which is the escape
     sequence to print "%" character.

  3. Add the max_args argument to check for the maximum allowed arguments
     count in a printf() string. This should be set to the arguments count
     of the expected format. Then the parse_printf_arg_fmt() function will
     return an error if the arguments count is exceeded.

The two additional arguments allow us to use parse_printf_arg_fmt() in
printf() and grub_printf_fmt_check() calls.

When parse_printf_arg_fmt() is used by grub_printf_fmt_check() the
function parse user provided untrusted format string too. So, in
that case it is better to be too strict than too lenient.

Signed-off-by: Thomas Frauendorfer | Miray Software <tf@miray.de>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 grub-core/kern/misc.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++---
 include/grub/misc.h   | 16 ++++++++++
 2 files changed, 94 insertions(+), 4 deletions(-)

diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 074728b2b..3af336ee2 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -645,8 +645,26 @@ grub_lltoa (char *str, int c, unsigned long long n)
   return p;
 }
 
-static void
-parse_printf_arg_fmt (const char *fmt0, struct printf_args *args)
+/*
+ * Parse printf() fmt0 string into args arguments.
+ *
+ * The parsed arguments are either used by a printf() function to format the 
fmt0
+ * string or they are used to compare a format string from an untrusted source
+ * against a format string with expected arguments.
+ *
+ * When the fmt_check is set to !0, e.g. 1, then this function is executed in
+ * printf() format check mode. This enforces stricter rules for parsing the
+ * fmt0 to limit exposure to possible errors in printf() handling. It also
+ * disables positional parameters, "$", because some formats, e.g "%s%1$d",
+ * cannot be validated with the current implementation.
+ *
+ * The max_args allows to set a maximum number of accepted arguments. If the 
fmt0
+ * string defines more arguments than the max_args then the 
parse_printf_arg_fmt()
+ * function returns an error. This is currently used for format check only.
+ */
+static grub_err_t
+parse_printf_arg_fmt (const char *fmt0, struct printf_args *args,
+                     int fmt_check, grub_size_t max_args)
 {
   const char *fmt;
   char c;
@@ -673,7 +691,12 @@ parse_printf_arg_fmt (const char *fmt0, struct printf_args 
*args)
        fmt++;
 
       if (*fmt == '$')
-       fmt++;
+       {
+         if (fmt_check)
+           return grub_error (GRUB_ERR_BAD_ARGUMENT,
+                              "positional arguments are not supported");
+         fmt++;
+       }
 
       if (*fmt =='-')
        fmt++;
@@ -705,9 +728,19 @@ parse_printf_arg_fmt (const char *fmt0, struct printf_args 
*args)
        case 's':
          args->count++;
          break;
+       case '%':
+         /* "%%" is the escape sequence to output "%". */
+         break;
+       default:
+         if (fmt_check)
+           return grub_error (GRUB_ERR_BAD_ARGUMENT, "unexpected format");
+         break;
        }
     }
 
+  if (fmt_check && args->count > max_args)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "too many arguments");
+
   if (args->count <= ARRAY_SIZE (args->prealloc))
     args->ptr = args->prealloc;
   else
@@ -715,6 +748,9 @@ parse_printf_arg_fmt (const char *fmt0, struct printf_args 
*args)
       args->ptr = grub_calloc (args->count, sizeof (args->ptr[0]));
       if (!args->ptr)
        {
+         if (fmt_check)
+           return grub_errno;
+
          grub_errno = GRUB_ERR_NONE;
          args->ptr = args->prealloc;
          args->count = ARRAY_SIZE (args->prealloc);
@@ -806,6 +842,8 @@ parse_printf_arg_fmt (const char *fmt0, struct printf_args 
*args)
          break;
        }
     }
+
+  return GRUB_ERR_NONE;
 }
 
 static void
@@ -813,7 +851,7 @@ parse_printf_args (const char *fmt0, struct printf_args 
*args, va_list args_in)
 {
   grub_size_t n;
 
-  parse_printf_arg_fmt (fmt0, args);
+  parse_printf_arg_fmt (fmt0, args, 0, 0);
 
   for (n = 0; n < args->count; n++)
     switch (args->ptr[n].type)
@@ -1121,6 +1159,42 @@ grub_xasprintf (const char *fmt, ...)
   return ret;
 }
 
+grub_err_t
+grub_printf_fmt_check (const char *fmt, const char *fmt_expected)
+{
+  struct printf_args args_expected, args_fmt;
+  grub_err_t ret;
+  grub_size_t n;
+
+  if (fmt == NULL || fmt_expected == NULL)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "invalid format");
+
+  ret = parse_printf_arg_fmt (fmt_expected, &args_expected, 1, GRUB_SIZE_MAX);
+  if (ret != GRUB_ERR_NONE)
+    return ret;
+
+  /* Limit parsing to the number of expected arguments. */
+  ret = parse_printf_arg_fmt (fmt, &args_fmt, 1, args_expected.count);
+  if (ret != GRUB_ERR_NONE)
+    {
+      free_printf_args (&args_expected);
+      return ret;
+    }
+
+  for (n = 0; n < args_fmt.count; n++)
+    if (args_fmt.ptr[n].type != args_expected.ptr[n].type)
+     {
+       ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "arguments types do not 
match");
+       break;
+     }
+
+  free_printf_args (&args_expected);
+  free_printf_args (&args_fmt);
+
+  return ret;
+}
+
+
 /* Abort GRUB. This function does not return.  */
 static void __attribute__ ((noreturn))
 grub_abort (void)
diff --git a/include/grub/misc.h b/include/grub/misc.h
index 1d6124a7a..7d2b55196 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -459,6 +459,22 @@ grub_error_load (const struct grub_error_saved *save)
   grub_errno = save->grub_errno;
 }
 
+/*
+ * grub_printf_fmt_checks() a fmt string for printf() against an expected
+ * format. It is intended for cases where the fmt string could come from
+ * an outside source and cannot be trusted.
+ *
+ * While expected fmt accepts a printf() format string it should be kept
+ * as simple as possible. The printf() format strings with positional
+ * parameters are NOT accepted, neither for fmt nor for fmt_expected.
+ *
+ * The fmt is accepted if it has equal or less arguments than fmt_expected
+ * and if the type of all arguments match.
+ *
+ * Returns GRUB_ERR_NONE if fmt is acceptable.
+ */
+grub_err_t EXPORT_FUNC (grub_printf_fmt_check) (const char *fmt, const char 
*fmt_expected);
+
 #if BOOT_TIME_STATS
 struct grub_boot_time
 {
-- 
2.11.0