Re: AddressSanitizer: global-buffer-overflow in rl_filename_completion_function

[Eduardo A . Bustamante López]

On Thu, Jun 15, 2017 at 09:41:08AM -0500, Eduardo Bustamante wrote:
> Found by fuzzing `read -e' with AFL. The stacktrace reported by Address
> Sanitizer is followed by the base64 encoded crashing input.
> 
> 
> ==1098==ERROR: AddressSanitizer: global-buffer-overflow on address 
> 0x55e61a6b4c5c at pc 0x55e61a3426ca bp 0x7fff1820a300 sp 0x7fff1820a2f8
> READ of size 4 at 0x55e61a6b4c5c thread T0
>     #0 0x55e61a3426c9 in bash_dequote_filename 
> (/home/dualbus/src/gnu/bash-build/bash+0x17a6c9)

Easy fix. `p' is a signed char pointer, therefore when `*p = 255', it tries to
dereference `sh_syntaxtab[-1]'.

dualbus@debian:~/src/gnu/bash$ git difftool -y -x 'diff -c' -- bashline.c
*** /tmp/mVc4sH_bashline.c      2017-06-16 09:52:56.471508904 -0500
--- bashline.c  2017-06-16 09:48:55.706503276 -0500
***************
*** 3886,3892 ****
            *r++ = *p;
          /* Backslashes are preserved within double quotes unless the
             character is one that is defined to be escaped */
!         else if (quoted == '"' && ((sh_syntaxtab[p[1]] & CBSDQUOTE) == 0))
            *r++ = *p;
  
          *r++ = *++p;
--- 3886,3892 ----
            *r++ = *p;
          /* Backslashes are preserved within double quotes unless the
             character is one that is defined to be escaped */
!         else if (quoted == '"' && ((sh_syntaxtab[(unsigned char)p[1]] & 
CBSDQUOTE) == 0))
            *r++ = *p;
  
          *r++ = *++p;





Maybe it's a good idea to change these too. In locale.c there shouldn't be a
problem, because the loop is constrained to `x < sh_syntabsiz', but perhaps
just to silence compiler warnings :-)?

dualbus@debian:~/src/gnu/bash$ git difftool -y -x 'diff -c' -- locale.c parse.y 
*** /tmp/OyLWya_locale.c        2017-06-16 09:55:15.854368199 -0500
--- locale.c    2017-06-16 09:50:52.816950476 -0500
***************
*** 552,565 ****
    for (x = 0; x < sh_syntabsiz; x++)
      {
        if (isblank ((unsigned char)x))
!       sh_syntaxtab[x] |= CSHBRK|CBLANK;
        else if (member (x, shell_break_chars))
        {
!         sh_syntaxtab[x] |= CSHBRK;
!         sh_syntaxtab[x] &= ~CBLANK;
        }
        else
!       sh_syntaxtab[x] &= ~(CSHBRK|CBLANK);
      }
  }
  
--- 552,565 ----
    for (x = 0; x < sh_syntabsiz; x++)
      {
        if (isblank ((unsigned char)x))
!       sh_syntaxtab[(unsigned char)x] |= CSHBRK|CBLANK;
        else if (member (x, shell_break_chars))
        {
!         sh_syntaxtab[(unsigned char)x] |= CSHBRK;
!         sh_syntaxtab[(unsigned char)x] &= ~CBLANK;
        }
        else
!       sh_syntaxtab[(unsigned char)x] &= ~(CSHBRK|CBLANK);
      }
  }
  
*** /tmp/qKFxWa_parse.y 2017-06-16 09:55:15.862368362 -0500
--- parse.y     2017-06-16 09:52:22.522808775 -0500
***************
*** 4842,4848 ****
  
              /* If the next character is to be quoted, note it now. */
              if (cd == 0 || cd == '`' ||
!                 (cd == '"' && peek_char >= 0 && (sh_syntaxtab[peek_char] & 
CBSDQUOTE)))
                pass_next_character++;
  
              quoted = 1;
--- 4842,4848 ----
  
              /* If the next character is to be quoted, note it now. */
              if (cd == 0 || cd == '`' ||
!                 (cd == '"' && peek_char >= 0 && (sh_syntaxtab[(unsigned 
char)peek_char] & CBSDQUOTE)))
                pass_next_character++;
  
              quoted = 1;

-- 
Eduardo Bustamante
https://dualbus.me/