| From 83603bea6ce8fdff5ab3fbc4c9e592a8c71a8706 Mon Sep 17 00:00:00 2001 |
| From: Thomas Frauendorfer | Miray Software <tf@miray.de> |
| Date: Thu, 4 Feb 2021 19:02:33 +0100 |
| Subject: [PATCH] kern/misc: Add function to check printf() format against |
| expected format |
| |
| 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> |
| Signed-off-by: Stefan SΓΈrensen <stefan.sorensen@spectralink.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 22417f7..90317b6 100644 |
| --- a/grub-core/kern/misc.c |
| +++ b/grub-core/kern/misc.c |
| @@ -632,8 +632,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; |
| @@ -660,7 +678,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++; |
| @@ -691,9 +714,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 |
| @@ -701,6 +734,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); |
| @@ -791,6 +827,8 @@ parse_printf_arg_fmt (const char *fmt0, struct printf_args *args) |
| break; |
| } |
| } |
| + |
| + return GRUB_ERR_NONE; |
| } |
| |
| static void |
| @@ -798,7 +836,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) |
| @@ -1105,6 +1143,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 ee48eb7..d1c5709 100644 |
| --- a/include/grub/misc.h |
| +++ b/include/grub/misc.h |
| @@ -440,6 +440,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.14.2 |
| |