Skip to content
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.

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

fix: more ICC protections against invalid tag sizes #4565

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Dec 17, 2024

No description provided.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@@ -20,3 +20,27 @@ src/corrupt-exif-1626.jpg : 256 x 256, 3 channel, uint8 jpeg
oiio:ColorSpace: "sRGB"
corrupt-icc-4551.jpg
DCT coefficient (lossy) or spatial difference (lossless) out of range
Reading src/corrupt-icc-4552.jpg
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick question: since the file we're reading here is supposed to be corrupted, shouldn't we be failing when reading it ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, there's a balance -- if there's something wrong with one tag in the ICC profile, which is just one (non-essential to many apps) piece of metadata in the file, do we press on and try to read everything else that we can for the file? Or upon the first problem, no matter how minor, do we conservatively abort because it might be a sign that the whole file is suspicious or maybe even malicious? We usually err on the side of trying to read as much as we can, unless the whole file is unrecoverably broken.

This is the point of #4560: adding an attribute that, in the future, will give an app using OIIO more control over tradeoff I just described.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense !
LGTM then :)

@lgritz lgritz merged commit 57de455 into AcademySoftwareFoundation:main Dec 20, 2024
29 checks passed
@lgritz lgritz deleted the lg-icc branch December 23, 2024 06:27
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Dec 23, 2024
@lgritz
Copy link
Collaborator Author

lgritz commented Jan 4, 2025

Noting for the record that this turns out to also have fixed #4450.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants