removing code injection from images

[fmw42]

User bonzo and I have been trying to write some interesting jquery applications that work on images uploaded to a server. So we are concerned about code injection in an image.

This is a link to an image containing php code injection in the comment section http://nullcandy.com/demo/kittens/kitten.jpg

identify -verbose kitten.jpg shows the php code there.

I can remove it by using -strip or by setting the comment to an empty string, for example as

Code: Select all

convert kitten.jpg -set comment "" kitten_new.png

Using -strip has the unfortunate result of removing any profiles as well. Whereas using -set comment, does not address the possibility that code has been injected into EXIF and/or IPTC data.

I was wondering if we could get some way to remove all meta data, leaving the profiles? Perhaps this exists, but I am unaware of how to do that. Or a way to remove the EXIF and IPTC data.

One suggestion would be either arguments for -strip that allow one to choose to strip either 1)metadata or 2) metadata and profiles. I know we have +profile "*" to remove profiles. Or perhaps a -define for -strip. Or a new function to just strip meta data. Or a new function to just strip EXIF and IPTC data.

It would be even better if the stripping was done before reading the image as in the new features to strip profiles (-define profile:skip=icc)

I am open to alternates means and to suggestions.

[glennrp]

You can use

Code: Select all

-define profile:skip="*"

to skip all profiles while reading PNG or JPEG images. "-define profile:skip=iptc" will also work while
reading JPEGs (but not PNGs) to skip IPTC profiles. I believe "-define profile:skip=8bim" will skip 8BIM
profiles from both JPEG and PNG files. The "profile:skip" is fairly new and probably still needs some
work, for example, to extend it so the PNG decoder will ignore IPTC profiles.

[fmw42]

Mac OSX

Version: ImageMagick 6.8.9-6 Q16 x86_64 2014-07-28 http://www.imagemagick.org
Copyright: Copyright (C) 1999-2014 ImageMagick Studio LLC
Features: DPC Modules
Delegates: bzlib cairo fftw fontconfig freetype gslib jbig jng jp2 jpeg lcms lqr ltdl lzma openexr png ps rsvg tiff webp x xml zlib


Neither of these removed the exif profile nor the exif metadata. It is not the profiles I want to remove. I want to remove meta data leaving the color profiles such as icc and icm.

Code: Select all

convert -define profile:skip=exif SH100147.JPG tmp.jpg

convert SH100147.JPG -define profile:skip=exif tmp2.jpg

The following behaved the same as -strip, removing all profiles and meta data.

Code: Select all

convert -define profile:skip="*" SH100147.JPG tmp3.jpg

Is -define profile:skip=exif and -define profile:skip=iptc supported so as to remove the profile and meta data?

If these worked to remove their respective profile and meta data, then one could remove comments (-set comment) and all exif data (-define profile:skip=exif) and all iptc data ( -define profile:skip=iptc) and still leave other icc and icm profiles. But that is 3 items to do. It would be better if there was a -define meta:skip="*" for all meta data and profiles, except for icc and icm. Or something like -define profile:noskip:"icc,icm"

[glennrp]

Mac OSX

Neither of these removed the exif profile nor the exif metadata. It is not the profiles I want to remove. I want to remove meta data leaving the color profiles such as icc and icm.

Code: Select all

convert -define profile:skip=exif SH100147.JPG tmp.jpg

What happens if you do

Code: Select all

convert -define profile:skip=app SH100147.JPG tmp4.jpg

Looking at coders/jpeg.c it seems that that might skip the exif profile.

[fmw42]

convert -define profile:skip=app SH100147.JPG tmp4.jpg

Seems to remove all profiles and meta data same as -strip or -define profile:skip="*"

[dlemstra]

Shouldn't you fix this where you present the image data? They might find another spot that you didn't think of now. If you specify -define skip:profile=APP,IPTC it should only keep the ICC profile. You should combine this with -set comment to also clear the comment.

Code: Select all

convert -define profile:skip=app,iptc SH100147.JPG -set comment "" tmp.jpg

[fmw42]

Shouldn't you fix this where you present the image data? They might find another spot that you didn't think of now.

I am not sure what you mean by "where you present the image data". Other people are uploading images to the server and we want to prevent them from injecting code in the images. So we have to try to remove this code between copying the uploaded data to a temp image file for further processing.

If you specify -define skip:profile=APP,IPTC it should only keep the ICC profile. You should combine this with -set comment to also clear the comment.

Yes we would combine them, but we need some way to remove the IPTC and EXIF profiles and meta data leaving the icc and icm profiles.

This command removes all profiles and meta data. It does not leave the icc or icm profile behind as we would need.

Code: Select all

convert -define profile:skip=app,EXIF SH100147.JPG tmp4.jpg

Thus no different from -strip or -define profile:skip="*"


In fact, this

Code: Select all

convert -define profile:skip=EXIF tmpA.jpg tmpB.jpg

did not even remove the EXIF profile nor the EXIF metadata.

So it would look like this is a bug. If it worked, that would solve my problem and I could do as you suggest by combining the -define for EXIF and IPTC and -set comment "".

I will report to the bugs forum.

[Bonzo]

I have just remembered I read a post that I can not find ( or completely remember ) where one way to remove the php when using GD was to read the image rather than load it then write it again. Does that make sense?

A png injection example: https://www.idontplaydarts.com/2012/06/ ... at-chunks/ and a quote "Re encoding the image will not stop someone from uploading a shell. The only sure way to prevent it is to re-encode and scan the image for the presence of php tags."

[fmw42]

Glenn would need to comment on how to avoid or clear the PNG IDAT.

[glennrp]

Glenn would need to comment on how to avoid or clear the PNG IDAT.

The IDAT chunk contains the image pixel data. If you clear the IDAT you lose your pixels. So the
problem is not to "clear the IDAT" but to recognize that the IDAT contains code instead of pixels,
then clear it if it does.

[Bonzo]

but to recognize that the IDAT contains code instead of pixels, then clear it if it does.

This is the problem in both the jpg and png case - how do you do it?

I found some example GD code where you read the image file into a string and rebuild the image from that which removes the php code but I have not tried it with the png problem. I do not know how to do that with Imagemagick but Imagick may be able to do it.

I thought I could read the image and check for the occurance of <?php and <php in the file but reading the file either removes the characters or changes them in some way as I could not see them in the string.

[Bonzo]

I am a bit disappointed that the IM developers do not seem to be interested in this problem as it looks like a good option to have in IM.

A lot of people use images on their website and it would be a plus for using Imagemagick if the code injection could be removed in processing.

[Danack]

Hi Fmw42 + Bonzo,

You are taking a fundamentally wrong approach to security. You have identified a weakness and are trying to patch it by 'blacklisting' behaviour that shouldn't be allowed; that will always eventually fail as it requires you to blacklist every possible way the weakness can be attacked. Instead you should be 'whitelisting' the behaviour that is safe and should be allowed.


Specifically in this instance, the weakness is running any file that a user has control over. Doing that is always wrong, and that's the thing that should be prevented. This can be done either through your webserver e.g. in nginx, the following location block does not attempt to run any user uploaded file through PHP at all:

Code: Select all

    location ~* ^[^\?\&]+\.(html|jpg|jpeg|gif|png|ico|css|zip|tgz|gz|rar|bz2|doc|xls|pdf|ppt|txt|tar|mid|midi|wav|bmp|rtf|js|svg|woff|ttf)$ {
        try_files $uri /index.php?userFile=$1;
        expires 7d;
        add_header Pragma public;
        add_header Cache-Control "public, must-revalidate, proxy-revalidate";
    }

If you do want to echo the file through PHP(, e.g. for checking permissions) then again you should do it in a safe way i.e. with readfile which does not attempt to process the file, instead of something dumb like 'require'

Code: Select all

<?php
readfile($userUploadedFilename); //This is always safe against code injection.

Code: Select all

<?php
require($userUploadedFilename); //This is never safe against code injection.

Bongo wrote:
> I am a bit disappointed that the IM developers do not seem to be interested in this problem as it looks like a good option to have in IM.

Yeah.....top-tip, don't use passive-aggressive language like this. It really doesn't win friends or persuade people to your way of thinking - it's just pisses people off.

And incidentally, this is not a feature that should be in ImageMagick. The fact that you linked to some valid images which also contain code injection shows that it is fundamentally impossible for ImageMagick to be a prophylactic defense against code injection attacks.

cheers
Dan