From 41fa06451a688e7b33c4751a8b8f24a31bb9f115 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. --- 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 fdde86f6..d92c5da8 100644 --- a/nifti2/nifti_tool.c +++ b/nifti2/nifti_tool.c @@ -3344,7 +3344,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; @@ -3470,7 +3470,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; @@ -3707,7 +3707,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; @@ -3810,7 +3810,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 */ @@ -3840,7 +3840,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; } @@ -3853,7 +3853,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; @@ -3870,6 +3870,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 ba57369d..b93d115e 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 5d9f89a6..129e8cad 100644 --- a/niftilib/nifti1_tool.c +++ b/niftilib/nifti1_tool.c @@ -2592,7 +2592,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; @@ -2796,7 +2796,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; @@ -2866,7 +2866,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 */ @@ -2892,7 +2892,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; } @@ -2905,7 +2905,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; @@ -2922,6 +2922,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 b099924a..ef8e2e98 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);