fix(tiff): fix buffer overrun and make better error reporting#5082
fix(tiff): fix buffer overrun and make better error reporting#5082lgritz merged 1 commit intoAcademySoftwareFoundation:mainfrom
Conversation
Certain CMYK files (maybe only corrupt ones?) hit an edge case that caused us to read the original 4 channels but into space for only the 3 channels we ultimately report back. Fix that logic. Also noticed several spots where we didn't correctly report back error messages for newer versions of libtiff. This was some kind of oversight from long ago. Signed-off-by: Larry Gritz <lg@larrygritz.com>
lgritz
force-pushed
the
lg-tiff-cmyk-fix
branch
from
306aa76 to
5b16b8e
Compare
jessey-git
left a comment
jessey-git
left a comment
There was a problem hiding this comment.
Core changes seem fine, but like some of the other PRs, there's a variety of arithmetic vulns still left. For TIFF, there's many calculations like (1 << m_bitspersample) that will give the wrong result for m_bitspersample >= 32. It needs to be (1ull << m_bitspersample)
I haven't checked the other calculations but with TIFFs very high channel count limit, there might be other expressions that overflow too.
|
How about if I split it and leave this as-is to fix the immediate problem that was reported, and I will make a second PR that does a more comprehensive check for overflows and fixex for them? |
|
It turns out that the Actually, reading the TIFF 6.0 spec, on p. 23, it sure looks like palette images can only be 4 or 8 bits per sample. But the libtiff set of test images indeed has 2, 4, 8, and yes, 16 bps palette images. So I think we should support up to 16, since we might encounter it in the wild, but no higher. We don't currently check for this, though, so I will add that as a separate PR. |
|
Yeah, a followup PR is fine. Good that we're limited by the "spec" but I don't know if a hand-crafted file can worm its way to that point in the code. I'd opt for fixing those shifts if nothing else to prevent static analysis from flagging them (which is how I found them). |
|
I think that the static analysis won't flag it if it is preceded by a proper check and early exit if m_bitspersample > 16. |
jessey-git
left a comment
jessey-git
left a comment
There was a problem hiding this comment.
These changes are ready I think.
…ySoftwareFoundation#5082) Certain CMYK files (maybe only corrupt ones?) hit an edge case that caused us to read the original 4 channels but into space for only the 3 channels we ultimately report back. Fix that logic. Also noticed several spots where we didn't correctly report back error messages for newer versions of libtiff. This was some kind of oversight from long ago. Signed-off-by: Larry Gritz <lg@larrygritz.com>
…ySoftwareFoundation#5082) Certain CMYK files (maybe only corrupt ones?) hit an edge case that caused us to read the original 4 channels but into space for only the 3 channels we ultimately report back. Fix that logic. Also noticed several spots where we didn't correctly report back error messages for newer versions of libtiff. This was some kind of oversight from long ago. Signed-off-by: Larry Gritz <lg@larrygritz.com> Signed-off-by: Vlad (Kuzmin) Erium <libalias@gmail.com>
…ySoftwareFoundation#5082) Certain CMYK files (maybe only corrupt ones?) hit an edge case that caused us to read the original 4 channels but into space for only the 3 channels we ultimately report back. Fix that logic. Also noticed several spots where we didn't correctly report back error messages for newer versions of libtiff. This was some kind of oversight from long ago. Signed-off-by: Larry Gritz <lg@larrygritz.com> Signed-off-by: Vlad (Kuzmin) Erium <libalias@gmail.com> Signed-off-by: Vlad <shaamaan@gmail.com>
…ySoftwareFoundation#5082) Certain CMYK files (maybe only corrupt ones?) hit an edge case that caused us to read the original 4 channels but into space for only the 3 channels we ultimately report back. Fix that logic. Also noticed several spots where we didn't correctly report back error messages for newer versions of libtiff. This was some kind of oversight from long ago. Signed-off-by: Larry Gritz <lg@larrygritz.com> Signed-off-by: Vlad <shaamaan@gmail.com>
2 participants
Certain CMYK files (maybe only corrupt ones?) hit an edge case that caused us to read the original 4 channels but into space for only the 3 channels we ultimately report back. Fix that logic.
Also noticed several spots where we didn't correctly report back error messages for newer versions of libtiff. This was some kind of oversight from long ago.