Skip to content

Commit 7a062cf

Browse files
nikicnikic
committed
Handle EXIF offsets in a principled manner
exif_process_IFD_TAG() currently accepts a dir_entry, offset_base and IFDlength. However, it's very hard to follow how these values are related to each other and the addressable memory region. As we add additional bounds check, this gets further confused. One of the basic cases is where dir_entry is in [offset_base, offset_base+IFDlength), in which case the memory [dir_entry, offset_base+IFDlength) is valid, but the memory [offset_base, dir_entry) is not necessarily valid. I wasn't able to understand what exactly is valid if dir_entry is outside [offset_base, offset_base+IFDlength) This patch changes everything to use a struct that separately stores offset_base and the valid memory region and adds helpers to fetch offsets and check that pointers are in-bounds. Closes GH-5068.
1 parent 3b08f53 commit 7a062cf

File tree

1 file changed

+105
-51
lines changed

1 file changed

+105
-51
lines changed

‎ext/exif/exif.c‎

Lines changed: 105 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,10 @@
5050
/* needed for ssize_t definition */
5151
#include <sys/types.h>
5252

53+
#ifdef __SANITIZE_ADDRESS__
54+
# include <sanitizer/asan_interface.h>
55+
#endif
56+
5357
typedef unsigned char uchar;
5458

5559
#ifndef TRUE
@@ -2018,6 +2022,63 @@ typedef struct {
20182022
} jpeg_sof_info;
20192023
/* }}} */
20202024

2025+
/* Base address for offset references, together with valid memory range.
2026+
* The valid range does not necessarily include the offset base. */
2027+
typedef struct {
2028+
char *offset_base;
2029+
char *valid_start; /* inclusive */
2030+
char *valid_end; /* exclusive */
2031+
} exif_offset_info;
2032+
2033+
static zend_always_inline zend_bool ptr_offset_overflows(char *ptr, size_t offset) {
2034+
return UINTPTR_MAX - (uintptr_t) ptr < offset;
2035+
}
2036+
2037+
static inline void exif_offset_info_init(
2038+
exif_offset_info *info, char *offset_base, char *valid_start, size_t valid_length) {
2039+
ZEND_ASSERT(!ptr_offset_overflows(valid_start, valid_length));
2040+
#ifdef __SANITIZE_ADDRESS__
2041+
ZEND_ASSERT(!__asan_region_is_poisoned(valid_start, valid_length));
2042+
#endif
2043+
info->offset_base = offset_base;
2044+
info->valid_start = valid_start;
2045+
info->valid_end = valid_start + valid_length;
2046+
}
2047+
2048+
/* Try to get a pointer at offset_base+offset with length dereferenceable bytes. */
2049+
static inline char *exif_offset_info_try_get(
2050+
const exif_offset_info *info, size_t offset, size_t length) {
2051+
char *start, *end;
2052+
if (ptr_offset_overflows(info->offset_base, offset)) {
2053+
return NULL;
2054+
}
2055+
2056+
start = info->offset_base + offset;
2057+
if (ptr_offset_overflows(start, length)) {
2058+
return NULL;
2059+
}
2060+
2061+
end = start + length;
2062+
if (start < info->valid_start || end > info->valid_end) {
2063+
return NULL;
2064+
}
2065+
2066+
return start;
2067+
}
2068+
2069+
static inline zend_bool exif_offset_info_contains(
2070+
const exif_offset_info *info, char *start, size_t length) {
2071+
char *end;
2072+
if (ptr_offset_overflows(start, length)) {
2073+
return 0;
2074+
}
2075+
2076+
/* start and valid_start are both inclusive, end and valid_end are both exclusive,
2077+
* so we use >= and <= to do the checks, respectively. */
2078+
end = start + length;
2079+
return start >= info->valid_start && end <= info->valid_end;
2080+
}
2081+
20212082
/* {{{ exif_file_sections_add
20222083
Add a file_section to image_info
20232084
returns the used block or -1. if size>0 and data == NULL buffer of size is allocated
@@ -2624,8 +2685,8 @@ static void exif_process_SOFn (uchar *Data, int marker, jpeg_sof_info *result)
26242685
/* }}} */
26252686

26262687
/* forward declarations */
2627-
static int exif_process_IFD_in_JPEG(image_info_type *ImageInfo, char *dir_start, char *offset_base, size_t IFDlength, size_t displacement, int section_index, int tag);
2628-
static int exif_process_IFD_TAG( image_info_type *ImageInfo, char *dir_entry, char *offset_base, size_t IFDlength, size_t displacement, int section_index, int ReadNextIFD, tag_table_type tag_table);
2688+
static int exif_process_IFD_in_JPEG(image_info_type *ImageInfo, char *dir_start, const exif_offset_info *info, size_t displacement, int section_index, int tag);
2689+
static int exif_process_IFD_TAG(image_info_type *ImageInfo, char *dir_entry, const exif_offset_info *info, size_t displacement, int section_index, int ReadNextIFD, tag_table_type tag_table);
26292690

26302691
/* {{{ exif_get_markername
26312692
Get name of marker */
@@ -2871,7 +2932,7 @@ static void exif_thumbnail_build(image_info_type *ImageInfo) {
28712932

28722933
/* {{{ exif_thumbnail_extract
28732934
* Grab the thumbnail, corrected */
2874-
static void exif_thumbnail_extract(image_info_type *ImageInfo, char *offset, size_t length) {
2935+
static void exif_thumbnail_extract(image_info_type *ImageInfo, const exif_offset_info *info) {
28752936
if (ImageInfo->Thumbnail.data) {
28762937
exif_error_docref("exif_read_data#error_mult_thumb" EXIFERR_CC, ImageInfo, E_WARNING, "Multiple possible thumbnails");
28772938
return; /* Should not happen */
@@ -2888,14 +2949,13 @@ static void exif_thumbnail_extract(image_info_type *ImageInfo, char *offset, siz
28882949
return;
28892950
}
28902951
/* Check to make sure we are not going to go past the ExifLength */
2891-
if (ImageInfo->Thumbnail.size > length
2892-
|| (ImageInfo->Thumbnail.offset + ImageInfo->Thumbnail.size) > length
2893-
|| ImageInfo->Thumbnail.offset > length - ImageInfo->Thumbnail.size
2894-
) {
2952+
char *thumbnail = exif_offset_info_try_get(
2953+
info, ImageInfo->Thumbnail.offset, ImageInfo->Thumbnail.size);
2954+
if (!thumbnail) {
28952955
EXIF_ERRLOG_THUMBEOF(ImageInfo)
28962956
return;
28972957
}
2898-
ImageInfo->Thumbnail.data = estrndup(offset + ImageInfo->Thumbnail.offset, ImageInfo->Thumbnail.size);
2958+
ImageInfo->Thumbnail.data = estrndup(thumbnail, ImageInfo->Thumbnail.size);
28992959
exif_thumbnail_build(ImageInfo);
29002960
}
29012961
/* }}} */
@@ -3063,14 +3123,14 @@ static int exif_process_unicode(image_info_type *ImageInfo, xp_field_type *xp_fi
30633123

30643124
/* {{{ exif_process_IFD_in_MAKERNOTE
30653125
* Process nested IFDs directories in Maker Note. */
3066-
static int exif_process_IFD_in_MAKERNOTE(image_info_type *ImageInfo, char * value_ptr, int value_len, char *offset_base, size_t IFDlength, size_t displacement)
3126+
static int exif_process_IFD_in_MAKERNOTE(image_info_type *ImageInfo, char * value_ptr, int value_len, const exif_offset_info *info, size_t displacement)
30673127
{
30683128
size_t i;
30693129
int de, section_index = SECTION_MAKERNOTE;
30703130
int NumDirEntries, old_motorola_intel;
30713131
const maker_note_type *maker_note;
30723132
char *dir_start;
3073-
int data_len;
3133+
exif_offset_info new_info;
30743134

30753135
for (i=0; i<=sizeof(maker_note_array)/sizeof(maker_note_type); i++) {
30763136
if (i==sizeof(maker_note_array)/sizeof(maker_note_type)) {
@@ -3122,12 +3182,11 @@ static int exif_process_IFD_in_MAKERNOTE(image_info_type *ImageInfo, char * valu
31223182

31233183
switch (maker_note->offset_mode) {
31243184
case MN_OFFSET_MAKER:
3125-
offset_base = value_ptr;
3126-
data_len = value_len;
3185+
exif_offset_info_init(&new_info, value_ptr, value_ptr, value_len);
3186+
info = &new_info;
31273187
break;
31283188
default:
31293189
case MN_OFFSET_NORMAL:
3130-
data_len = value_len;
31313190
break;
31323191
}
31333192

@@ -3143,7 +3202,7 @@ static int exif_process_IFD_in_MAKERNOTE(image_info_type *ImageInfo, char * valu
31433202
for (de=0;de<NumDirEntries;de++) {
31443203
size_t offset = 2 + 12 * de;
31453204
if (!exif_process_IFD_TAG(ImageInfo, dir_start + offset,
3146-
offset_base, data_len - offset, displacement, section_index, 0, maker_note->tag_table)) {
3205+
info, displacement, section_index, 0, maker_note->tag_table)) {
31473206
return FALSE;
31483207
}
31493208
}
@@ -3166,7 +3225,7 @@ static int exif_process_IFD_in_MAKERNOTE(image_info_type *ImageInfo, char * valu
31663225

31673226
/* {{{ exif_process_IFD_TAG
31683227
* Process one of the nested IFDs directories. */
3169-
static int exif_process_IFD_TAG(image_info_type *ImageInfo, char *dir_entry, char *offset_base, size_t IFDlength, size_t displacement, int section_index, int ReadNextIFD, tag_table_type tag_table)
3228+
static int exif_process_IFD_TAG(image_info_type *ImageInfo, char *dir_entry, const exif_offset_info *info, size_t displacement, int section_index, int ReadNextIFD, tag_table_type tag_table)
31703229
{
31713230
size_t length;
31723231
unsigned int tag, format, components;
@@ -3206,28 +3265,16 @@ static int exif_process_IFD_TAG(image_info_type *ImageInfo, char *dir_entry, cha
32063265
byte_count = (size_t)byte_count_signed;
32073266

32083267
if (byte_count > 4) {
3209-
offset_val = php_ifd_get32u(dir_entry+8, ImageInfo->motorola_intel);
32103268
/* If its bigger than 4 bytes, the dir entry contains an offset. */
3211-
value_ptr = offset_base+offset_val;
3212-
/*
3213-
dir_entry is ImageInfo->file.list[sn].data+2+i*12
3214-
offset_base is ImageInfo->file.list[sn].data-dir_offset
3215-
dir_entry - offset_base is dir_offset+2+i*12
3216-
*/
3217-
if (byte_count > IFDlength || offset_val > IFDlength-byte_count || value_ptr < dir_entry || offset_val < (size_t)(dir_entry-offset_base) || dir_entry <= offset_base) {
3269+
offset_val = php_ifd_get32u(dir_entry+8, ImageInfo->motorola_intel);
3270+
value_ptr = exif_offset_info_try_get(info, offset_val, byte_count);
3271+
if (!value_ptr) {
32183272
/* It is important to check for IMAGE_FILETYPE_TIFF
32193273
* JPEG does not use absolute pointers instead its pointers are
32203274
* relative to the start of the TIFF header in APP1 section. */
3275+
// TODO: Shouldn't we also be taking "displacement" into account here?
32213276
if (byte_count > ImageInfo->FileSize || offset_val>ImageInfo->FileSize-byte_count || (ImageInfo->FileType!=IMAGE_FILETYPE_TIFF_II && ImageInfo->FileType!=IMAGE_FILETYPE_TIFF_MM && ImageInfo->FileType!=IMAGE_FILETYPE_JPEG)) {
3222-
if (value_ptr < dir_entry) {
3223-
/* we can read this if offset_val > 0 */
3224-
/* some files have their values in other parts of the file */
3225-
exif_error_docref("exif_read_data#error_ifd" EXIFERR_CC, ImageInfo, E_WARNING, "Process tag(x%04X=%s): Illegal pointer offset(x%04X < x%04X)", tag, exif_get_tagname_debug(tag, tag_table), offset_val, dir_entry);
3226-
} else {
3227-
/* this is for sure not allowed */
3228-
/* exception are IFD pointers */
3229-
exif_error_docref("exif_read_data#error_ifd" EXIFERR_CC, ImageInfo, E_WARNING, "Process tag(x%04X=%s): Illegal pointer offset(x%04X + x%04X = x%04X > x%04X)", tag, exif_get_tagname_debug(tag, tag_table), offset_val, byte_count, offset_val+byte_count, IFDlength);
3230-
}
3277+
exif_error_docref("exif_read_data#error_ifd" EXIFERR_CC, ImageInfo, E_WARNING, "Process tag(x%04X=%s): Illegal pointer offset(x%04X + x%04X = x%04X > x%04X)", tag, exif_get_tagname_debug(tag, tag_table), offset_val, byte_count, offset_val+byte_count, ImageInfo->FileSize);
32313278
return FALSE;
32323279
}
32333280
if (byte_count>sizeof(cbuf)) {
@@ -3263,7 +3310,8 @@ static int exif_process_IFD_TAG(image_info_type *ImageInfo, char *dir_entry, cha
32633310
} else {
32643311
/* 4 bytes or less and value is in the dir entry itself */
32653312
value_ptr = dir_entry+8;
3266-
offset_val= value_ptr-offset_base;
3313+
// TODO: This is dubious, but the value is only used for debugging.
3314+
offset_val = value_ptr-info->offset_base;
32673315
}
32683316

32693317
ImageInfo->sections_found |= FOUND_ANY_TAG;
@@ -3446,7 +3494,7 @@ static int exif_process_IFD_TAG(image_info_type *ImageInfo, char *dir_entry, cha
34463494
break;
34473495

34483496
case TAG_MAKER_NOTE:
3449-
if (!exif_process_IFD_in_MAKERNOTE(ImageInfo, value_ptr, byte_count, offset_base, IFDlength, displacement)) {
3497+
if (!exif_process_IFD_in_MAKERNOTE(ImageInfo, value_ptr, byte_count, info, displacement)) {
34503498
EFREE_IF(outside);
34513499
return FALSE;
34523500
}
@@ -3482,13 +3530,14 @@ static int exif_process_IFD_TAG(image_info_type *ImageInfo, char *dir_entry, cha
34823530
sub_section_index = SECTION_INTEROP;
34833531
break;
34843532
}
3485-
Subdir_start = offset_base + php_ifd_get32u(value_ptr, ImageInfo->motorola_intel);
3486-
if (Subdir_start < offset_base || Subdir_start > offset_base+IFDlength) {
3533+
offset_val = php_ifd_get32u(value_ptr, ImageInfo->motorola_intel);
3534+
Subdir_start = exif_offset_info_try_get(info, offset_val, 0);
3535+
if (!Subdir_start) {
34873536
exif_error_docref("exif_read_data#error_ifd" EXIFERR_CC, ImageInfo, E_WARNING, "Illegal IFD Pointer");
34883537
EFREE_IF(outside);
34893538
return FALSE;
34903539
}
3491-
if (!exif_process_IFD_in_JPEG(ImageInfo, Subdir_start, offset_base, IFDlength, displacement, sub_section_index, tag)) {
3540+
if (!exif_process_IFD_in_JPEG(ImageInfo, Subdir_start, info, displacement, sub_section_index, tag)) {
34923541
EFREE_IF(outside);
34933542
return FALSE;
34943543
}
@@ -3506,7 +3555,7 @@ static int exif_process_IFD_TAG(image_info_type *ImageInfo, char *dir_entry, cha
35063555

35073556
/* {{{ exif_process_IFD_in_JPEG
35083557
* Process one of the nested IFDs directories. */
3509-
static int exif_process_IFD_in_JPEG(image_info_type *ImageInfo, char *dir_start, char *offset_base, size_t IFDlength, size_t displacement, int section_index, int tag)
3558+
static int exif_process_IFD_in_JPEG(image_info_type *ImageInfo, char *dir_start, const exif_offset_info *info, size_t displacement, int section_index, int tag)
35103559
{
35113560
int de;
35123561
int NumDirEntries;
@@ -3518,21 +3567,21 @@ static int exif_process_IFD_in_JPEG(image_info_type *ImageInfo, char *dir_start,
35183567

35193568
ImageInfo->sections_found |= FOUND_IFD0;
35203569

3521-
if ((dir_start + 2) > (offset_base+IFDlength)) {
3570+
if (!exif_offset_info_contains(info, dir_start, 2)) {
35223571
exif_error_docref("exif_read_data#error_ifd" EXIFERR_CC, ImageInfo, E_WARNING, "Illegal IFD size");
35233572
return FALSE;
35243573
}
35253574

35263575
NumDirEntries = php_ifd_get16u(dir_start, ImageInfo->motorola_intel);
35273576

3528-
if ((dir_start+2+NumDirEntries*12) > (offset_base+IFDlength)) {
3529-
exif_error_docref("exif_read_data#error_ifd" EXIFERR_CC, ImageInfo, E_WARNING, "Illegal IFD size: x%04X + 2 + x%04X*12 = x%04X > x%04X", (int)((size_t)dir_start+2-(size_t)offset_base), NumDirEntries, (int)((size_t)dir_start+2+NumDirEntries*12-(size_t)offset_base), IFDlength);
3577+
if (!exif_offset_info_contains(info, dir_start+2, NumDirEntries*12)) {
3578+
exif_error_docref("exif_read_data#error_ifd" EXIFERR_CC, ImageInfo, E_WARNING, "Illegal IFD size: x%04X + 2 + x%04X*12 = x%04X > x%04X", (int)((size_t)dir_start+2-(size_t)info->valid_start), NumDirEntries, (int)((size_t)dir_start+2+NumDirEntries*12-(size_t)info->valid_start), info->valid_end - info->valid_start);
35303579
return FALSE;
35313580
}
35323581

35333582
for (de=0;de<NumDirEntries;de++) {
35343583
if (!exif_process_IFD_TAG(ImageInfo, dir_start + 2 + 12 * de,
3535-
offset_base, IFDlength, displacement, section_index, 1, exif_get_tag_table(section_index))) {
3584+
info, displacement, section_index, 1, exif_get_tag_table(section_index))) {
35363585
return FALSE;
35373586
}
35383587
}
@@ -3546,7 +3595,7 @@ static int exif_process_IFD_in_JPEG(image_info_type *ImageInfo, char *dir_start,
35463595
* Hack to make it process IDF1 I hope
35473596
* There are 2 IDFs, the second one holds the keys (0x0201 and 0x0202) to the thumbnail
35483597
*/
3549-
if ((dir_start+2+12*de + 4) > (offset_base+IFDlength)) {
3598+
if (!exif_offset_info_contains(info, dir_start+2+NumDirEntries*12, 4)) {
35503599
exif_error_docref("exif_read_data#error_ifd" EXIFERR_CC, ImageInfo, E_WARNING, "Illegal IFD size");
35513600
return FALSE;
35523601
}
@@ -3556,16 +3605,16 @@ static int exif_process_IFD_in_JPEG(image_info_type *ImageInfo, char *dir_start,
35563605
}
35573606

35583607
if (NextDirOffset) {
3559-
/* the next line seems false but here IFDlength means length of all IFDs */
3560-
if (offset_base + NextDirOffset < offset_base || offset_base + NextDirOffset > offset_base+IFDlength) {
3608+
char *next_dir_start = exif_offset_info_try_get(info, NextDirOffset, 0);
3609+
if (!next_dir_start) {
35613610
exif_error_docref("exif_read_data#error_ifd" EXIFERR_CC, ImageInfo, E_WARNING, "Illegal IFD offset");
35623611
return FALSE;
35633612
}
35643613
/* That is the IFD for the first thumbnail */
35653614
#ifdef EXIF_DEBUG
35663615
exif_error_docref(NULL EXIFERR_CC, ImageInfo, E_NOTICE, "Expect next IFD to be thumbnail");
35673616
#endif
3568-
if (exif_process_IFD_in_JPEG(ImageInfo, offset_base + NextDirOffset, offset_base, IFDlength, displacement, SECTION_THUMBNAIL, 0)) {
3617+
if (exif_process_IFD_in_JPEG(ImageInfo, next_dir_start, info, displacement, SECTION_THUMBNAIL, 0)) {
35693618
#ifdef EXIF_DEBUG
35703619
exif_error_docref(NULL EXIFERR_CC, ImageInfo, E_NOTICE, "Thumbnail size: 0x%04X", ImageInfo->Thumbnail.size);
35713620
#endif
@@ -3574,7 +3623,7 @@ static int exif_process_IFD_in_JPEG(image_info_type *ImageInfo, char *dir_start,
35743623
&& ImageInfo->Thumbnail.offset
35753624
&& ImageInfo->read_thumbnail
35763625
) {
3577-
exif_thumbnail_extract(ImageInfo, offset_base, IFDlength);
3626+
exif_thumbnail_extract(ImageInfo, info);
35783627
}
35793628
return TRUE;
35803629
} else {
@@ -3591,6 +3640,7 @@ static int exif_process_IFD_in_JPEG(image_info_type *ImageInfo, char *dir_start,
35913640
static void exif_process_TIFF_in_JPEG(image_info_type *ImageInfo, char *CharBuf, size_t length, size_t displacement)
35923641
{
35933642
unsigned exif_value_2a, offset_of_ifd;
3643+
exif_offset_info info;
35943644

35953645
/* set the thumbnail stuff to nothing so we can test to see if they get set up */
35963646
if (memcmp(CharBuf, "II", 2) == 0) {
@@ -3620,7 +3670,8 @@ static void exif_process_TIFF_in_JPEG(image_info_type *ImageInfo, char *CharBuf,
36203670

36213671
ImageInfo->sections_found |= FOUND_IFD0;
36223672
/* First directory starts at offset 8. Offsets starts at 0. */
3623-
exif_process_IFD_in_JPEG(ImageInfo, CharBuf+offset_of_ifd, CharBuf, length/*-14*/, displacement, SECTION_IFD0, 0);
3673+
exif_offset_info_init(&info, CharBuf, CharBuf, length/*-14*/);
3674+
exif_process_IFD_in_JPEG(ImageInfo, CharBuf+offset_of_ifd, &info, displacement, SECTION_IFD0, 0);
36243675

36253676
#ifdef EXIF_DEBUG
36263677
exif_error_docref(NULL EXIFERR_CC, ImageInfo, E_NOTICE, "Process TIFF in JPEG done");
@@ -4116,9 +4167,12 @@ static int exif_process_IFD_in_TIFF(image_info_type *ImageInfo, size_t dir_offse
41164167
exif_error_docref(NULL EXIFERR_CC, ImageInfo, E_NOTICE, "Next IFD: %s done", exif_get_sectionname(sub_section_index));
41174168
#endif
41184169
} else {
4119-
if (!exif_process_IFD_TAG(ImageInfo, (char*)dir_entry,
4120-
(char*)(ImageInfo->file.list[sn].data-dir_offset),
4121-
ifd_size, 0, section_index, 0, tag_table)) {
4170+
exif_offset_info info;
4171+
exif_offset_info_init(&info,
4172+
(char *) (ImageInfo->file.list[sn].data - dir_offset),
4173+
(char *) ImageInfo->file.list[sn].data, ifd_size);
4174+
if (!exif_process_IFD_TAG(ImageInfo, (char*)dir_entry, &info,
4175+
0, section_index, 0, tag_table)) {
41224176
return FALSE;
41234177
}
41244178
}

0 commit comments

Comments
 (0)