Fix TGA RLE read buffer overflow and add EOF error detection (CVE-2021-36489)

[gusnan]

This is provided by co-pilot - I can provide the full co-pilot business if required.

Fix TGA RLE read buffer overflow and add EOF error detection
- Modify rle_tga_read8, rle_tga_read16, rle_tga_read24, rle_tga_read32 to return pointer (or NULL on error)
- Add EOF detection for packet header and value fields in all RLE routines
- Clamp packet lengths to prevent buffer overflow past scanline width (w)
- Mask out 1-bit alpha value in single_tga_read16 (value &= 0x7FFF)
- Update load_tga_pf to check for NULL returns and cleanup/abort on error

Fixes: Debian #1032670
Reference: liballeg/allegro5#1253
Reference: liballeg/allegro5@2c712d4

Add EOF checks after reading color values in RLE routines
Use pack_feof() to detect EOF after single_tga_read32/24/16 calls
in the run-length packet handling code.

Add EOF checks after raw packet reads in RLE routines
Use pack_feof() to detect EOF after raw_tga_read* calls in the
raw packet handling code paths for all RLE routines.

Add EOF error detection to PCX RLE decode
Add EOF checks in the PCX RLE decode loop to prevent issues when
loading truncated or malformed PCX files. Similar to the TGA fixes,
this properly detects end-of-file conditions and returns NULL with
proper cleanup.

See https://security-tracker.debian.org/tracker/CVE-2021-36489

[gusnan]

See #37

[SiegeLord]

Hmm... this looks weird in places. E.g. this in A5:

static unsigned char *rle_tga_read8(unsigned char *b, int w, ALLEGRO_FILE *f)
{
   int value, count, c = 0;

   do {
      count = al_fgetc(f);
      if (count & 0x80) {
         /* run-length packet */
         count = (count & 0x7F) + 1;
         c += count;
         if (c > w) {
            /* Stepped past the end of the line, error */
            return NULL;
         }
         value = al_fgetc(f);
         while (count--)
            *b++ = value;
      }
      else {
         /* raw packet */
         count++;
         c += count;
         if (c > w) {
            /* Stepped past the end of the line, error */
            return NULL;
         }
         b = raw_tga_read8(b, count, f);
      }
   } while (c < w);
   return b;
}

vs this PR

static unsigned char *rle_tga_read8(unsigned char *b, int w, PACKFILE *f)
{
   int value, count, c = 0;

   do {
      count = pack_getc(f);
      if (count == EOF)
	 return NULL;
      if (count & 0x80) {
	 /* run-length packet */
	 count = (count & 0x7F) + 1;
	 if (c + count > w)
	    count = w - c;
	 c += count;
	 value = pack_getc(f);
	 if (value == EOF)
	    return NULL;
	 while (count--)
	    *b++ = value;
      }
      else {
	 /* raw packet */
	 count++;
	 if (c + count > w)
	    count = w - c;
	 c += count;
	 b = raw_tga_read8(b, count, f);
	 if (pack_feof(f))
	    return NULL;
      }
   } while (c < w);

   return b;
}

The if (c + count > w) is different, and I'm not sure which one's better.

[gusnan]

It is indeed. Do you want me to update the pull request to make the rle_tga_read* functions like in A5?

[SiegeLord]

Ideally they would be the same, and the better variant be chosen for both. I'd have to investigate what the correct behavior is. E.g. https://en.wikipedia.org/wiki/Truevision_TGA#Specification_discrepancies says some things which seem more consistent with this PR, but I don't want to necessarily believe that unequivocally, and would rather see some example TGA files that exhibit the issues.