Skip to content

Cleanup exif checks #10402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Cleanup exif checks #10402

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d3510b
Remove duplicated length check in exif
nielsdos Jan 21, 2023
36a73fa
Remove always false condition from exif
nielsdos Jan 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions ext/exif/exif.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3679,11 +3679,6 @@ static void exif_process_TIFF_in_JPEG(image_info_type *ImageInfo, char *CharBuf,
return;
}

if (length < 2) {
exif_error_docref(NULL EXIFERR_CC, ImageInfo, E_WARNING, "Missing TIFF alignment marker");
return;
}

/* set the thumbnail stuff to nothing so we can test to see if they get set up */
if (memcmp(CharBuf, "II", 2) == 0) {
ImageInfo->motorola_intel = 0;
Expand Down Expand Up @@ -3810,12 +3805,6 @@ static bool exif_scan_JPEG_header(image_info_type *ImageInfo)

fpos = php_stream_tell(ImageInfo->infile);

if (marker == 0xff) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could it be still somewhat useful (e.g. fuzzing) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand your question. This check can never be hit because of the do-while loop: if we get here marker must be !=0xff. Unless this code was meant to check in a different way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind I misread the code it s clearer when unfolded.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good idea to replace that with a ZEND_ASSERT(), so it's easier to understand (and to also have a safeguard just in case the code above would be modified).

/* 0xff is legal padding, but if we get that many, something's wrong. */
exif_error_docref(NULL EXIFERR_CC, ImageInfo, E_WARNING, "To many padding bytes");
return false;
}

/* Read the length of the section. */
if ((lh = php_stream_getc(ImageInfo->infile)) == (unsigned int)EOF) {
EXIF_ERRLOG_CORRUPT(ImageInfo)
Expand Down