Message ID | 20180525151329.23778403744B5@oldenburg.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | libio: Avoid _allocate_buffer, _free_buffer pointers [BZ #23236] | expand |
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); >
On 05/30/2018 06:36 PM, Carlos O'Donell wrote: >> - 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? Yes, it is possible that they are used from 90s libstdc++ and similar code. Having valid function pointers there helps with compatibility, I think. Thanks, Florian
On 05/30/2018 12:40 PM, Florian Weimer wrote: > On 05/30/2018 06:36 PM, Carlos O'Donell wrote: >>> - 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? > > Yes, it is possible that they are used from 90s libstdc++ and similar > code. Having valid function pointers there helps with compatibility, > I think. OK, looks good to me then. Thanks for the hardening work. Reviewed-by: Carlos O'Donell <carlos@redhat.com>
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; /* 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; 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; }; /* 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 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; } 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 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; 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; 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; } 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);