From d58780080b5e4095aaf443db0b83ee7d2896042b Mon Sep 17 00:00:00 2001 From: Sean McBride Date: Fri, 2 Jan 2026 00:16:49 -0500 Subject: [PATCH] ENH: Bound the field-modification helpers by the destination size modify_all_fields() and modify_field() wrote into a caller-supplied buffer at an offset taken from the field table, with no way to check that the write stayed inside it. Both now take the buffer size, and modify_field() rejects a field whose offset plus size * len exceeds it. The check sits ahead of the switch, so it covers every write path rather than the string case alone, and it reports and returns like the other failures in the function; an assert() would compile away in the release builds that ship. No field table can trip it today: check_total_size() already requires the offsets to tile the structure exactly. It bounds future edits to them. (cherry picked from commit 7356eb146fc1d33ef10e18889f12fcb6c2477440) --- nifti2/nifti_tool.c | 22 ++++++++++++++++------ nifti2/nifti_tool.h | 4 ++-- niftilib/nifti1_tool.c | 20 +++++++++++++++----- niftilib/nifti1_tool.h | 4 ++-- 4 files changed, 35 insertions(+), 15 deletions(-) diff --git a/nifti2/nifti_tool.c b/nifti2/nifti_tool.c index ac249bf8..91726dbe 100644 --- a/nifti2/nifti_tool.c +++ b/nifti2/nifti_tool.c @@ -3343,7 +3343,7 @@ int act_mod_hdrs( nt_opts * opts ) } /* okay, let's actually trash the data fields */ - if( modify_all_fields(nhdr, opts, g_hdr1_fields, NT_HDR1_NUM_FIELDS) ) + if( modify_all_fields(nhdr, sizeof(*nhdr), opts, g_hdr1_fields, NT_HDR1_NUM_FIELDS) ) { free(nhdr); return 1; @@ -3461,7 +3461,7 @@ int act_mod_hdr2s( nt_opts * opts ) } /* okay, let's actually trash the data fields */ - if( modify_all_fields(nhdr, opts, g_hdr2_fields, NT_HDR2_NUM_FIELDS) ) + if( modify_all_fields(nhdr, sizeof(*nhdr), opts, g_hdr2_fields, NT_HDR2_NUM_FIELDS) ) { free(nhdr); return 1; @@ -3684,7 +3684,7 @@ int act_mod_nims( nt_opts * opts ) opts->flist.len, opts->infiles.list[filec]); /* okay, let's actually trash the data fields */ - if( modify_all_fields(nim, opts, g_nim2_fields, NT_NIM_NUM_FIELDS) ) + if( modify_all_fields(nim, sizeof(*nim), opts, g_nim2_fields, NT_NIM_NUM_FIELDS) ) { nifti_image_free(nim); return 1; @@ -3787,7 +3787,7 @@ int write_hdr2_to_file( nifti_2_header * nhdr, const char * fname ) /*---------------------------------------------------------------------- * modify all fields in the list *----------------------------------------------------------------------*/ -int modify_all_fields( void * basep, nt_opts * opts, field_s * fields, int flen) +int modify_all_fields( void * basep, size_t baselen, nt_opts * opts, field_s * fields, int flen) { field_s * fp; int fc, lc; /* field and list counters */ @@ -3817,7 +3817,7 @@ int modify_all_fields( void * basep, nt_opts * opts, field_s * fields, int flen) return 1; } - if( modify_field( basep, fp, opts->vlist.list[lc]) ) + if( modify_field( basep, baselen, fp, opts->vlist.list[lc]) ) return 1; } @@ -3830,7 +3830,7 @@ int modify_all_fields( void * basep, nt_opts * opts, field_s * fields, int flen) * * pointer fields are not allowed here *----------------------------------------------------------------------*/ -int modify_field(void * basep, field_s * field, const char * data) +int modify_field(void * basep, size_t baselen, field_s * field, const char * data) { float fval; const char * posn = data; @@ -3847,6 +3847,16 @@ int modify_field(void * basep, field_s * field, const char * data) return 1; } + /* every case below writes field->len elements at field->offset */ + if( field->offset < 0 || field->size < 0 || field->len < 0 || + (size_t)field->offset + (size_t)field->size * (size_t)field->len > baselen ) + { + fprintf(stderr,"** field '%s' (offset %d, %d x %d bytes) does not fit " + "in a %zu byte structure\n", + field->name, field->offset, field->len, field->size, baselen); + return 1; + } + switch( field->type ) { case DT_UNKNOWN: diff --git a/nifti2/nifti_tool.h b/nifti2/nifti_tool.h index d548f66d..b36178ef 100644 --- a/nifti2/nifti_tool.h +++ b/nifti2/nifti_tool.h @@ -306,8 +306,8 @@ NI2_API int fill_hdr2_field_array(field_s * nh_fields); NI2_API int fill_nim1_field_array(field_s * nim_fields); NI2_API int fill_nim2_field_array(field_s * nim_fields); NI2_API int fill_ana_field_array(field_s * ah_fields); -NI2_API int modify_all_fields(void *basep, nt_opts *opts, field_s *fields, int flen); -NI2_API int modify_field (void * basep, field_s * field, const char * data); +NI2_API int modify_all_fields(void *basep, size_t baseplen, nt_opts *opts, field_s *fields, int flen); +NI2_API int modify_field (void * basep, size_t baseplen, field_s * field, const char * data); NI2_API int process_opts (int argc, const char * argv[], nt_opts * opts); NI2_API int remove_ext_list (nifti_image * nim, const char ** elist, int len); NI2_API int usage (const char * prog, int level); diff --git a/niftilib/nifti1_tool.c b/niftilib/nifti1_tool.c index 15bb165a..e467d887 100644 --- a/niftilib/nifti1_tool.c +++ b/niftilib/nifti1_tool.c @@ -2591,7 +2591,7 @@ int act_mod_hdrs( nt_opts * opts ) } /* okay, let's actually trash the data fields */ - if( modify_all_fields(nhdr, opts, g_hdr_fields, NT_HDR_NUM_FIELDS) ) + if( modify_all_fields(nhdr, sizeof(*nhdr), opts, g_hdr_fields, NT_HDR_NUM_FIELDS) ) { free(nhdr); return 1; @@ -2787,7 +2787,7 @@ int act_mod_nims( nt_opts * opts ) opts->flist.len, opts->infiles.list[filec]); /* okay, let's actually trash the data fields */ - if( modify_all_fields(nim, opts, g_nim_fields, NT_NIM_NUM_FIELDS) ) + if( modify_all_fields(nim, sizeof(*nim), opts, g_nim_fields, NT_NIM_NUM_FIELDS) ) { nifti_image_free(nim); return 1; @@ -2857,7 +2857,7 @@ int write_hdr_to_file( nifti_1_header * nhdr, const char * fname ) /*---------------------------------------------------------------------- * modify all fields in the list *----------------------------------------------------------------------*/ -int modify_all_fields( void * basep, nt_opts * opts, field_s * fields, int flen) +int modify_all_fields( void * basep, size_t baselen, nt_opts * opts, field_s * fields, int flen) { field_s * fp; int fc, lc; /* field and list counters */ @@ -2883,7 +2883,7 @@ int modify_all_fields( void * basep, nt_opts * opts, field_s * fields, int flen) return 1; } - if( modify_field( basep, fp, opts->vlist.list[lc]) ) + if( modify_field( basep, baselen, fp, opts->vlist.list[lc]) ) return 1; } @@ -2896,7 +2896,7 @@ int modify_all_fields( void * basep, nt_opts * opts, field_s * fields, int flen) * * pointer fields are not allowed here *----------------------------------------------------------------------*/ -int modify_field(void * basep, field_s * field, const char * data) +int modify_field(void * basep, size_t baselen, field_s * field, const char * data) { float fval; const char * posn = data; @@ -2913,6 +2913,16 @@ int modify_field(void * basep, field_s * field, const char * data) return 1; } + /* every case below writes field->len elements at field->offset */ + if( field->offset < 0 || field->size < 0 || field->len < 0 || + (size_t)field->offset + (size_t)field->size * (size_t)field->len > baselen ) + { + fprintf(stderr,"** field '%s' (offset %d, %d x %d bytes) does not fit " + "in a %zu byte structure\n", + field->name, field->offset, field->len, field->size, baselen); + return 1; + } + switch( field->type ) { case DT_UNKNOWN: diff --git a/niftilib/nifti1_tool.h b/niftilib/nifti1_tool.h index a8ca5ddd..3fbde617 100644 --- a/niftilib/nifti1_tool.h +++ b/niftilib/nifti1_tool.h @@ -140,8 +140,8 @@ int fill_field (field_s *fp, int type, int offset, int num, const char *na int fill_hdr_field_array(field_s * nh_fields); int fill_nim_field_array(field_s * nim_fields); int fill_ana_field_array(field_s * ah_fields); -int modify_all_fields(void *basep, nt_opts *opts, field_s *fields, int flen); -int modify_field (void * basep, field_s * field, const char * data); +int modify_all_fields(void *basep, size_t baseplen, nt_opts *opts, field_s *fields, int flen); +int modify_field (void * basep, size_t baseplen, field_s * field, const char * data); int process_opts (int argc, const char * argv[], nt_opts * opts); int remove_ext_list (nifti_image * nim, const char ** elist, int len); int usage (const char * prog, int level);