Skip to content
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.

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

memory exhaustion in ReadTGAImage #472

Closed
jgj212 opened this Issue May 4, 2017 · 8 comments

Comments

Projects
None yet
4 participants
@jgj212
Copy link
Contributor

commented May 4, 2017

ImageMagick 7.0.5-6

$magick identify $FILE

When identify VST file, imagemagick will allocate memory to store data in function ReadTGAImage in coders\tga.c (line 274)

if (image->storage_class == PseudoClass)  // line 263, can be controlled 
{
	...

          size_t
            one;  // line 270

          one=1; // line 272
          image->colors=one << tga_info.bits_per_pixel; // line 273, can be controlled
          if (AcquireImageColormap(image,image->colors,exception) == MagickFalse)  // line 274
	
	...
}

tga_info.bits_per_pixel is diretly from VST file without checking in tga.c (line 231):

    tga_info.bits_per_pixel=(unsigned char) ReadBlobByte(image);        // can be controlled by modify VST file

By review the founction code, tga_info.bits_per_pixel max valid value is 32.
On 32bit os, size_t one will be 32bit, so image->colors can be overflow to 0.
On 64bit os, size_t one will be 64bit, so image->colors can be large as 0x100000000(64GB).

Normally, this will not cause problem because image->storage_class is equal PseudoClass.
But image->storage_class is also can be controlled , it is assigned as follow

  if ((tga_info.image_type == TGAColormap) ||
      (tga_info.image_type == TGAMonochrome) ||
      (tga_info.image_type == TGARLEColormap) ||
      (tga_info.image_type == TGARLEMonochrome))  // image_type can be controlled
    image->storage_class=PseudoClass;

image_type is diretly from VST file

  tga_info.image_type=(TGAImageType) ReadBlobByte(image);

Memory allocation is earlly than the security checking

  status=SetImageExtent(image,image->columns,image->rows,exception); // line 320

So, modifying the image_type and bits_per_pixel can cause ImageMagick to allocate a large amount of memory, this may cause a memory exhaustion

Reproducer: https://github.com/jgj212/poc/blob/master/ImageMagick-7.0.5-6-memory-exhaustion.VST
Credit: ADLab of Venustech

@mikayla-grace

This comment has been minimized.

Copy link

commented May 6, 2017

With the latest IM 7.0.5-6 compiled with afl-clang, we cannot reproduce the problem you posted. We get expected results:

$ magick identify ImageMagick-7.0.5-6-memory-exhaustion.VST
identify: improper image header `ImageMagick-7.0.5-6-memory-exhaustion.VST' @ error/tga.c/ReadTGAImage/237
@jgj212

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2017

@mikayla-grace Are you testing on 32bit os or 64 bit os? On 32bit os, the overflow will not happend, this may cause this code path will not hit. I will test it again, and i use gcc to compile the IM 7.0.5-6

@dlemstra

This comment has been minimized.

Copy link
Member

commented May 7, 2017

Just tested this on a 64-bit build of Windows:

D:\>convert -version
Version: ImageMagick 7.0.5-6 Q16 x64 2017-05-07 http://www.imagemagick.org
Copyright: Copyright (C) 1999-2015 ImageMagick Studio LLC
License: http://www.imagemagick.org/script/license.php
Visual C++: 191025019
Features: Debug Cipher DPC HDRI Modules OpenCL OpenMP
Delegates (built-in): bzlib cairo flif freetype jng jp2 jpeg lcms lqr openexr pangocairo png ps rsvg tiff webp xml zlib

D:\>convert ImageMagick-7.0.5-6-memory-exhaustion.VST null:
convert: improper image header `ImageMagick-7.0.5-6-memory-exhaustion.VST' @ error/tga.c/ReadTGAImage/275.
convert: no images defined `null:' @ error/convert.c/ConvertImageCommand/3254.

And with a debugger I get the following values:

image->colors=one << tga_info.bits_per_pixel; // 4294967296
if (image->colors > ((~0UL)/sizeof(*image->colormap))) // 4294967296 > 48806446
  ThrowReaderException(CorruptImageError,"ImproperImageHeader");
@jgj212

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2017

this line is right
image->colors=one << tga_info.bits_per_pixel; // 4294967296

i am not sure whether next line will trigger before allocate memory with size_0x100000000
if (image->colors > ((~0UL)/sizeof(*image->colormap))) // 4294967296 > 48806446

@dlemstra thank you. i will test it ago at tomorrow.

@jgj212

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2017

@dlemstra @mikayla-grace I found that Cristy commit a patch to this issue a day ago. Maybe he forget to
reference to this issue.

It is here :
fbb5e1c

@dlemstra dlemstra added the bug label May 8, 2017

@dlemstra dlemstra closed this May 8, 2017

@bastien-roucaries

This comment has been minimized.

Copy link

commented Jul 13, 2017

Does this affect V6 ? I have not found the commit

@bastien-roucaries

This comment has been minimized.

Copy link

commented Jul 13, 2017

this is CVE-2017-11170 memory exhaustion in ReadTGAImage

@dlemstra

This comment has been minimized.

Copy link
Member

commented Jul 14, 2017

This is the IM6 commit: ea03f17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.