fix: Detect invalid ICC profile tags #4557
Conversation
Affects jpeg, tiff, and other formats with embedded ICC profiles. Corrupted/invalid tag offset and/or length values weres causing us to touch memory outside the profile. Fixes 4551 Signed-off-by: Larry Gritz <[email protected]>
|
Fixes #4551 |
| // Nah, just skip an ICC specific error? | ||
| errorfmt("Possible corrupt file, could not decode ICC profile: {}\n", | ||
| errormsg); | ||
| return false; |
There was a problem hiding this comment.
Initially I was a bit concerned that this would prevent loading of the jpeg itself as you alluded to, however, it looks like the reader here doesn't care about the return value of this function, so I think the image will still be read but would be up to the discretion of the original caller to see if they care about the error status being set or not... It's a bit risky as you say but I don't have a large corpus of jpegs around to know the extent of what might be lurking in terms of folks complaining about it.
There was a problem hiding this comment.
Yeah, I honestly don't know the right balance between trying to muddle through reading files that are partially broken, versus being more security-conservative and stopping as soon as something is clearly invalid in the file.
Perhaaps the right thing thing (in the long run, not in this PR) is to add a global attribute about whether/how much to be strict about abandoning reads of files that have anything invalid in them.
Signed-off-by: Larry Gritz <[email protected]>
Affects jpeg, tiff, and other formats with embedded ICC profiles. Corrupted/invalid tag offset and/or length values were causing us to touch memory outside the memory allocated for the profile. Fixes AcademySoftwareFoundation#4551 Not sure if this is wise or not, but for JPEGs, I also enabled some previously commented-out code that has the effect of treating a file with a verifiably corrupted/nonsensical ICC profile block as a failed file. If people ever complain about this in the future, maybe we'll come back and add some kind of global option that lets the app express how tolerant OIIO readers should be about corruptions in files -- do you assume that the whole file is bogus or malicious upon discovering the first invalidity, or do you press on and hope you can get something else useful from the pixels? Let's be cautious for now. --------- Signed-off-by: Larry Gritz <[email protected]>
Affects jpeg, tiff, and other formats with embedded ICC profiles. Corrupted/invalid tag offset and/or length values were causing us to touch memory outside the memory allocated for the profile. Fixes AcademySoftwareFoundation#4551 Not sure if this is wise or not, but for JPEGs, I also enabled some previously commented-out code that has the effect of treating a file with a verifiably corrupted/nonsensical ICC profile block as a failed file. If people ever complain about this in the future, maybe we'll come back and add some kind of global option that lets the app express how tolerant OIIO readers should be about corruptions in files -- do you assume that the whole file is bogus or malicious upon discovering the first invalidity, or do you press on and hope you can get something else useful from the pixels? Let's be cautious for now. --------- Signed-off-by: Larry Gritz <[email protected]>
Affects jpeg, tiff, and other formats with embedded ICC profiles. Corrupted/invalid tag offset and/or length values were causing us to touch memory outside the memory allocated for the profile.
Fixes #4551
Not sure if this is wise or not, but for JPEGs, I also enabled some previously commented-out code that has the effect of treating a file with a verifiably corrupted/nonsensical ICC profile block as a failed file. If people ever complain about this in the future, maybe we'll come back and add some kind of global option that lets the app express how tolerant OIIO readers should be about corruptions in files -- do you assume that the whole file is bogus or malicious upon discovering the first invalidity, or do you press on and hope you can get something else useful from the pixels? Let's be cautious for now.