Bug 1017326 - (CVE-2016-10070) VUL-0: CVE-2016-10070, CVE-2016-10071,CVE-2017-13143: ImageMagick: Missing out of bound checks for mat files
(CVE-2016-10070)
VUL-0: CVE-2016-10070, CVE-2016-10071,CVE-2017-13143: ImageMagick: Missing ou...
Status: RESOLVED FIXED
: 1016596 (view as bug list)
Classification: Novell Products
Product: SUSE Security Incidents
Classification: Novell Products
Component: Incidents
unspecified
Other Other
: P3 - Medium : Minor
: ---
Assigned To: Security Team bot
Security Team bot
CVSSv2:SUSE:CVE-2016-10070:4.3:(AV:N/...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-27 09:23 UTC by Johannes Segitz
Modified: 2017-08-24 07:51 UTC (History)
3 users (show)

See Also:
Found By: ---
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---


Attachments
reproducer image (9.10 KB, application/octet-stream)
2017-01-26 13:43 UTC, Matthias Gerstner
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Johannes Segitz 2016-12-27 09:23:48 UTC
Debian bug: https://bugs.debian.org/845246
Reference URL: https://security-tracker.debian.org/845246
Upstream commit: 
  - https://github.com/ImageMagick/ImageMagick/commit/b173a352397877775c51c9a0e9d59eb6ce24c455
  - https://github.com/ImageMagick/ImageMagick/commit/f3b483e8b054c50149912523b4773687e18afe25
Upstream issue: https://github.com/ImageMagick/ImageMagick/issues/131
Upstream version fixed: 6.9.4-0

Use CVE-2016-10070 for b173a352397877775c51c9a0e9d59eb6ce24c455.
Use CVE-2016-10071 for f3b483e8b054c50149912523b4773687e18afe25.
Comment 1 Swamp Workflow Management 2016-12-27 23:03:41 UTC
bugbot adjusting priority
Comment 2 Johannes Segitz 2016-12-28 13:21:13 UTC
*** Bug 1016596 has been marked as a duplicate of this bug. ***
Comment 3 Johannes Segitz 2016-12-28 13:21:49 UTC
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
Comment 4 Petr Gajdos 2017-01-26 11:23:49 UTC
(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?
Comment 5 Matthias Gerstner 2017-01-26 13:28:36 UTC
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"
Comment 6 Matthias Gerstner 2017-01-26 13:43:42 UTC
Created attachment 711770 [details]
reproducer image
Comment 7 Matthias Gerstner 2017-01-26 13:50:11 UTC
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.
Comment 8 Matthias Gerstner 2017-01-26 15:36:49 UTC
(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.
Comment 9 Petr Gajdos 2017-01-27 09:36:36 UTC
(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.
Comment 10 Petr Gajdos 2017-01-27 09:48:32 UTC
(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
Comment 11 Petr Gajdos 2017-01-27 10:06:41 UTC
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.
Comment 12 Petr Gajdos 2017-01-27 10:17:40 UTC
(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.
Comment 13 Petr Gajdos 2017-01-27 10:48:10 UTC
To sum up, affected: GraphicsMagick, ImageMagick
Comment 14 Petr Gajdos 2017-01-27 11:04:56 UTC
Packages submitted, I believe all fixed.
Comment 15 Matthias Gerstner 2017-01-27 11:30:59 UTC
(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.
Comment 17 Bernhard Wiedemann 2017-01-27 13:04:42 UTC
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
Comment 18 Swamp Workflow Management 2017-02-06 14:09:35 UTC
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
Comment 19 Swamp Workflow Management 2017-02-06 14:14:18 UTC
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
Comment 20 Swamp Workflow Management 2017-02-20 14:12:26 UTC
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
Comment 21 Swamp Workflow Management 2017-02-21 14:10:15 UTC
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
Comment 22 Swamp Workflow Management 2017-03-01 20:11:22 UTC
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
Comment 23 Swamp Workflow Management 2017-03-02 14:10:25 UTC
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
Comment 24 Matthias Gerstner 2017-03-06 09:42:59 UTC
All codestreams released. openSUSE comes from SLE. Closing.
Comment 25 Marcus Meissner 2017-08-24 07:51:28 UTC
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.