Bug #71534 [Csd]: Type confusion in exif_read_data() leading to heap overflow in debug mode

Edit report at https://bugs.php.net/bug.php?id=71534&edit=1 ID: 71534 User updated by: hlt99 at blinkenshell dot org Reported by: hlt99 at blinkenshell dot org Summary: Type confusion in exif_read_data() leading to heap overflow in debug mode Status: Closed Type: Bug Package: EXIF related Operating System: Arch Linux (64-bit) PHP Version: 7.0.3 Assigned To: kalle Block user comment: N Private report: N New Comment: Looks good to me! However the commit message is inaccurate/misleading. This type confusion bug is present in both debug mode *and* non-debug mode. In order to demonstrate possible security implications of this particular bug, I chained it together with another flaw (#71535 [1]) to obtain arbitrary heap memory overwrite. This other bug was only present in debug mode and has already been patched in commit 0b9c87a [2]. Since this february commit the heap issue in debug mode was mitigated, but left this original bug unpatched until now. I hope this clarifies the now somewhat misleading title of this report. [1] https://bugs.php.net/bug.php?id=71535 [2] http://git.php.net/?p=php-src.git;a=commit;h=0b9c87a02bacfbf1d1383ad393bda78e5d65570c Previous Comments: ------------------------------------------------------------------------ [2016-08-07 03:42:01] kalle@php.net I committed a fix based on your patch to master (7.2.0), could you please check it out and confirm? Thanks for making PHP even greater! ------------------------------------------------------------------------ [2016-08-07 03:41:20] kalle@php.net Automatic comment on behalf of kalle Revision: http://git.php.net/?p=php-src.git;a=commit;h=af56fed73b6a2f07127f48e05aa4837bfc06c42d Log: Fixed bug #71534 (Type confusion in exif_read_data() leading to heap overflow in debug mode) ------------------------------------------------------------------------ [2016-08-06 07:40:53] hlt99 at blinkenshell dot org Sample poc.tiff reuploaded. From my memories: These were the two code paths I found that crashed PHP. To maintain the smallest possible footprint of my naive patch attempt I added the casts to unsigned types only there. However I'm quite sure I did not check the others. So consider it as an oversight. ------------------------------------------------------------------------ [2016-08-05 08:17:07] kalle@php.net Hi Thanks a lot for the patch! I can kinda follow were you are going with this and I think I can work with that. As you note in the patch, returning TAG_FMT_UNDEFINED can have a side effect, since it is used at the end of the big if/else as we pass it to exif_iif_add_tag(). Another thing I was wondering about, is that in your patch, you only cast it to an unsigned int before two instances of exif_convert_any_to_int() (in case the ImageInfo->Thumbnail.data hits either TAG_JPEG_INTERCHANGE_FORMAT_LEN or TAG_STRIP_BYTE_COUNTS), but we still have a few others, is there any reasoning behind this or just an oversight? Oh and one last favor, the PoC.tiff, could you also re-upload that so I can toy around with it? ------------------------------------------------------------------------ [2016-08-05 07:23:32] hlt99 at blinkenshell dot org Please take this patch with a grain of salt as it may have unintended side effects! I merely used it to prevent afl-fuzz from running into the crash over and over again. Now, after you've been warned: patch reuploaded. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=71534 -- Edit this bug report at https://bugs.php.net/bug.php?id=71534&edit=1