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

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.)

(cherry picked from commit 4e8a6346cd3da2d88bbad745a1769260d36f2783)

diff --git a/ChangeLog b/ChangeLog
index e45fa8e6d0066e7347933cadf38cce55e3528363..0126fb391b3229f4286f68cf43a5562698614caa 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,27 @@
+2018-06-01  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.
+
 2018-05-23  H.J. Lu  <hongjiu.lu@intel.com>
 
        [BZ #23196]

diff --git a/NEWS b/NEWS
index c6c5538192a538242d5294d1d015b52c8ccd7272..b8bd101f6caf329ac7f35a8d60c4e2cc0dceb444 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -140,6 +140,7 @@ The following bugs are resolved with this release:
   [23037] resolv: Fully initialize struct mmsghdr in send_dg
   [23137] s390: Fix blocking pthread_join
   [23196] __mempcpy_avx512_no_vzeroupper mishandles large copies
+  [23236] Harden function pointers in _IO_str_fields
 \f
 Version 2.26
 

diff --git a/debug/vasprintf_chk.c b/debug/vasprintf_chk.c
index 5ebfeed2393508b8fa6f07982ba0b557490a9206..1d989dd7073773cb79211bfc4f02e5f14b4df617 100644 (file)
--- 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;
 
   /* 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 e391efd48aecb441ced8419871591546f1221b8b..c26239968d9766a856cd4c7f64b2e721f2cdb4b5 100644 (file)
--- a/libio/memstream.c
+++ b/libio/memstream.c
@@ -90,8 +90,8 @@ __open_memstream (char **bufloc, _IO_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, _IO_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;
 
   new_f->fp.bufloc = bufloc;
   new_f->fp.sizeloc = sizeloc;

diff --git a/libio/strfile.h b/libio/strfile.h
index 74bd4eb269ec3167d379ae2b440544b19ceb97fb..7ce1008516786735ec51ec0f2b1ef118effde82a 100644 (file)
--- a/libio/strfile.h
+++ b/libio/strfile.h
@@ -34,8 +34,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;
 };
 
 /* This is needed for the Irix6 N32 ABI, which has a 64 bit off_t type,
@@ -55,10 +58,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)
-
 /* frozen: set when the program has requested that the array object not
    be altered, reallocated, or freed. */
 #define _IO_STR_FROZEN(FP) ((FP)->_f._IO_file_flags & _IO_USER_BUF)

diff --git a/libio/strops.c b/libio/strops.c
index d72a47c6493df48f1d5ca4b661694a9da8db5d35..625c09416c33a87c1950e26dad783d6f8b2ab9f5 100644 (file)
--- a/libio/strops.c
+++ b/libio/strops.c
@@ -61,7 +61,7 @@ _IO_str_init_static_internal (_IO_strfile *sf, char *ptr, _IO_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;
 }
 
 void
@@ -103,8 +103,7 @@ _IO_str_overflow (_IO_FILE *fp, int c)
          _IO_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);
          if (new_buf == NULL)
            {
              /*          __ferror(fp) = 1; */
@@ -113,7 +112,7 @@ _IO_str_overflow (_IO_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);
              /* Make sure _IO_setb won't try to delete _IO_buf_base. */
              fp->_IO_buf_base = NULL;
            }
@@ -182,15 +181,14 @@ enlarge_userbuf (_IO_FILE *fp, _IO_off64_t offset, int reading)
 
   _IO_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);
   if (newbuf == NULL)
     return 1;
 
   if (oldbuf != NULL)
     {
       memcpy (newbuf, oldbuf, _IO_blen (fp));
-      (*((_IO_strfile *) fp)->_s._free_buffer) (oldbuf);
+      free (oldbuf);
       /* 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 (_IO_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 a9a21545a2b833d3cd3944291c819d655fee05b3..50e3559ab8e8e159f23345ed8f3d5f7ff9a2f694 100644 (file)
--- a/libio/vasprintf.c
+++ b/libio/vasprintf.c
@@ -54,8 +54,8 @@ _IO_vasprintf (char **result_ptr, const char *format, _IO_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;
   ret = _IO_vfprintf (&sf._sbf._f, format, args);
   if (ret < 0)
     {

diff --git a/libio/wmemstream.c b/libio/wmemstream.c
index 103a760bf5144a35d12665588d6ba5a07467cd0c..fdabaae6a9ec3db0390b86d10da461e5084a8bf0 100644 (file)
--- a/libio/wmemstream.c
+++ b/libio/wmemstream.c
@@ -92,8 +92,8 @@ open_wmemstream (wchar_t **bufloc, _IO_size_t *sizeloc)
   _IO_wstr_init_static (&new_f->fp._sf._sbf._f, buf,
                        _IO_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;
 
   new_f->fp.bufloc = bufloc;
   new_f->fp.sizeloc = sizeloc;

diff --git a/libio/wstrops.c b/libio/wstrops.c
index bb62fd6702c80113ba0b13e97e742f768bf8402b..8f5d45a9d1c479301c145cde6885302ca0b7c5f6 100644 (file)
--- a/libio/wstrops.c
+++ b/libio/wstrops.c
@@ -63,7 +63,7 @@ _IO_wstr_init_static (_IO_FILE *fp, wchar_t *ptr, _IO_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;
 }
 
 _IO_wint_t
@@ -95,9 +95,7 @@ _IO_wstr_overflow (_IO_FILE *fp, _IO_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));
          if (new_buf == NULL)
            {
              /*          __ferror(fp) = 1; */
@@ -106,7 +104,7 @@ _IO_wstr_overflow (_IO_FILE *fp, _IO_wint_t c)
          if (old_buf)
            {
              __wmemcpy (new_buf, old_buf, old_wblen);
-             (*((_IO_strfile *) fp)->_s._free_buffer) (old_buf);
+             free (old_buf);
              /* 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 (_IO_FILE *fp, _IO_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));
   if (newbuf == NULL)
     return 1;
 
   if (oldbuf != NULL)
     {
       __wmemcpy (newbuf, oldbuf, _IO_wblen (fp));
-      (*((_IO_strfile *) fp)->_s._free_buffer) (oldbuf);
+      free (oldbuf);
       /* 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 (_IO_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);
   fp->_wide_data->_IO_buf_base = NULL;
 
   _IO_wdefault_finish (fp, 0);