I don't see a need for any of these changes. There is a significant cost to setting the seekable_stream option because it prohibits reading directly from a file descriptor and requires copying all the input to a temporary file (or would requiring buffering all input to allocated memory). The AllocateImageColormap() function will not allocate more than 64k colormap entries in any build configuration (a 524k byte allocation in total) so this is not much of a DOS opportunity.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Indeed, I had not noticed there is a check against MaxColormapSize in AllocateImageColormap(). Thanks for your explanation, feel free to close.
Individual checks to verify that the requested number of colors do not
exceed the amount allowed by the file format are definitely still
useful (e.g. 256 entries is still a lot less memory than 64k entries)
since it is good to reject corrupt files. Some file formats are
sloppy and provide a larger storage unit than is required and so a
corrupt file is able to specify a large value.
I checked bmp.c and dib.c and see that they are already adding additional checks/constraints on the number of colors prior to attempting to allocate the colormap. I do not see the same in pict.c. It seems unlikely that PICT actually supports more than 256 colors.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Due to existing checks/constaints, the severity of this issue is within reasonable bounds in GraphicsMagick and not a denial of service opportunity. There is no need for any code changes at this time.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Few links relating to CVE-2018-16645
http://www.cvedetails.com/cve/CVE-2018-16645/
https://github.com/ImageMagick/ImageMagick/issues/1268
https://github.com/ImageMagick/ImageMagick/commit/ecb31dbad39ccdc65868d5d2a37f0f0521250832
There seem to be similar issue in pict.c.
I don't see a need for any of these changes. There is a significant cost to setting the seekable_stream option because it prohibits reading directly from a file descriptor and requires copying all the input to a temporary file (or would requiring buffering all input to allocated memory). The AllocateImageColormap() function will not allocate more than 64k colormap entries in any build configuration (a 524k byte allocation in total) so this is not much of a DOS opportunity.
Indeed, I had not noticed there is a check against MaxColormapSize in AllocateImageColormap(). Thanks for your explanation, feel free to close.
On Mon, 10 Sep 2018, Petr Gajdos wrote:
Individual checks to verify that the requested number of colors do not
exceed the amount allowed by the file format are definitely still
useful (e.g. 256 entries is still a lot less memory than 64k entries)
since it is good to reject corrupt files. Some file formats are
sloppy and provide a larger storage unit than is required and so a
corrupt file is able to specify a large value.
Bob
Bob Friesenhahn
bfriesen@simple.dallas.tx.us, http://www.simplesystems.org/users/bfriesen/
GraphicsMagick Maintainer, http://www.GraphicsMagick.org/
I checked bmp.c and dib.c and see that they are already adding additional checks/constraints on the number of colors prior to attempting to allocate the colormap. I do not see the same in pict.c. It seems unlikely that PICT actually supports more than 256 colors.
Due to existing checks/constaints, the severity of this issue is within reasonable bounds in GraphicsMagick and not a denial of service opportunity. There is no need for any code changes at this time.