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

imagemagick identify&convert heap-buffer-overflow #538

Closed
zyy89 opened this issue Jul 4, 2017 · 16 comments

Comments

Projects
None yet
6 participants
@zyy89
Copy link

commented Jul 4, 2017

ImageMagick-7.0.6-0

  ~$identify $FILE or convert $FILE

build instructions:

  ~$ sudo apt-get install imagemagick libmagick++-dev
  ~$ CC="gcc" CFLAGS="-fsanitize=address" ./configure 
  ~$ make
  ~$ make install

when identify or convert MNG file, imagemagick will cause a heap buffer overflow

=================================================================
==6483==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000008c5a at pc 0x7fee316dfb7d bp 0x7ffeba5682b0 sp 0x7ffeba5682a0
READ of size 1 at 0x602000008c5a thread T0
#0 0x7fee316dfb7c in mng_get_long coders/png.c:1636
#1 0x7fee316f08ad in ReadOneMNGImage coders/png.c:5741
#2 0x7fee316fae29 in ReadMNGImage coders/png.c:7497
#3 0x7fee31272c4f in ReadImage MagickCore/constitute.c:497
#4 0x7fee314bee06 in ReadStream MagickCore/stream.c:1045
#5 0x7fee3127217c in PingImage MagickCore/constitute.c:226
#6 0x7fee312725a9 in PingImages MagickCore/constitute.c:327
#7 0x7fee30c32a2a in IdentifyImageCommand MagickWand/identify.c:319
#8 0x7fee30c89c7a in MagickCommandGenesis MagickWand/mogrify.c:183
#9 0x40169a in MagickMain utilities/magick.c:149
#10 0x4017be in main utilities/magick.c:180
#11 0x7fee3050282f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
#12 0x4012c8 in _start (/home/sf/ImageMagick-7.0.6-0/build-gcc/bin/magick+0x4012c8)

The vulnerability is caused when identify MNG image, which happens in function mng_get_long (coders/png.c:1636) which is called by line 5741 at coders/png.c.
Here is the critical code of mng_get_long and its call code:

static long mng_get_long(unsigned char *p)
{
  return((long) ((p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3]));
}
5728	                        if (change_delay == 2)
5729	                          default_frame_delay=frame_delay;
5730	
5731	                        p+=4;
5732	
5733	                        if (logging != MagickFalse)
5734	                          (void) LogMagickEvent(CoderEvent,GetMagickModule(),
(gdb) 
5735	                            "    Framing_delay=%.20g",(double) frame_delay);
5736	                      }
5737	
5738	                    if (change_timeout)
5739	                      {
5740	                        frame_timeout=1UL*image->ticks_per_second*
5741	                          mng_get_long(p);
5742	
5743	                        if (mng_info->ticks_per_second != 0)

It is caused by heap buffer overflow, which is caused by a read operation without overflow check.
The p buffer is pointer to chunk, its buffer data and length are read from input file,

        p=NULL;
        chunk=(unsigned char *) NULL;

        if (length != 0)
          {
            chunk=(unsigned char *) AcquireQuantumMemory(length,
             sizeof(*chunk));

            if (chunk == (unsigned char *) NULL)
              ThrowReaderException(ResourceLimitError,
                "MemoryAllocationFailed");

            for (i=0; i < (ssize_t) length; i++)
              chunk[i]=(unsigned char) ReadBlobByte(image);

            p=chunk;
          }

When setting proper length and repeat value, it is possible to disclosing some critical data, such as heap chunk data and even other applications’ private data.

Testcase: https://github.com/zyy89/pocs/blob/master/imagemagick-heap-buffer-overflow-1

@mikayla-grace

This comment has been minimized.

Copy link

commented Jul 6, 2017

Thanks for the problem report. We can reproduce it and will have a patch to fix it in GIT master branch @ https://github.com/ImageMagick/ImageMagick later today. The patch will be available in the beta releases of ImageMagick @ http://www.imagemagick.org/download/beta/ by sometime tomorrow.

@dlemstra dlemstra closed this in 280e592 Jul 6, 2017

dlemstra pushed a commit that referenced this issue Jul 6, 2017

@dlemstra dlemstra added the bug label Jul 6, 2017

@bastien-roucaries

This comment has been minimized.

Copy link

commented Jul 8, 2017

Does it affect v6 ?

@mikayla-grace

This comment has been minimized.

Copy link

commented Jul 8, 2017

Yes.

@bastien-roucaries

This comment has been minimized.

Copy link

commented Jul 9, 2017

Did you jhave a commit for v6 ?

@bastien-roucaries

This comment has been minimized.

Copy link

commented Jul 15, 2017

Ping ?

@dlemstra

This comment has been minimized.

Copy link
Member

commented Jul 15, 2017

280e592 is 7 and 7a9a910 is 6.

@bastien-roucaries

This comment has been minimized.

Copy link

commented Jul 15, 2017

Will want to be ping for correct fix (glenn one). Thank you nevertheless

@glennrp

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2017

Final fix for IM7 is commit 6a4f9c1

@bastien-roucaries

This comment has been minimized.

Copy link

commented Jul 19, 2017

And for v6 ? Does we need to apply previous commit ?

@glennrp

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2017

I believe v6 was not vulnerable to this particular problem. It was fixed in V6 around version 6.9.5-3.

@bastien-roucaries

This comment has been minimized.

Copy link

commented Jul 21, 2017

glenn instead of if (change_clipping && ((p-chunk) < (ssize_t) (length-16))) v6 has if (change_clipping && ((p-chunk) < (ssize_t) (length-17)))

is it normal ?

@glennrp

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

It should be 16.

@glennrp

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

I fixed the 17 -> 16 thing. It wasn't a security problem but could have caused certain FRAM chunks to be rejected incorrectly. See commit 1fdc09d

@carnil

This comment has been minimized.

Copy link

commented Jul 29, 2017

@glennrp: could you help identify the commit in v6, around 6.9.5-3 which fixes this issue?

@glennrp

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2017

IM6 commits 2443022 and 1fdc09d

@carnil

This comment has been minimized.

Copy link

commented Jul 29, 2017

@glennrp: thank you!

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.