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

diff --git a/ChangeLog b/ChangeLog
index 74588dac11fe6930e5ddfdb0a255f769cab7726d..1b5b27ce4b2900decc1f78c1f1ae0d661cec314f 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-30  Paul Pluzhnikov  <ppluzhnikov@google.com>
 
        * sysdeps/x86_64/fpu/libm-test-ulps (log_vlen8_avx2): Update for

diff --git a/debug/vasprintf_chk.c b/debug/vasprintf_chk.c
index 46603d953889304fb23334ef9dab6e449ee5c3aa..48b4741651c6e319a4099812656c6210b192d6c1 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 8d2726dea32aa69d87019fb5f2e12ca8e5859c1d..b5eaa5476c81e898ff6f137bfd3bbe6657ae63a4 100644 (file)
--- 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;
 
   new_f->fp.bufloc = bufloc;
   new_f->fp.sizeloc = sizeloc;

diff --git a/libio/strfile.h b/libio/strfile.h
index 46ac81809a3ab0fe3e98677e4e10d09f08b2458a..75caac2af5fbd6a2bbb5fb880a1db792adf3339d 100644 (file)
--- 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;
 };
 
 /* 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)
-
 /* 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 eddd722c093ee431ada44fa212475f0e9f6c63b7..df8268b7abc0e5874e222cf68b911e8825350162 100644 (file)
--- 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;
 }
 
 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);
          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);
              /* 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);
   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 (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 08218ddbe765a878f9acd3a52b23011a7b9e3707..6c35d2b10881753bc9d52e3764161c0d3626ad01 100644 (file)
--- 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;
   ret = _IO_vfprintf (&sf._sbf._f, format, args);
   if (ret < 0)
     {

diff --git a/libio/wmemstream.c b/libio/wmemstream.c
index 88044f315008ec48c308d6cb4545e26392e976d7..c92d2da4b2974cbe24aedc0e3afc1349be9d07f3 100644 (file)
--- 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;
 
   new_f->fp.bufloc = bufloc;
   new_f->fp.sizeloc = sizeloc;

diff --git a/libio/wstrops.c b/libio/wstrops.c
index 36e8b20fefcb75e4b456976b803d6a35ae914be0..6626f2f71166bd3aa29e967cb775498ce723b476 100644 (file)
--- 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;
 }
 
 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));
          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);
              /* 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));
   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 (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);