Re: [PATCH] libio: Avoid _allocate_buffer, _free_buffer pointers [BZ #23236]

[Carlos O'Donell <carlos at redhat dot com>]

On 05/25/2018 11:13 AM, Florian Weimer wrote:
> These unmangled function pointers reside on the heap and could
> be targeted by exploit writers, effectively bypassing libio vtable
> validation.  Instead, we ignore these pointers and always call
> malloc or free.
> 
> In theory, this is a backwards-incompatible change, but using the
> global heap instead of the user-supplied callback functions should
> have little application impact.  (The old libstdc++ implementation
> exposed this functionality via a public, undocumented constructor
> in its strstreambuf class.)

I like the idea behind the cleanup, I have one question below about
assignment of malloc/free. See below.

> 2018-05-25  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #23236]
> 	* libio/strfile.h (struct _IO_str_fields): Rename members to
> 	discourage their use and add comment.
> 	(_IO_STR_DYNAMIC): Remove unused macro.
> 	* libio/strops.c (_IO_str_init_static_internal): Do not use
> 	callback pointers.  Call malloc and free.
> 	(_IO_str_overflow): Do not use callback pointers.  Call malloc
> 	and free.
> 	(enlarge_userbuf): Likewise.
> 	(_IO_str_finish): Call free.
> 	* libio/wstrops.c (_IO_wstr_init_static): Initialize
> 	_allocate_buffer_unused.
> 	(_IO_wstr_overflow): Do not use callback pointers.  Call malloc
> 	and free.
> 	(enlarge_userbuf): Likewise.
> 	(_IO_wstr_finish): Call free.
> 	* debug/vasprintf_chk.c (__vasprintf_chk): Initialize
> 	_allocate_buffer_unused, _free_buffer_unused.
> 	* libio/memstream.c (__open_memstream): Likewise.
> 	* libio/vasprintf.c (_IO_vasprintf): Likewise.
> 	* libio/wmemstream.c (open_wmemstream): Likewise.
> 
> diff --git a/debug/vasprintf_chk.c b/debug/vasprintf_chk.c
> index 46603d9538..48b4741651 100644
> --- a/debug/vasprintf_chk.c
> +++ b/debug/vasprintf_chk.c
> @@ -55,8 +55,8 @@ __vasprintf_chk (char **result_ptr, int flags, const char *format,
>    _IO_JUMPS (&sf._sbf) = &_IO_str_jumps;
>    _IO_str_init_static_internal (&sf, string, init_string_size, string);
>    sf._sbf._f._flags &= ~_IO_USER_BUF;
> -  sf._s._allocate_buffer = (_IO_alloc_type) malloc;
> -  sf._s._free_buffer = (_IO_free_type) free;
> +  sf._s._allocate_buffer_unused = (_IO_alloc_type) malloc;
> +  sf._s._free_buffer_unused = (_IO_free_type) free;

See comments below.

>  
>    /* For flags > 0 (i.e. __USE_FORTIFY_LEVEL > 1) request that %n
>       can only come from read-only format strings.  */
> diff --git a/libio/memstream.c b/libio/memstream.c
> index 8d2726dea3..b5eaa5476c 100644
> --- a/libio/memstream.c
> +++ b/libio/memstream.c
> @@ -90,8 +90,8 @@ __open_memstream (char **bufloc, size_t *sizeloc)
>    _IO_JUMPS_FILE_plus (&new_f->fp._sf._sbf) = &_IO_mem_jumps;
>    _IO_str_init_static_internal (&new_f->fp._sf, buf, BUFSIZ, buf);
>    new_f->fp._sf._sbf._f._flags &= ~_IO_USER_BUF;
> -  new_f->fp._sf._s._allocate_buffer = (_IO_alloc_type) malloc;
> -  new_f->fp._sf._s._free_buffer = (_IO_free_type) free;
> +  new_f->fp._sf._s._allocate_buffer_unused = (_IO_alloc_type) malloc;
> +  new_f->fp._sf._s._free_buffer_unused = (_IO_free_type) free;

See comments below.

>  
>    new_f->fp.bufloc = bufloc;
>    new_f->fp.sizeloc = sizeloc;
> diff --git a/libio/strfile.h b/libio/strfile.h
> index 46ac81809a..75caac2af5 100644
> --- a/libio/strfile.h
> +++ b/libio/strfile.h
> @@ -32,8 +32,11 @@ typedef void (*_IO_free_type) (void*);
>  
>  struct _IO_str_fields
>  {
> -  _IO_alloc_type _allocate_buffer;
> -  _IO_free_type _free_buffer;
> +  /* These members are preserved for ABI compatibility.  The glibc
> +     implementation always calls malloc/free for user buffers if
> +     _IO_USER_BUF or _IO_FLAGS2_USER_WBUF are not set.  */
> +  _IO_alloc_type _allocate_buffer_unused;
> +  _IO_free_type _free_buffer_unused;

OK.

>  };
>  
>  /* This is needed for the Irix6 N32 ABI, which has a 64 bit off_t type,
> @@ -53,10 +56,6 @@ typedef struct _IO_strfile_
>    struct _IO_str_fields _s;
>  } _IO_strfile;
>  
> -/* dynamic: set when the array object is allocated (or reallocated)  as
> -   necessary to hold a character sequence that can change in length. */
> -#define _IO_STR_DYNAMIC(FP) ((FP)->_s._allocate_buffer != (_IO_alloc_type)0)
> -

OK.

>  /* frozen: set when the program has requested that the array object not
>     be altered, reallocated, or freed. */
>  #define _IO_STR_FROZEN(FP) ((FP)->_f._flags & _IO_USER_BUF)
> diff --git a/libio/strops.c b/libio/strops.c
> index eddd722c09..df8268b7ab 100644
> --- a/libio/strops.c
> +++ b/libio/strops.c
> @@ -61,7 +61,7 @@ _IO_str_init_static_internal (_IO_strfile *sf, char *ptr, size_t size,
>        fp->_IO_read_end = end;
>      }
>    /* A null _allocate_buffer function flags the strfile as being static. */
> -  sf->_s._allocate_buffer = (_IO_alloc_type) 0;
> +  sf->_s._allocate_buffer_unused = (_IO_alloc_type) 0;

OK.

>  }
>  
>  void
> @@ -103,8 +103,7 @@ _IO_str_overflow (FILE *fp, int c)
>  	  size_t new_size = 2 * old_blen + 100;
>  	  if (new_size < old_blen)
>  	    return EOF;
> -	  new_buf
> -	    = (char *) (*((_IO_strfile *) fp)->_s._allocate_buffer) (new_size);
> +	  new_buf = malloc (new_size);

OK.

>  	  if (new_buf == NULL)
>  	    {
>  	      /*	  __ferror(fp) = 1; */
> @@ -113,7 +112,7 @@ _IO_str_overflow (FILE *fp, int c)
>  	  if (old_buf)
>  	    {
>  	      memcpy (new_buf, old_buf, old_blen);
> -	      (*((_IO_strfile *) fp)->_s._free_buffer) (old_buf);
> +	      free (old_buf);

OK.

>  	      /* Make sure _IO_setb won't try to delete _IO_buf_base. */
>  	      fp->_IO_buf_base = NULL;
>  	    }
> @@ -182,15 +181,14 @@ enlarge_userbuf (FILE *fp, off64_t offset, int reading)
>  
>    size_t newsize = offset + 100;
>    char *oldbuf = fp->_IO_buf_base;
> -  char *newbuf
> -    = (char *) (*((_IO_strfile *) fp)->_s._allocate_buffer) (newsize);
> +  char *newbuf = malloc (newsize);

OK.

>    if (newbuf == NULL)
>      return 1;
>  
>    if (oldbuf != NULL)
>      {
>        memcpy (newbuf, oldbuf, _IO_blen (fp));
> -      (*((_IO_strfile *) fp)->_s._free_buffer) (oldbuf);
> +      free (oldbuf);

OK.

>        /* Make sure _IO_setb won't try to delete
>  	 _IO_buf_base. */
>        fp->_IO_buf_base = NULL;
> @@ -346,7 +344,7 @@ void
>  _IO_str_finish (FILE *fp, int dummy)
>  {
>    if (fp->_IO_buf_base && !(fp->_flags & _IO_USER_BUF))
> -    (((_IO_strfile *) fp)->_s._free_buffer) (fp->_IO_buf_base);
> +    free (fp->_IO_buf_base);
>    fp->_IO_buf_base = NULL;
>  
>    _IO_default_finish (fp, 0);
> diff --git a/libio/vasprintf.c b/libio/vasprintf.c
> index 08218ddbe7..6c35d2b108 100644
> --- a/libio/vasprintf.c
> +++ b/libio/vasprintf.c
> @@ -54,8 +54,8 @@ _IO_vasprintf (char **result_ptr, const char *format, va_list args)
>    _IO_JUMPS (&sf._sbf) = &_IO_str_jumps;
>    _IO_str_init_static_internal (&sf, string, init_string_size, string);
>    sf._sbf._f._flags &= ~_IO_USER_BUF;
> -  sf._s._allocate_buffer = (_IO_alloc_type) malloc;
> -  sf._s._free_buffer = (_IO_free_type) free;
> +  sf._s._allocate_buffer_unused = (_IO_alloc_type) malloc;
> +  sf._s._free_buffer_unused = (_IO_free_type) free;

See next comment.

>    ret = _IO_vfprintf (&sf._sbf._f, format, args);
>    if (ret < 0)
>      {
> diff --git a/libio/wmemstream.c b/libio/wmemstream.c
> index 88044f3150..c92d2da4b2 100644
> --- a/libio/wmemstream.c
> +++ b/libio/wmemstream.c
> @@ -92,8 +92,8 @@ open_wmemstream (wchar_t **bufloc, size_t *sizeloc)
>    _IO_wstr_init_static (&new_f->fp._sf._sbf._f, buf,
>  			BUFSIZ / sizeof (wchar_t), buf);
>    new_f->fp._sf._sbf._f._flags2 &= ~_IO_FLAGS2_USER_WBUF;
> -  new_f->fp._sf._s._allocate_buffer = (_IO_alloc_type) malloc;
> -  new_f->fp._sf._s._free_buffer = (_IO_free_type) free;
> +  new_f->fp._sf._s._allocate_buffer_unused = (_IO_alloc_type) malloc;
> +  new_f->fp._sf._s._free_buffer_unused = (_IO_free_type) free;

Do we have to assign malloc/free to these? Why not just a static value of '1'?
It would make it more clear that they are unused and crash. Or is it possible
that they could get used by legacy code?

>  
>    new_f->fp.bufloc = bufloc;
>    new_f->fp.sizeloc = sizeloc;
> diff --git a/libio/wstrops.c b/libio/wstrops.c
> index 36e8b20fef..6626f2f711 100644
> --- a/libio/wstrops.c
> +++ b/libio/wstrops.c
> @@ -63,7 +63,7 @@ _IO_wstr_init_static (FILE *fp, wchar_t *ptr, size_t size,
>        fp->_wide_data->_IO_read_end = end;
>      }
>    /* A null _allocate_buffer function flags the strfile as being static. */
> -  (((_IO_strfile *) fp)->_s._allocate_buffer) = (_IO_alloc_type)0;
> +  (((_IO_strfile *) fp)->_s._allocate_buffer_unused) = (_IO_alloc_type)0;

OK, but should be '(_IO_alloc_type) 0' for the right style.

>  }
>  
>  wint_t
> @@ -95,9 +95,7 @@ _IO_wstr_overflow (FILE *fp, wint_t c)
>  	      || __glibc_unlikely (new_size > SIZE_MAX / sizeof (wchar_t)))
>  	    return EOF;
>  
> -	  new_buf
> -	    = (wchar_t *) (*((_IO_strfile *) fp)->_s._allocate_buffer) (new_size
> -									* sizeof (wchar_t));
> +	  new_buf = malloc (new_size * sizeof (wchar_t));

OK.

>  	  if (new_buf == NULL)
>  	    {
>  	      /*	  __ferror(fp) = 1; */
> @@ -106,7 +104,7 @@ _IO_wstr_overflow (FILE *fp, wint_t c)
>  	  if (old_buf)
>  	    {
>  	      __wmemcpy (new_buf, old_buf, old_wblen);
> -	      (*((_IO_strfile *) fp)->_s._free_buffer) (old_buf);
> +	      free (old_buf);

OK.

>  	      /* Make sure _IO_setb won't try to delete _IO_buf_base. */
>  	      fp->_wide_data->_IO_buf_base = NULL;
>  	    }
> @@ -186,16 +184,14 @@ enlarge_userbuf (FILE *fp, off64_t offset, int reading)
>      return 1;
>  
>    wchar_t *oldbuf = wd->_IO_buf_base;
> -  wchar_t *newbuf
> -    = (wchar_t *) (*((_IO_strfile *) fp)->_s._allocate_buffer) (newsize
> -								* sizeof (wchar_t));
> +  wchar_t *newbuf = malloc (newsize * sizeof (wchar_t));

OK.

>    if (newbuf == NULL)
>      return 1;
>  
>    if (oldbuf != NULL)
>      {
>        __wmemcpy (newbuf, oldbuf, _IO_wblen (fp));
> -      (*((_IO_strfile *) fp)->_s._free_buffer) (oldbuf);
> +      free (oldbuf);

OK.

>        /* Make sure _IO_setb won't try to delete
>  	 _IO_buf_base. */
>        wd->_IO_buf_base = NULL;
> @@ -357,7 +353,7 @@ void
>  _IO_wstr_finish (FILE *fp, int dummy)
>  {
>    if (fp->_wide_data->_IO_buf_base && !(fp->_flags2 & _IO_FLAGS2_USER_WBUF))
> -    (((_IO_strfile *) fp)->_s._free_buffer) (fp->_wide_data->_IO_buf_base);
> +    free (fp->_wide_data->_IO_buf_base);

OK.

>    fp->_wide_data->_IO_buf_base = NULL;
>  
>    _IO_wdefault_finish (fp, 0);
>