Skip to content

fix(targa): Protection against corrupt, mis-sized palette #5165

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

Merged
lgritz merged 2 commits into from
Apr 26, 2026
Merged

fix(targa): Protection against corrupt, mis-sized palette #5165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f911099
fix(targa): Protection against corrupt, mis-sized palette
lgritz Apr 23, 2026
e5fab92
Additional fixes: longstanding bug misunderstanding palette start off…
lgritz Apr 25, 2026
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
6 changes: 3 additions & 3 deletions src/targa.imageio/targa_pvt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ typedef struct {
uint8_t idlen; ///< image comment length
uint8_t cmap_type; ///< palette type
uint8_t type; ///< image type (see tga_image_type)
uint16_t cmap_first; ///< offset to first entry
uint16_t cmap_length; ///<
uint8_t cmap_size; ///< palette size
uint16_t cmap_first; ///< offset to first stored color entry
uint16_t cmap_length; ///< number of stored color map entries
uint8_t cmap_size; ///< bits per palette entry: 15, 16, 24, or 32
uint16_t x_origin; ///<
uint16_t y_origin; ///<
uint16_t width; ///< image width
Expand Down
15 changes: 11 additions & 4 deletions src/targa.imageio/targainput.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,6 @@ TGAInput::open(const std::string& name, ImageSpec& newspec)
errorfmt("Palette image with no palette");
return false;
}

if (is_palette()) {
if (m_tga.type == TYPE_GRAY || m_tga.type == TYPE_GRAY_RLE) {
// it should be an error for TYPE_RGB* as well, but apparently some
Expand All @@ -216,6 +215,12 @@ TGAInput::open(const std::string& name, ImageSpec& newspec)
errorfmt("Illegal palette entry size: {} bits", m_tga.cmap_size);
return false;
}
if (m_tga.cmap_first + m_tga.cmap_length > (uint64_t(1) << m_tga.bpp)) {
Copy link
Copy Markdown
Contributor

@jessey-git jessey-git Apr 25, 2026

Choose a reason for hiding this comment

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

I would naively assume upon reading this conditional that either the addition, or direction of the inequality, was wrong. Or maybe that it should be if (first + number_of_potential_values > length) { fail } instead? Could it not be the case that a file uses start = 200 and length 256 and would still be valid for an 8bpp image - assuming that the image never used values > 56?

Copy link
Copy Markdown
Collaborator Author

@lgritz lgritz Apr 25, 2026

Choose a reason for hiding this comment

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

Your question forced me to admit that my understanding here was shaky, so I did some research and I think our code is wrong (and always has been). Here's what I learned, assuming I understand it correctly now.

As you probably know, TARGA files come from Truevision, who made early PC frame buffers / accelerators. The hardware palette registers were a shared resource, and often the OS or other hardware features wanted to reserve the first 16 or whatever and not have those change, nor store them in the file. The 'start' value is the destination offset into a hardware LUT where your palette should be copied.

Near as I can tell -- and contrary to our buggy code! -- the indices in the image pixels are absolute, not relative to the palette's start value. So if the (first+length) is more than can be addressed by the bpp, something has gone awry, because those high indices can never be specified in the pixels.

It seems that our current code in decode_pixel is wrong, it's adding the indices in the file to the start offset, but I now believe that it shouldn't be doing that. In fact, since we only allocate and store the ones beginning with 'start', what we should be doing is subtracting the start value to index into our own array.

Of course, it's been decades since Truevision boards, so anybody making TGA files is not worried about overwriting reserved LUT entries in the hardware, and thus there is no reason to reserve palette value and have the start value be anything other than 0. This is probably why we've never discovered this issue before.

I will revise to fix these bugs as well.

Copy link
Copy Markdown
Collaborator Author

@lgritz lgritz Apr 25, 2026

Choose a reason for hiding this comment

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

My copy of the Targa spec actually doesn't say whether the pixel values are absolute or relative indices. But I spot-checked some other software to see what they did, and it seems they all treat it as absolute. I suppose the only files you'd find with a nonzero start value are ones that date from the era where people were intending the image to run on a specific hardware board (because the number of palette registers you'd want to not disturb wasn't the same for all board models), and that's the reason we never came across this condition in practice.

errorfmt(
"Too big a color palette ({}) for {} bpp, assume corruption",
m_tga.cmap_first + m_tga.cmap_length, m_tga.bpp);
return false;
}
}

m_alpha_type = TGA_ALPHA_NONE;
Expand Down Expand Up @@ -572,18 +577,20 @@ TGAInput::decode_pixel(unsigned char* in, unsigned char* out,
unsigned char* palette, int bytespp, int palbytespp,
size_t palette_alloc_size)
{
unsigned int k = 0;
uint64_t k = 0;
// I hate nested switches...
switch (m_tga.type) {
case TYPE_PALETTED:
case TYPE_PALETTED_RLE:
for (int i = 0; i < bytespp; ++i)
k |= in[i] << (8 * i); // Assemble it in little endian order
k = (m_tga.cmap_first + k) * palbytespp;
if (k + palbytespp > palette_alloc_size) {
if (k < m_tga.cmap_first || k >= m_tga.cmap_first + m_tga.cmap_length) {
errorfmt("Corrupt palette index");
return false;
}
// The palette indices stored in the file's pixels are absolute, so
// subtract the start offset to correctly index from the palette.
k = (k - m_tga.cmap_first) * uint64_t(palbytespp);
switch (palbytespp) {
case 2:
// see the comment for 16bpp RGB below for an explanation of this
Expand Down
5 changes: 4 additions & 1 deletion testsuite/targa/ref/out.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,14 +201,17 @@ Full command line was:
oiiotool ERROR: read : "src/crash1707.tga": Invalid alpha type 138. Corrupted header?
Full command line was:
> oiiotool --oiioattrib try_all_readers 0 src/crash1707.tga -o crash1707.exr
oiiotool ERROR: read : "src/crash1708.tga": Corrupt palette index
oiiotool ERROR: read : "src/crash1708.tga": Too big a color palette (382) for 8 bpp, assume corruption
Full command line was:
> oiiotool --oiioattrib try_all_readers 0 src/crash1708.tga -o crash1708.exr
oiiotool ERROR: read : "src/crash3952.tga": Uncompressed image size 15231.5 MB exceeds the 1024 MB limit.
Image claimed to be 60927x65535, 4-channel uint8. Possible corrupt input?
If this is a valid file, raise the OIIO attribute "limits:imagesize_MB".
Full command line was:
> oiiotool --oiioattrib limits:imagesize_MB 1024 --oiioattrib try_all_readers 0 src/crash3952.tga -o crash3952.exr
oiiotool ERROR: read : "src/crash-20250423.tga": Corrupt palette index
Full command line was:
> oiiotool --oiioattrib try_all_readers 0 src/crash-20250423.tga -o out.null
Reading src/1x1.tga
src/1x1.tga : 1 x 1, 3 channel, uint2 targa
SHA-1: DB9237F28F9622F7B7E73B783A8822B63BDB3708
Expand Down
1 change: 1 addition & 0 deletions testsuite/targa/run.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
command += oiiotool("--oiioattrib limits:imagesize_MB 1024 "
"--oiioattrib try_all_readers 0 "
"src/crash3952.tga -o crash3952.exr", failureok = True)
command += oiiotool("--oiioattrib try_all_readers 0 src/crash-20250423.tga -o out.null", failureok=True);

# Test odds and ends, unusual files
command += rw_command("src", "1x1.tga")
Expand Down
Binary file added testsuite/targa/src/crash-20250423.tga
View file
Open in desktop
Binary file not shown.
Loading