Skip to content

Commit

Permalink
fix(targa): guard against corrupted tga files (#3768)
Browse files Browse the repository at this point in the history 
* Be more careful constructing strings from what we read in the header;
  corrupted files may make them not be zero-terminated.
  Fixes TALOS-2023-1707 / CVE-2023-24473

* Watch out for pixels with palette indices that exceed the size we
  allocated for the palette.
  Fixes TALOS-2023-1708 / CVE-2023-22845

* Check alpha_type for valid range.
  • Loading branch information
@lgritz
lgritz authored Feb 9, 2023
1 parent 2a6f2c8 commit 759fcd3
Show file tree
Hide file tree
Showing 6 changed files with 73 additions and 32 deletions.
3 changes: 2 additions & 1 deletion src/targa.imageio/targa_pvt.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ enum tga_alpha_type {
TGA_ALPHA_UNDEFINED_IGNORE = 1, ///< can ignore alpha
TGA_ALPHA_UNDEFINED_RETAIN = 2, ///< undefined, but should be retained
TGA_ALPHA_USEFUL = 3, ///< useful alpha data is present
TGA_ALPHA_PREMULTIPLIED = 4 ///< alpha is pre-multiplied (arrrgh!)
TGA_ALPHA_PREMULTIPLIED = 4, ///< alpha is pre-multiplied (arrrgh!)
TGA_ALPHA_INVALID // one past the last valid value
// values 5-127 are reserved
// values 128-255 are unassigned
};
Expand Down
94 changes: 63 additions & 31 deletions src/targa.imageio/targainput.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,9 @@ class TGAInput final : public ImageInput {
bool readimg();

/// Helper function: decode a pixel.
inline void decode_pixel(unsigned char* in, unsigned char* out,
inline bool decode_pixel(unsigned char* in, unsigned char* out,
unsigned char* palette, int bytespp,
int palbytespp);
int palbytespp, size_t palette_alloc_size);

// Read one byte
bool read(uint8_t& buf) { return ioread(&buf, sizeof(buf), 1); }
Expand All @@ -112,6 +112,21 @@ class TGAInput final : public ImageInput {
swap_endian(&buf);
return ok;
}

// Read maxlen bytes from the file, if it doesn't start with 0, add the
// contents as attribute `name`. Return true if the read was successful,
// false if there was an error reading from the file. The attribute is
// only added if the string is non-empty.
bool read_bytes_for_string_attribute(string_view name, size_t maxlen)
{
char* buf = OIIO_ALLOCA(char, maxlen);
OIIO_DASSERT(buf != nullptr);
if (!ioread(buf, maxlen))
return false;
if (buf[0])
m_spec.attribute(name, Strutil::safe_string_view(buf, maxlen));
return true;
}
};


Expand Down Expand Up @@ -188,19 +203,18 @@ TGAInput::open(const std::string& name, ImageSpec& newspec)
return false;
}

if (is_palette()
&& (m_tga.type == TYPE_GRAY || m_tga.type == TYPE_GRAY_RLE)) {
// it should be an error for TYPE_RGB* as well, but apparently some
// *very* old TGAs can be this way, so we'll hack around it
errorfmt("Palette defined for grayscale image");
return false;
}

if (is_palette()
&& (m_tga.cmap_size != 15 && m_tga.cmap_size != 16
&& m_tga.cmap_size != 24 && m_tga.cmap_size != 32)) {
errorfmt("Illegal palette entry size: {} bits", m_tga.cmap_size);
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
// *very* old TGAs can be this way, so we'll hack around it
errorfmt("Palette defined for grayscale image");
return false;
}
if (m_tga.cmap_size != 15 && m_tga.cmap_size != 16
&& m_tga.cmap_size != 24 && m_tga.cmap_size != 32) {
errorfmt("Illegal palette entry size: {} bits", m_tga.cmap_size);
return false;
}
}

m_alpha_type = TGA_ALPHA_NONE;
Expand Down Expand Up @@ -324,10 +338,8 @@ TGAInput::read_tga2_header()
} buf;

// load image author
if (!ioread(buf.c, 41, 1))
if (!read_bytes_for_string_attribute("Artist", 41))
return false;
if (buf.c[0])
m_spec.attribute("Artist", (char*)buf.c);

// load image comments
if (!ioread(buf.c, 324, 1))
Expand Down Expand Up @@ -365,10 +377,8 @@ TGAInput::read_tga2_header()
}

// job name/ID
if (!ioread(buf.c, 41, 1))
if (!read_bytes_for_string_attribute("DocumentName", 41))
return false;
if (buf.c[0])
m_spec.attribute("DocumentName", (char*)buf.c);

// job time
if (!ioread(buf.s, 2, 3))
Expand All @@ -390,7 +400,7 @@ TGAInput::read_tga2_header()
return false;
if (buf.c[0]) {
// tack on the version number and letter
std::string soft((const char*)buf.c);
std::string soft = Strutil::safe_string((const char*)buf.c, 41);
soft += Strutil::fmt::format(" {}.{}", n / 100, n % 100);
if (l != ' ')
soft += l;
Expand Down Expand Up @@ -458,6 +468,10 @@ TGAInput::read_tga2_header()
// alpha type
if (!ioread(buf.c, 1, 1))
return false;
if (buf.c[0] >= TGA_ALPHA_INVALID) {
errorfmt("Invalid alpha type {}. Corrupted header?", (int)buf.c[0]);
return false;
}
m_alpha_type = (tga_alpha_type)buf.c[0];
if (m_alpha_type)
m_spec.attribute("targa:alpha_type", m_alpha_type);
Expand Down Expand Up @@ -522,11 +536,13 @@ TGAInput::get_thumbnail(ImageBuf& thumb, int subimage)
alphabits = 8;
// read palette, if there is any
std::unique_ptr<unsigned char[]> palette;
size_t palette_alloc_size = 0;
if (is_palette()) {
if (!ioseek(m_ofs_palette)) {
return false;
}
palette.reset(new unsigned char[palbytespp * m_tga.cmap_length]);
palette_alloc_size = palbytespp * m_tga.cmap_length;
palette.reset(new unsigned char[palette_alloc_size]);
if (!ioread(palette.get(), palbytespp, m_tga.cmap_length))
return false;
if (!ioseek(m_ofs_thumb + 2)) {
Expand All @@ -542,7 +558,9 @@ TGAInput::get_thumbnail(ImageBuf& thumb, int subimage)
x++, img += m_spec.nchannels) {
if (!ioread(in, bytespp, 1))
return false;
decode_pixel(in, pixel, palette.get(), bytespp, palbytespp);
if (!decode_pixel(in, pixel, palette.get(), bytespp, palbytespp,
palette_alloc_size))
return false;
memcpy(img, pixel, m_spec.nchannels);
}
}
Expand All @@ -557,9 +575,10 @@ TGAInput::get_thumbnail(ImageBuf& thumb, int subimage)



inline void
inline bool
TGAInput::decode_pixel(unsigned char* in, unsigned char* out,
unsigned char* palette, int bytespp, int palbytespp)
unsigned char* palette, int bytespp, int palbytespp,
size_t palette_alloc_size)
{
unsigned int k = 0;
// I hate nested switches...
Expand All @@ -569,6 +588,10 @@ TGAInput::decode_pixel(unsigned char* in, unsigned char* out,
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) {
errorfmt("Corrupt palette index");
return false;
}
switch (palbytespp) {
case 2:
// see the comment for 16bpp RGB below for an explanation of this
Expand Down Expand Up @@ -643,6 +666,7 @@ TGAInput::decode_pixel(unsigned char* in, unsigned char* out,
memcpy(out, in, bytespp);
break;
}
return true;
}


Expand Down Expand Up @@ -710,8 +734,10 @@ TGAInput::readimg()

// read palette, if there is any
std::unique_ptr<unsigned char[]> palette;
size_t palette_alloc_size = 0;
if (is_palette()) {
palette.reset(new unsigned char[palbytespp * m_tga.cmap_length]);
palette_alloc_size = palbytespp * m_tga.cmap_length;
palette.reset(new unsigned char[palette_alloc_size]);
if (!ioread(palette.get(), palbytespp, m_tga.cmap_length))
return false;
}
Expand All @@ -725,7 +751,9 @@ TGAInput::readimg()
for (int64_t x = 0; x < m_spec.width; x++) {
if (!ioread(in, bytespp, 1))
return false;
decode_pixel(in, pixel, palette.get(), bytespp, palbytespp);
if (!decode_pixel(in, pixel, palette.get(), bytespp, palbytespp,
palette_alloc_size))
return false;
memcpy(m_buf.get() + y * m_spec.width * m_spec.nchannels
+ x * m_spec.nchannels,
pixel, m_spec.nchannels);
Expand All @@ -743,7 +771,9 @@ TGAInput::readimg()
return false;
}
packet_size = 1 + (in[0] & 0x7f);
decode_pixel(&in[1], pixel, palette.get(), bytespp, palbytespp);
if (!decode_pixel(&in[1], pixel, palette.get(), bytespp,
palbytespp, palette_alloc_size))
return false;
if (in[0] & 0x80) { // run length packet
// DBG("[tga] run length packet size {} @ ({},{})\n",
// packet_size, x, y);
Expand Down Expand Up @@ -786,8 +816,10 @@ TGAInput::readimg()
DBG("Failed on scanline {}\n", y);
return false;
}
decode_pixel(&in[1], pixel, palette.get(), bytespp,
palbytespp);
if (!decode_pixel(&in[1], pixel, palette.get(),
bytespp, palbytespp,
palette_alloc_size))
return false;
// DBG("\t\t@ ({},{}): [{:d} {:d} {:d} {:d}]\n", x, y,
// pixel[0], pixel[1], pixel[2], pixel[3]);
}
Expand Down
6 changes: 6 additions & 0 deletions testsuite/targa/ref/out.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,12 @@ Full command line was:
oiiotool ERROR: read : "src/crash6.tga": Read error: hit end of file
Full command line was:
> oiiotool --oiioattrib try_all_readers 0 src/crash6.tga -o crash6.exr
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
Full command line was:
> oiiotool --oiioattrib try_all_readers 0 src/crash1708.tga -o crash1707.exr
Reading src/1x1.tga
src/1x1.tga : 1 x 1, 3 channel, uint2 targa
SHA-1: DB9237F28F9622F7B7E73B783A8822B63BDB3708
Expand Down
2 changes: 2 additions & 0 deletions testsuite/targa/run.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
command += oiiotool("--oiioattrib try_all_readers 0 src/crash4.tga -o crash4.exr", failureok = True)
command += oiiotool("--oiioattrib try_all_readers 0 src/crash5.tga -o crash5.exr", failureok = True)
command += oiiotool("--oiioattrib try_all_readers 0 src/crash6.tga -o crash6.exr", failureok = True)
command += oiiotool("--oiioattrib try_all_readers 0 src/crash1707.tga -o crash1707.exr", failureok = True)
command += oiiotool("--oiioattrib try_all_readers 0 src/crash1708.tga -o crash1707.exr", failureok = True)

# Test odds and ends, unusual files
command += rw_command("src", "1x1.tga")
Expand Down
Binary file added testsuite/targa/src/crash1707.tga
View file
Binary file not shown.
Binary file added testsuite/targa/src/crash1708.tga
View file
Binary file not shown.

0 comments on commit 759fcd3

Please sign in to comment.