Bug#1002621: gif2apng: Stack based overflow in the handling of input and output file name

[Kolja Grassmann <koljagrassmann@mailbox.org>]

Package: gif2apng
Version: 1.9+srconly-3
Severity: important

Dear Maintainer,

I found a stack based overflow in the handling of the command line arguments in the gif2apng package.

The responsible code looks as follows:

gif2apng.cpp (line 421-518):
int main(int argc, char** argv)
{
   [...]
   char                   szIn[256];
   char                   szOut[256];
   [...]
   szIn[0] = 0;
   szOut[0] = 0;

   for (i=1; i<argc; i++)
   {
      szOpt = argv[i];

      if (szOpt[0] == '/' || szOpt[0] == '-')
      {
            [...]
      }
      else
      if (szIn[0] == 0)
         strcpy(szIn, szOpt, 255);
      else
      if (szOut[0] == 0)
         strcpy(szOut, szOpt);
   }

As can be seen there are two buffers of size 256. The command line arguments for the input and output file names are copied into these buffers without any validation of their size. Providing a large enough filename will overwrite data
on the stack, that is located after the buffer.

To test this I provided a large filename to the program as a file name as follows:
$gif2apng $(python3 -c "print('A'*10000)")

This led to to the following output:
*** buffer overflow detected ***: terminated

This indicates, that the stack canary placed on the stack by gcc was overwritten by the input. So there seems to be a memory corruption issue here, which could allow the execution of arbitrary code in the context of the program depending on the other data on the stack or in combination with a memory leak.

I tested this after installing the current version from the testing repositories on Debian 10.

Patch suggestion:

I think it should be enough to use strncpy instead of strcpy here and to ensure, that the string is zero terminated. There is a case later in the code where the input will be copied into an other buffer again using strcpy. This should also be changed, so that no out of bounds write is possible at this location. I changed the lines 512 to 532 of the gif2apng.cpp file as follows:

[...]
      if (szIn[0] == 0)
         strncpy(szIn, szOpt, 255); // strcpy ->  strncpy
      else
      if (szOut[0] == 0)
         strncpy(szOut, szOpt, 255); // strcpy ->  strncpy
   }
   szIn[255] = 0; // Ensure, that the string is terminated with a zero byte
   szOut[255] = 0;
   if (deflate_method == 0)
      printf(" using ZLIB\n\n");
   else if (deflate_method == 1)
      printf(" using 7ZIP with %d iterations\n\n", iter);
   else if (deflate_method == 2)
      printf(" using ZOPFLI with %d iterations\n\n", iter);

   if (szOut[0] == 0)
   {
      strncpy(szOut, szIn, 251); // Ensure, that with .png appended the string
still fits into the buffer
      if ((szExt = strrchr(szOut, '.')) != NULL) *szExt = 0;
      strcat(szOut, ".png");
   }
[...]

I was able to compile the code with these changes and the issue seemed to be fixed.

Best regards
Kolja

-- System Information:
Debian Release: 10.11
  APT prefers oldstable-updates
  APT policy: (500, 'oldstable-updates'), (500, 'oldstable')
Architecture: amd64 (x86_64)

Kernel: Linux 4.19.0-18-amd64 (SMP w/8 CPU cores)
Kernel taint flags: TAINT_OOT_MODULE, TAINT_UNSIGNED_MODULE
Locale: LANG=de_DE.UTF-8, LC_CTYPE=de_DE.UTF-8 (charmap=UTF-8), LANGUAGE=de_DE.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /usr/bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled

Versions of packages gif2apng depends on:
ii  libc6       2.28-10
ii  libzopfli1  1.0.2-1
ii  zlib1g      1:1.2.11.dfsg-1

gif2apng recommends no packages.

Versions of packages gif2apng suggests:
pn  apng2gif  <none>

-- no debconf information