Bug 1017326 (CVE-2016-10070)

bugbot adjusting priority

*** Bug 1016596 has been marked as a duplicate of this bug. ***

From Matthias Gerstner 

An allocation for a number of (unsigned char) is made but in worst case a
number of (double) seems to be required for MAT images.

ImageMagick

[affected] SLE-12:Update in coders/mat.c:874
[affected] SLE-11:Update in coders/mat.c:819
[affected] openSUSE:13.2:Update in coders/mat.c:879

GraphicsMagick

[affected] SLE-11:Update in coders/mat.c:687
[affected] openSUSE:42.2:Update in coders/mat.c:556,994
[affected] openSUSE:42.1:Update in coders/mat.c:716
[affected] openSUSE:13.2:Update in coders/mat.c:710

(In reply to Johannes Segitz from comment #3)
> From Matthias Gerstner 
> 
> An allocation for a number of (unsigned char) is made but in worst case a
> number of (double) seems to be required for MAT images.
> 
> ImageMagick
> 
> [affected] SLE-12:Update in coders/mat.c:874
> [affected] SLE-11:Update in coders/mat.c:819
> [affected] openSUSE:13.2:Update in coders/mat.c:879
> 
> GraphicsMagick
> 
> [affected] SLE-11:Update in coders/mat.c:687
> [affected] openSUSE:42.2:Update in coders/mat.c:556,994
> [affected] openSUSE:42.1:Update in coders/mat.c:716
> [affected] openSUSE:13.2:Update in coders/mat.c:710

Hmm, there is a mess seems to me and I am not sure how to patch. There is in 11/ImageMagick and 12/ImageMagick:

BImgBuff = (unsigned char *) AcquireQuantumMemory((size_t) (ldblk),sizeof(unsigned char *));

So no sizeof(unsigned char). That looks like nonsense as the sizeof(unsigned char*) is used for calculating unsigned char array size. But if I am correct sizeof(double) is equal to sizeof(unsigned char *) at least for 64-bit architectures, so that should be okay.

In GraphicsMagick, there is though:

BImgBuff = MagickAllocateMemory(unsigned char *,(size_t) (ldblk));

But unsigned char * type is not used for calculating array size, the unsigned char array size, it is used just for type conversion.

Perhaps, to be on safe side, we could use something like:

For ImageMagick:

-    BImgBuff = (unsigned char *) AcquireQuantumMemory((size_t) (ldblk),sizeof(unsigned char *));
+    BImgBuff = (unsigned char *) AcquireQuantumMemory((size_t) (ldblk),sizeof(double));

For GraphicsMagick:

-    BImgBuff = MagickAllocateMemory(unsigned char *,(size_t) (ldblk));    
+    BImgBuff = MagickAllocateMemory(unsigned char *,(size_t) (MagickArraySize(ldblk,sizeof(double))));

What do you think?

Hi Petr,

(In reply to pgajdos@suse.com from comment #4)

> There is in 11/ImageMagick and 12/ImageMagick:
>
> [...]
>
> So no sizeof(unsigned char). That looks like nonsense as the sizeof(unsigned
> char*) is used for calculating unsigned char array size.

I think it *is* nonsense and it was changed by upstream years ago:

https://github.com/ImageMagick/ImageMagick/commit/ed0c03298dead0137b12cbdabd7d72875edf4b70

> But if I am correct sizeof(double) is equal to sizeof(unsigned char *) at
> least for 64-bit architectures, so that should be okay.

To avoid running software that runs only by coincidence I think we should
patch it as you suggested by changing to sizeof(double).

> In GraphicsMagick, there is though:
>
> BImgBuff = MagickAllocateMemory(unsigned char *,(size_t) (ldblk));
>
> But unsigned char * type is not used for calculating array size, the
> unsigned char array size, it is used just for type conversion.

Your proposed fix looks good to me.

For better understanding this: I've just had a hard time seeing the point of
this fix at all. Because ldblk is already set to a suitable value when
interpreting the image header, for example in:

SUSE:SLE-11:Update/GraphicsMagick/GraphicsMagick-1.2.5/coders/mat.c:650

The buffer BImgBuff is supposed to hold a single row of the image. This part
is fine. However after iterating over all image rows is finished, the row
buffer is reused for some additional image data in:

SUSE:SLE-11:Update/GraphicsMagick/GraphicsMagick-1.2.5/coders/mat.c:758

*That* is where the problem arises, because CellType can change here from
uint8 values to double values, for example, and then the original allocation
of BImgBuff is too small. Therefore BImgBuff needs to consider the worst case.

Would have been nice of the developer to drop at least one line of comment or
a commit message to explain this ...

Please don't overlook that this issue is regarding two CVEs. The other issue
seems a fixed memory leak from this commit:

https://github.com/ImageMagick/ImageMagick/commit/b173a352397877775c51c9a0e9d59eb6ce24c455

For some reason both CVEs refer to the same issue "out-of bounds read in
mat.c"

Created attachment 711770 [details]
reproducer image

QA reproducer:

The file from attachment 711770 [details] can be used to trigger the out-of-bounds read
issue. The file comes from the original bug report here:

https://bugs.launchpad.net/ubuntu/+source/imagemagick/+bug/1545366

I've been able to reproduce the issue using GraphicsMagick on SLES-11-SP4,
using the following command line:

  valgrind gm convert CVE-2016-10070.mat /dev/null

Valgrind will output multiple "Invalid read of size 4" errors.

Interestingly, in our versions ImageMagick, where currently sizeof(unsigned
char*) is used (i.e. the allocation is large enough as pgajdos pointer out) we
still get valgrind errors about "Conditional jump or move depends on
uninitialised value(s)" from the same location.

I think that is, because the BImgBuff memory region beyond the original image
size is never initialized with anything. So upstream might still have an issue
here.

(In reply to matthias.gerstner@suse.com from comment #7)

> I think that is, because the BImgBuff memory region beyond the original
> image size is never initialized with anything. So upstream might still have
> an issue here.

After checking that this follow-up issue is still in the current master branch
of ImageMagick I've opened an issue upstream:

https://github.com/ImageMagick/ImageMagick/issues/362

Maybe we can fix this completely, while we're at it.

(In reply to Matthias Gerstner from comment #5)
> For better understanding this: I've just had a hard time seeing the point of
> this fix at all. Because ldblk is already set to a suitable value when
> interpreting the image header, for example in:
> 
> SUSE:SLE-11:Update/GraphicsMagick/GraphicsMagick-1.2.5/coders/mat.c:650

Not sure the lines match for me, you mean the switch (CellType) block, correct? I had noticed it too but had not dare to try understand why the allocation is wrong.

> The buffer BImgBuff is supposed to hold a single row of the image. This part
> is fine. However after iterating over all image rows is finished, the row
> buffer is reused for some additional image data in:
> 
> SUSE:SLE-11:Update/GraphicsMagick/GraphicsMagick-1.2.5/coders/mat.c:758

Inside CalcMinMax(), correct? That explains it. Thanks for digging.
 
> *That* is where the problem arises, because CellType can change here from
> uint8 values to double values, for example, and then the original allocation
> of BImgBuff is too small. Therefore BImgBuff needs to consider the worst
> case.
> 
> Would have been nice of the developer to drop at least one line of comment or
> a commit message to explain this ...
> 
> Please don't overlook that this issue is regarding two CVEs. The other issue
> seems a fixed memory leak from this commit:

Yes, I am aware of that.

(In reply to Matthias Gerstner from comment #8)
> Maybe we can fix this completely, while we're at it.

The fix is already commited and indeed - valgrind errors went away, at least for 12/ImageMagick.

https://github.com/ImageMagick/ImageMagick/commit/f86268752ffc70e40b6e1afdebfc96dcc29452db

For 11/ImageMagick I get:

$ convert 128 bleble.jpg         
convert: no decode delegate for this image format `128'.
convert: missing an image filename `bleble.jpg'.
$

So the format is not recognized.

(In reply to Petr Gajdos from comment #11)
> For 11/ImageMagick I get:
> 
> $ convert 128 bleble.jpg         
> convert: no decode delegate for this image format `128'.
> convert: missing an image filename `bleble.jpg'.
> $
> 
> So the format is not recognized.

Aha, the file need to be renamed to 128.mat.

To sum up, affected: GraphicsMagick, ImageMagick

Packages submitted, I believe all fixed.

(In reply to pgajdos@suse.com from comment #10)

> The fix is already commited and indeed - valgrind errors went away, at least
> for 12/ImageMagick.

Yes, they've reacted to the issue I've created. They don't seem to think it a
big issue and just initialize the buffer with zeroes now.

As I don't fully understand the image format I can't tell them otherwise.
Still I wonder why they try to find the maximum value out of undefined data -
or now zero data. At least it will be defined behaviour now and our QA doesn't
see valgrind errors left over after the update.

This is an autogenerated message for OBS integration:
This bug (1017326) was mentioned in
https://build.opensuse.org/request/show/452917 42.2 / GraphicsMagick
https://build.opensuse.org/request/show/452918 42.1 / GraphicsMagick

openSUSE-SU-2017:0391-1: An update that fixes 12 vulnerabilities is now available.

Category: security (moderate)
Bug References: 1017310,1017312,1017313,1017314,1017318,1017321,1017322,1017324,1017325,1017326,1020443,1020448
CVE References: CVE-2016-10048,CVE-2016-10050,CVE-2016-10051,CVE-2016-10052,CVE-2016-10059,CVE-2016-10064,CVE-2016-10065,CVE-2016-10068,CVE-2016-10069,CVE-2016-10070,CVE-2016-10146,CVE-2017-5511
Sources used:
openSUSE Leap 42.1 (src):    GraphicsMagick-1.3.21-26.1

openSUSE-SU-2017:0399-1: An update that fixes 8 vulnerabilities is now available.

Category: security (moderate)
Bug References: 1017310,1017312,1017313,1017314,1017324,1017326,1020443,1020448
CVE References: CVE-2016-10048,CVE-2016-10050,CVE-2016-10051,CVE-2016-10052,CVE-2016-10068,CVE-2016-10070,CVE-2016-10146,CVE-2017-5511
Sources used:
openSUSE Leap 42.2 (src):    GraphicsMagick-1.3.25-9.1

SUSE-SU-2017:0518-1: An update that fixes 11 vulnerabilities is now available.

Category: security (moderate)
Bug References: 1017310,1017311,1017312,1017313,1017318,1017321,1017322,1017324,1017326,1020443,1020448
CVE References: CVE-2016-10048,CVE-2016-10049,CVE-2016-10050,CVE-2016-10051,CVE-2016-10059,CVE-2016-10064,CVE-2016-10065,CVE-2016-10068,CVE-2016-10070,CVE-2016-10146,CVE-2017-5511
Sources used:
SUSE Studio Onsite 1.3 (src):    GraphicsMagick-1.2.5-4.62.1
SUSE Linux Enterprise Software Development Kit 11-SP4 (src):    GraphicsMagick-1.2.5-4.62.1
SUSE Linux Enterprise Debuginfo 11-SP4 (src):    GraphicsMagick-1.2.5-4.62.1

SUSE-SU-2017:0529-1: An update that fixes 25 vulnerabilities is now available.

Category: security (moderate)
Bug References: 1017308,1017310,1017311,1017312,1017313,1017314,1017318,1017319,1017320,1017321,1017322,1017324,1017325,1017326,1017421,1020433,1020435,1020436,1020439,1020441,1020443,1020446,1020448
CVE References: CVE-2016-10046,CVE-2016-10048,CVE-2016-10049,CVE-2016-10050,CVE-2016-10051,CVE-2016-10052,CVE-2016-10059,CVE-2016-10060,CVE-2016-10061,CVE-2016-10062,CVE-2016-10063,CVE-2016-10064,CVE-2016-10065,CVE-2016-10068,CVE-2016-10069,CVE-2016-10070,CVE-2016-10071,CVE-2016-10144,CVE-2016-10145,CVE-2016-10146,CVE-2017-5506,CVE-2017-5507,CVE-2017-5508,CVE-2017-5510,CVE-2017-5511
Sources used:
SUSE Linux Enterprise Workstation Extension 12-SP2 (src):    ImageMagick-6.8.8.1-59.1
SUSE Linux Enterprise Workstation Extension 12-SP1 (src):    ImageMagick-6.8.8.1-59.1
SUSE Linux Enterprise Software Development Kit 12-SP2 (src):    ImageMagick-6.8.8.1-59.1
SUSE Linux Enterprise Software Development Kit 12-SP1 (src):    ImageMagick-6.8.8.1-59.1
SUSE Linux Enterprise Server for Raspberry Pi 12-SP2 (src):    ImageMagick-6.8.8.1-59.1
SUSE Linux Enterprise Server 12-SP2 (src):    ImageMagick-6.8.8.1-59.1
SUSE Linux Enterprise Server 12-SP1 (src):    ImageMagick-6.8.8.1-59.1
SUSE Linux Enterprise Desktop 12-SP2 (src):    ImageMagick-6.8.8.1-59.1
SUSE Linux Enterprise Desktop 12-SP1 (src):    ImageMagick-6.8.8.1-59.1

SUSE-SU-2017:0586-1: An update that fixes 21 vulnerabilities is now available.

Category: security (moderate)
Bug References: 1017308,1017310,1017311,1017312,1017313,1017314,1017318,1017319,1017320,1017321,1017322,1017324,1017326,1017421,1020433,1020435,1020436,1020439,1020441,1020443,1020448
CVE References: CVE-2016-10046,CVE-2016-10048,CVE-2016-10049,CVE-2016-10050,CVE-2016-10051,CVE-2016-10052,CVE-2016-10059,CVE-2016-10060,CVE-2016-10063,CVE-2016-10064,CVE-2016-10065,CVE-2016-10068,CVE-2016-10070,CVE-2016-10071,CVE-2016-10144,CVE-2016-10145,CVE-2016-10146,CVE-2017-5506,CVE-2017-5507,CVE-2017-5508,CVE-2017-5511
Sources used:
SUSE Linux Enterprise Software Development Kit 11-SP4 (src):    ImageMagick-6.4.3.6-7.65.1
SUSE Linux Enterprise Server 11-SP4 (src):    ImageMagick-6.4.3.6-7.65.1
SUSE Linux Enterprise Debuginfo 11-SP4 (src):    ImageMagick-6.4.3.6-7.65.1

openSUSE-SU-2017:0587-1: An update that fixes 25 vulnerabilities is now available.

Category: security (moderate)
Bug References: 1017308,1017310,1017311,1017312,1017313,1017314,1017318,1017319,1017320,1017321,1017322,1017324,1017325,1017326,1017421,1020433,1020435,1020436,1020439,1020441,1020443,1020446,1020448
CVE References: CVE-2016-10046,CVE-2016-10048,CVE-2016-10049,CVE-2016-10050,CVE-2016-10051,CVE-2016-10052,CVE-2016-10059,CVE-2016-10060,CVE-2016-10061,CVE-2016-10062,CVE-2016-10063,CVE-2016-10064,CVE-2016-10065,CVE-2016-10068,CVE-2016-10069,CVE-2016-10070,CVE-2016-10071,CVE-2016-10144,CVE-2016-10145,CVE-2016-10146,CVE-2017-5506,CVE-2017-5507,CVE-2017-5508,CVE-2017-5510,CVE-2017-5511
Sources used:
openSUSE Leap 42.2 (src):    ImageMagick-6.8.8.1-28.1
openSUSE Leap 42.1 (src):    ImageMagick-6.8.8.1-30.1

All codestreams released. openSUSE comes from SLE. Closing.

The uninitialized memory read was assigned CVE-2017-13143.

https://github.com/ImageMagick/ImageMagick/issues/362

As it is already fixed, I am not opening a new bug for it.