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.

Sign up for GitHub

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

Jump to bottom

use of uninitialized data in ImageMagick/coders/mat.c:407 #362

Closed
mgerstner opened this issue Jan 26, 2017 · 4 comments
Closed

use of uninitialized data in ImageMagick/coders/mat.c:407 #362

mgerstner opened this issue Jan 26, 2017 · 4 comments

Comments

@mgerstner
Copy link

In issue #131 an out of bounds read involving the mat image format has been fixed.

After the fixing commits the buffer bImgBuff is large enough to deal with the PoC file that lead to issue #131.

However, after the fix the coder still accesses uninitialized data which might pose a security issue or at least a bug. The first undefined access happens within coders/mat.c:1196 in a call to calcMinMax(). The back part of the buffer bImgBuff is now large enough but does seemingly not contain any sensible data.

I've tested this using the current ImageMagick master branch git revision 4a44cbd.

Undefined access is detected by valgrind using this command line:

libtool --mode=execute valgrind ./utilities/magick $POC_FILE /dev/null
@mgerstner
Copy link
Author

Complete valgrind output:

Memcheck, a memory error detector
Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
Command: /suse/mgerstner/local/upstream/ImageMagick/utilities/.libs/magick /suse/mgerstner/.downloads/CVE-2016-10070.mat /dev/null

Conditional jump or move depends on uninitialised value(s)
at 0x50844A0: CalcMinMax (mat.c:407)
by 0x5086797: ReadMATImage (mat.c:1196)
by 0x4EE22AA: ReadImage (constitute.c:555)
by 0x4EE327B: ReadImages (constitute.c:857)
by 0x55D5AF9: CLINoImageOperator (operation.c:4743)
by 0x55D6AB8: CLIOption (operation.c:5238)
by 0x556BD0C: ProcessCommandOptions (magick-cli.c:421)
by 0x556C753: MagickImageCommand (magick-cli.c:791)
by 0x5586ED3: MagickCommandGenesis (mogrify.c:183)
by 0x40137E: MagickMain (magick.c:149)
by 0x84716E4: (below main) (libc-start.c:289)

Conditional jump or move depends on uninitialised value(s)
at 0x50844AB: CalcMinMax (mat.c:409)
by 0x5086797: ReadMATImage (mat.c:1196)
by 0x4EE22AA: ReadImage (constitute.c:555)
by 0x4EE327B: ReadImages (constitute.c:857)
by 0x55D5AF9: CLINoImageOperator (operation.c:4743)
by 0x55D6AB8: CLIOption (operation.c:5238)
by 0x556BD0C: ProcessCommandOptions (magick-cli.c:421)
by 0x556C753: MagickImageCommand (magick-cli.c:791)
by 0x5586ED3: MagickCommandGenesis (mogrify.c:183)
by 0x40137E: MagickMain (magick.c:149)
by 0x84716E4: (below main) (libc-start.c:289)

Conditional jump or move depends on uninitialised value(s)
at 0x5084A53: InsertComplexFloatRow.isra.1 (mat.c:262)
by 0x508687E: ReadMATImage (mat.c:1211)
by 0x4EE22AA: ReadImage (constitute.c:555)
by 0x4EE327B: ReadImages (constitute.c:857)
by 0x55D5AF9: CLINoImageOperator (operation.c:4743)
by 0x55D6AB8: CLIOption (operation.c:5238)
by 0x556BD0C: ProcessCommandOptions (magick-cli.c:421)
by 0x556C753: MagickImageCommand (magick-cli.c:791)
by 0x5586ED3: MagickCommandGenesis (mogrify.c:183)
by 0x40137E: MagickMain (magick.c:149)
by 0x84716E4: (below main) (libc-start.c:289)

Conditional jump or move depends on uninitialised value(s)
at 0x5084A35: InsertComplexFloatRow.isra.1 (mat.c:280)
by 0x508687E: ReadMATImage (mat.c:1211)
by 0x4EE22AA: ReadImage (constitute.c:555)
by 0x4EE327B: ReadImages (constitute.c:857)
by 0x55D5AF9: CLINoImageOperator (operation.c:4743)
by 0x55D6AB8: CLIOption (operation.c:5238)
by 0x556BD0C: ProcessCommandOptions (magick-cli.c:421)
by 0x556C753: MagickImageCommand (magick-cli.c:791)
by 0x5586ED3: MagickCommandGenesis (mogrify.c:183)
by 0x40137E: MagickMain (magick.c:149)
by 0x84716E4: (below main) (libc-start.c:289)

magick: UnableToOpenConfigureFile `magic.xml' @ warning/configure.c/GetConfigureOptions/715.

HEAP SUMMARY:
in use at exit: 49,628 bytes in 243 blocks
total heap usage: 5,315 allocs, 5,072 frees, 705,489 bytes allocated

LEAK SUMMARY:
definitely lost: 0 bytes in 0 blocks
indirectly lost: 0 bytes in 0 blocks
possibly lost: 1,352 bytes in 18 blocks
still reachable: 48,276 bytes in 225 blocks
of which reachable via heuristic:
newarray : 1,536 bytes in 16 blocks
suppressed: 0 bytes in 0 blocks
Rerun with --leak-check=full to see details of leaked memory

For counts of detected and suppressed errors, rerun with: -v
Use --track-origins=yes to see where uninitialised values come from
ERROR SUMMARY: 5248 errors from 4 contexts (suppressed: 0 from 0)

@mikayla-grace
Copy link

Thanks for alerting us to uninitialized data. In some cases, in the interest of performance, we accept that some data may be uninitialized without any ill effects or security issues. However, in this case performance is not an issue so we initialized the buffer and valgrind no longer complains.

dlemstra pushed a commit that referenced this issue Jan 26, 2017
https://github.com/ImageMagick/ImageMagick/issues/362
f862687
@mgerstner
Copy link
Author

Yes this fixes the symptom, thank you.

I don't understand the MAT image format much, but I'm still wondering why the code is trying to calculate min/max values for undefined (or now after your fix: zero) data in the first place. That's why I was thinking there's some more serious bug behind this.

dlemstra pushed a commit that referenced this issue Jan 28, 2017
https://github.com/ImageMagick/ImageMagick/issues/362
51b0ae0
buildroot-auto-update pushed a commit to buildroot/buildroot that referenced this issue Jan 31, 2017
@jacmet
imagemagick: security bump to version 7.0.4-6
e5f505e 
Fixes an use of uninitialized data issue in MAT image format that may have
security impact:

ImageMagick/ImageMagick#362

[Peter: extend commit message, mention (potential) security impact]
Signed-off-by: Vicente Olivert Riera <Vincent.Riera@imgtec.com>
Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
woodsts pushed a commit to woodsts/buildroot that referenced this issue Feb 3, 2017
@jacmet
imagemagick: security bump to version 7.0.4-6
dc7fd80 
Fixes an use of uninitialized data issue in MAT image format that may have
security impact:

ImageMagick/ImageMagick#362

[Peter: extend commit message, mention (potential) security impact]
Signed-off-by: Vicente Olivert Riera <Vincent.Riera@imgtec.com>
Signed-off-by: Peter Korsgaard <peter@korsgaard.com>

(cherry picked from commit e5f505e)
jsonn pushed a commit to jsonn/pkgsrc that referenced this issue Feb 5, 2017
Updated ImageMagick to 7.0.4.7.
4b02741 
2017-02-04  7.0.4-7 Cristy  <quetzlzacatenango@image...>
  * Release ImageMagick version 7.0.4-7, GIT revision 19513:5783e57:20170204.

2017-01-28  7.0.4-7 Cristy  <quetzlzacatenango@image...>
  * Sanitize comments that include braces for the MIFF image format (reference
    ImageMagick/ImageMagick#368).

2017-01-27  7.0.4-7 Glenn Randers-Pehrson <glennrp@image...>
  * coders/png.c: Added support for a proposed new PNG chunk
    (zxIf, read-only) that is currently being discussed on the
    png-mng-misc at lists.sourceforge.net mailing list.  Enable
    exIf and zxIf with CPPFLAGS="-DexIf_SUPPORTED -DxzIf_SUPPORTED".
    If exIf is enabled, only the uncompressed exIF chunk will be
    written and the hex-encoded zTXt chunk containing the raw Exif
    profile won't be written.

2017-01-27  7.0.4-6 Cristy  <quetzlzacatenango@image...>
  * Release ImageMagick version 7.0.4-6, GIT revision 19442:4747de9:20170127.

2017-01-27  7.0.4-6 Cristy  <quetzlzacatenango@image...>
  * Uninitialized data in MAT image format (reference
    ImageMagick/ImageMagick#362).
  * Properly auto-fit caption (reference
    https://www.imagemagick.org/discourse-server/viewtopic.php?f=3&t=30887).
  * Correction to composite Over operator (reference
    https://www.imagemagick.org/discourse-server/viewtopic.php?f=3&t=31282).
  * Respect gravity option (reference
    https://www.imagemagick.org/discourse-server/viewtopic.php?f=3&t=31284).

2017-01-22  7.0.4-6 Glenn Randers-Pehrson <glennrp@image...>
  * Renamed read_vpag_chunk_callback() function to png_user_chunk_callback()
    in coders/png.c
  * Implemented a private PNG caNv (canvas) chunk for remembering the
    original dimensions and offsets when an image is cropped.  Previously
    we used the oFFs and vpAg chunks for this purpose, but this had potential
    conflicts with other applications that also use the oFFs chunk.
  * coders/png.c: Added support for a proposed new PNG chunk (exIf
    read-write, eXIf read-only) that is currently being discussed on the
    png-mng-misc at lists.sourceforge.net mailing list.

2017-01-22  7.0.4-6 Dirk Lemstra <dirk@lem.....org>
  * Replaced CoderSeekableStreamFlag with CoderDecoderSeekableStreamFlag and
    CoderEncoderSeekableStreamFlag.
clrpackages pushed a commit to clearlinux-pkgs/ImageMagick that referenced this issue Jul 17, 2017
@fenrus75
ImageMagick: Autospec creation for update from version 6.9.6 to versi…
2db69d0 
…on 6.9.7

2017-02-04  6.9.7-7 Cristy  <quetzlzacatenango@image...>
  * Release ImageMagick version 6.9.7-7, GIT revision 11338:cc980d1:20170204.

2017-01-28  6.9.7-7 Cristy  <quetzlzacatenango@image...>
  * Sanitize comments that include braces for the MIFF image format (reference
    ImageMagick/ImageMagick#368).

2017-01-27  6.9.7-6 Cristy  <quetzlzacatenango@image...>
  * Release ImageMagick version 6.9.7-6, GIT revision 11327:6b2f052:20170127.

2017-01-27  6.9.7-6 Cristy  <quetzlzacatenango@image...>
  * Uninitialized data in MAT image format (reference
    ImageMagick/ImageMagick#362).

2017-01-22  6.9.7-6 Glenn Randers-Pehrson <glennrp@image...>
@fgeek
Copy link

fgeek commented Aug 23, 2017

Please use CVE-2017-13143 for this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@fgeek @dlemstra @mikayla-grace @mgerstner