Re: [SECURITY PATCH 100/117] kern/parser: Fix a stack buffer overflow

[Chris Coulson]

Hi,

Sorry for taking a while to look at this.

On 10/06/2021 12:55, Paul Menzel wrote:
> Dear Daniel, dear Chris,
> 
> 
> Am 02.03.21 um 19:01 schrieb Daniel Kiper:
>> From: Chris Coulson <chris.coulson@canonical.com>
>>
>> grub_parser_split_cmdline() expands variable names present in the
>> supplied
>> command line in to their corresponding variable contents and uses a 1 kiB
>> stack buffer for temporary storage without sufficient bounds checking. If
>> the function is called with a command line that references a variable
>> with
>> a sufficiently large payload, it is possible to overflow the stack
>> buffer via tab completion, corrupt the stack frame and potentially
>> control execution.
>>
>> Fixes: CVE-2020-27749
>>
>> Reported-by: Chris Coulson <chris.coulson@canonical.com>
>> Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
>> Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
>> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
>> ---
>>   grub-core/kern/parser.c | 110
>> +++++++++++++++++++++++++++++-------------------
>>   1 file changed, 67 insertions(+), 43 deletions(-)
>>
>> diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c
>> index e010eaa1f..6ab7aa427 100644
>> --- a/grub-core/kern/parser.c
>> +++ b/grub-core/kern/parser.c
>> @@ -18,6 +18,7 @@
>>    */
>>     #include <grub/parser.h>
>> +#include <grub/buffer.h>
>>   #include <grub/env.h>
>>   #include <grub/misc.h>
>>   #include <grub/mm.h>
>> @@ -107,8 +108,8 @@ check_varstate (grub_parser_state_t s)
>>   }
>>     -static void
>> -add_var (char *varname, char **bp, char **vp,
>> +static grub_err_t
>> +add_var (grub_buffer_t varname, grub_buffer_t buf,
>>        grub_parser_state_t state, grub_parser_state_t newstate)
>>   {
>>     const char *val;
>> @@ -116,31 +117,41 @@ add_var (char *varname, char **bp, char **vp,
>>     /* Check if a variable was being read in and the end of the name
>>        was reached.  */
>>     if (!(check_varstate (state) && !check_varstate (newstate)))
>> -    return;
>> +    return GRUB_ERR_NONE;
>>   -  *((*vp)++) = '\0';
>> -  val = grub_env_get (varname);
>> -  *vp = varname;
>> +  if (grub_buffer_append_char (varname, '\0') != GRUB_ERR_NONE)
>> +    return grub_errno;
>> +
>> +  val = grub_env_get ((const char *) grub_buffer_peek_data (varname));
>> +  grub_buffer_reset (varname);
>>     if (!val)
>> -    return;
>> +    return GRUB_ERR_NONE;
>>       /* Insert the contents of the variable in the buffer.  */
>> -  for (; *val; val++)
>> -    *((*bp)++) = *val;
>> +  return grub_buffer_append_data (buf, val, grub_strlen (val));
>>   }
>>   -static void
>> -terminate_arg (char *buffer, char **bp, int *argc)
>> +static grub_err_t
>> +terminate_arg (grub_buffer_t buffer, int *argc)
>>   {
>> -  if (*bp != buffer && *((*bp) - 1) != '\0')
>> -    {
>> -      *((*bp)++) = '\0';
>> -      (*argc)++;
>> -    }
>> +  grub_size_t unread = grub_buffer_get_unread_bytes (buffer);
>> +
>> +  if (unread == 0)
>> +    return GRUB_ERR_NONE;
>> +
>> +  if (*(const char *) grub_buffer_peek_data_at (buffer, unread - 1)
>> == '\0')
>> +    return GRUB_ERR_NONE;
>> +
>> +  if (grub_buffer_append_char (buffer, '\0') != GRUB_ERR_NONE)
>> +    return grub_errno;
>> +
>> +  (*argc)++;
>> +
>> +  return GRUB_ERR_NONE;
>>   }
>>     static grub_err_t
>> -process_char (char c, char *buffer, char **bp, char *varname, char **vp,
>> +process_char (char c, grub_buffer_t buffer, grub_buffer_t varname,
>>             grub_parser_state_t state, int *argc,
>>             grub_parser_state_t *newstate)
>>   {
>> @@ -153,12 +164,13 @@ process_char (char c, char *buffer, char **bp,
>> char *varname, char **vp,
>>      * not describe the variable anymore, write the variable to
>>      * the buffer.
>>      */
>> -  add_var (varname, bp, vp, state, *newstate);
>> +  if (add_var (varname, buffer, state, *newstate) != GRUB_ERR_NONE)
>> +    return grub_errno;
>>       if (check_varstate (*newstate))
>>       {
>>         if (use)
>> -    *((*vp)++) = use;
>> +        return grub_buffer_append_char (varname, use);
>>       }
>>     else if (*newstate == GRUB_PARSER_STATE_TEXT &&
>>          state != GRUB_PARSER_STATE_ESC && grub_isspace (use))
>> @@ -167,10 +179,10 @@ process_char (char c, char *buffer, char **bp,
>> char *varname, char **vp,
>>          * Don't add more than one argument if multiple
>>          * spaces are used.
>>          */
>> -      terminate_arg (buffer, bp, argc);
>> +      return terminate_arg (buffer, argc);
>>       }
>>     else if (use)
>> -    *((*bp)++) = use;
>> +    return grub_buffer_append_char (buffer, use);
>>       return GRUB_ERR_NONE;
>>   }
>> @@ -181,19 +193,22 @@ grub_parser_split_cmdline (const char *cmdline,
>>                  int *argc, char ***argv)
>>   {
>>     grub_parser_state_t state = GRUB_PARSER_STATE_TEXT;
>> -  /* XXX: Fixed size buffer, perhaps this buffer should be dynamically
>> -     allocated.  */
>> -  char buffer[1024];
>> -  char *bp = buffer;
>> +  grub_buffer_t buffer, varname;
>>     char *rd = (char *) cmdline;
>>     char *rp = rd;
>> -  char varname[200];
>> -  char *vp = varname;
>> -  char *args;
>>     int i;
>>       *argc = 0;
>>     *argv = NULL;
>> +
>> +  buffer = grub_buffer_new (1024);
>> +  if (buffer == NULL)
>> +    return grub_errno;
>> +
>> +  varname = grub_buffer_new (200);
>> +  if (varname == NULL)
>> +    goto fail;
>> +
>>     do
>>       {
>>         if (rp == NULL || *rp == '\0')
>> @@ -219,7 +234,7 @@ grub_parser_split_cmdline (const char *cmdline,
>>       {
>>         grub_parser_state_t newstate;
>>   -      if (process_char (*rp, buffer, &bp, varname, &vp, state, argc,
>> +      if (process_char (*rp, buffer, varname, state, argc,
>>                   &newstate) != GRUB_ERR_NONE)
>>           goto fail;
>>   @@ -230,10 +245,12 @@ grub_parser_split_cmdline (const char *cmdline,
>>       /* A special case for when the last character was part of a
>>        variable.  */
>> -  add_var (varname, &bp, &vp, state, GRUB_PARSER_STATE_TEXT);
>> +  if (add_var (varname, buffer, state, GRUB_PARSER_STATE_TEXT) !=
>> GRUB_ERR_NONE)
>> +    goto fail;
>>       /* Ensure that the last argument is terminated. */
>> -  terminate_arg (buffer, &bp, argc);
>> +  if (terminate_arg (buffer, argc) != GRUB_ERR_NONE)
>> +    goto fail;
>>       /* If there are no args, then we're done. */
>>     if (!*argc)
>> @@ -242,38 +259,45 @@ grub_parser_split_cmdline (const char *cmdline,
>>         goto out;
>>       }
>>   -  /* Reserve memory for the return values.  */
>> -  args = grub_malloc (bp - buffer);
>> -  if (!args)
>> -    goto fail;
>> -  grub_memcpy (args, buffer, bp - buffer);
>> -
>>     *argv = grub_calloc (*argc + 1, sizeof (char *));
>>     if (!*argv)
>>       goto fail;
>>       /* The arguments are separated with 0's, setup argv so it points to
>>        the right values.  */
>> -  bp = args;
>>     for (i = 0; i < *argc; i++)
>>       {
>> -      (*argv)[i] = bp;
>> -      while (*bp)
>> -    bp++;
>> -      bp++;
>> +      char *arg;
>> +
>> +      if (i > 0)
>> +    {
>> +      if (grub_buffer_advance_read_pos (buffer, 1) != GRUB_ERR_NONE)
>> +        goto fail;
>> +    }
>> +
>> +      arg = (char *) grub_buffer_peek_data (buffer);
>> +      if (arg == NULL ||
>> +      grub_buffer_advance_read_pos (buffer, grub_strlen (arg)) !=
>> GRUB_ERR_NONE)
>> +    goto fail;
>> +
>> +      (*argv)[i] = arg;
>>       }
>>   +  /* Keep memory for the return values. */
>> +  grub_buffer_take_data (buffer);
>> +
>>     grub_errno = GRUB_ERR_NONE;
>>      out:
>>     if (rd != cmdline)
>>       grub_free (rd);
>> +  grub_buffer_free (buffer);
>> +  grub_buffer_free (varname);
>>       return grub_errno;
>>      fail:
>>     grub_free (*argv);
>> -  grub_free (args);
>>     goto out;
>>   }
>>   
> 
> This commit causes a regresses. Building it as a coreboot payload with
> `-Os`, it crashes in QEMU. I am using Debian sid/unstable with GCC 10.2.
> 
>     $ gcc --version | head -1
>     gcc (Debian 10.2.1-6) 10.2.1 20210110
> 
> Build the payload `default_payload.elf`.
> 
>     $ rm -rf build && mkdir build && cd build && ../configure CFLAGS=-O2
> TARGET_CFLAGS=-Os --with-platform=coreboot --with-platform=coreboot
> --enable-boot-time --disable-werror && make -j4 && make
> default_payload.elf && cd ..
> 
> Clone coreboot, run `make menuconfig`, increase the ROM size to 2 MB and
> under *Payload* choose *Add a payload (An ELF executable payload)*, and
> select `default_payload.elf`. Run `make -j$(nproc)` and run the image in
> QEMU:
> 
>     $ qemu-system-i386 -enable-kvm -m 2G -bios build/coreboot.rom
> -serial stdio
> 
> Notice:
> 
> ```
> […]
> BS: BS_PAYLOAD_LOAD run times (exec / console): 44 / 12 ms
> Jumping to boot code at 0x00009000(0x7ffa1000)
> CPU Index 0 - APIC 0 Unexpected Exception:6 @ 10:00009016 - Halting
> Code: 0 eflags: 00010002 cr2: 00000000
> eax: 00000000 ebx: 7ffc39a4 ecx: 0001a9f8 edx: 000000ff
> edi: 0000000b esi: 000001e4 ebp: 7ffc8fd8 esp: 7ffc8f7c
> 
> 0x00008fd0:    00 00 00 00 00 00 00 00
> 0x00008fd8:    00 00 00 00 00 00 00 00
> 0x00008fe0:    00 00 00 00 00 00 00 00
> 0x00008fe8:    00 00 00 00 00 00 00 00
> 0x00008ff0:    00 00 00 00 00 00 00 00
> 0x00008ff8:    00 00 00 00 00 00 00 00
> 0x00009000:    52 52 68 f8 a9 01 00 6a
> 0x00009008:    0b e8 23 cd 00 00 83 c4
> 0x00009010:    10 a0 00 00 00 00 0f 0b
> 0x00009018:    51 51 68 0a aa 01 00 6a
> 0x00009020:    0b e8 0b cd 00 00 83 c4
> 0x00009028:    10 eb e6 90 bc f0 ff 07
> 0x00009030:    00 e9 64 d7 00 00 66 90
> 0x00009038:    02 b0 ad 1b 02 00 00 00
> 0x00009040:    fc 4f 52 e4 55 57 56 53
> 0x00009048:    83 ec 1c 8b 4c 24 30 8b
> 0x7ffc8ff8:    0x7ffc33e0
> 0x7ffc8ff4:    0xdeadbeef
> 0x7ffc8ff0:    0xdeadbeef
> 0x7ffc8fec:    0x7ffaa05f
> 0x7ffc8fe8:    0xdeadbeef
> 0x7ffc8fe4:    0xdeadbeef
> 0x7ffc8fe0:    0xdeadbeef
> 0x7ffc8fdc:    0x7ffaa05f
> 0x7ffc8fd8:    0x7ffdcfe8 <-ebp
> 0x7ffc8fd4:    0x7ffc9000
> 0x7ffc8fd0:    0x00000000
> 0x7ffc8fcc:    0x00000000
> 0x7ffc8fc8:    0x7ffc8ff0
> 0x7ffc8fc4:    0xdeadbeef
> 0x7ffc8fc0:    0xdeadbeef
> 0x7ffc8fbc:    0x7ffae54a
> 0x7ffc8fb8:    0x7ffc8fd8
> 0x7ffc8fb4:    0x0000000b
> 0x7ffc8fb0:    0x00000000
> 0x7ffc8fac:    0x7ffae55e
> 0x7ffc8fa8:    0x7ffc39a4
> 0x7ffc8fa4:    0x0000000b
> 0x7ffc8fa0:    0x000001b8
> 0x7ffc8f9c:    0x7ffae401
> 0x7ffc8f98:    0x7ffc3978
> 0x7ffc8f94:    0x18cd145c
> 0x7ffc8f90:    0x0000007b
> 0x7ffc8f8c:    0x7ffa1000
> 0x7ffc8f88:    0x00009000
> 0x7ffc8f84:    0x00000000
> 0x7ffc8f80:    0x7ffa1000
> 0x7ffc8f7c:    0x7ffb040d <-esp
> ```
> 
I could reproduce this issue but it's a bit of a pain to debug. I had a
look at some of the coreboot documentation, but I'm not sure if there's
a nice way to debug issues like this.

This is just speculation, but one thing I've noticed that happens with
this patch is that gcc now emits a .text.unlikely section containing
part of terminate_arg when compiling grub-core/kern/parser.c. If I pass
-fno-reorder-functions to gcc, then this code gets emitted to the .text
section instead and the resulting grub binary seems to load fine. This
might suggest that perhaps this code is getting omitted from the final
binary, but I'm not sure yet how that would happen.

Cheers
- Chris

> 
> Kind regards,
> 
> Paul
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

OpenPGP_signature
Description: OpenPGP digital signature