Prevent shell injection in load_djpeg

[mbrown1413]

Although JpegImageFile.load_djpeg isn't documented or used anywhere, it is public. This fix prevents shell injection in filenames that contain single quotes.

[coveralls]

Coverage increased (+0.01%) when pulling b9907ad on mbrown1413:master into 9f0b688 on python-pillow:master.

[wiredfool]

I see 6 places where os.system is used in the package (+ 1 in setup that's not vulnerable)

PIL/JpegImagePlugin.py:            os.system("djpeg '%s' >'%s'" % (self.filename, path))
PIL/JpegImagePlugin.py:    os.system("cjpeg %s >%s" % (file, filename))
PIL/ImageShow.py:        os.system(self.get_command(file, **options))
PIL/ImageShow.py:            os.system(command)
PIL/GifImagePlugin.py:        os.system("ppmtogif %s >%s" % (file, filename))
PIL/GifImagePlugin.py:        os.system("ppmquant 256 %s | ppmtogif >%s" % (file, filename))

From a quick check of those, ImageShow has already had this done to it, and the other ones appear to have the same issue.

We should clean them all up, either this way or by changing them to use the subprocess interface that doesn't use a shell for redirection. If we do continue quoting, we should probably pull the quote function import logic into _util.py.

[hugovk]

Each of these six should have a test case, and the vulnerable ones should fail before the fixes. The tests should be in their own test_shell_injection.py (or something) file.

[mbrown1413]

We really should change all os.system calls to use subprocess. I can work on test cases for this.

I have a few questions before I write tests:

• If I call load_djpeg before actually using the image data, does it prevent the normal jpeg loader from being called? Is this how it's intended to be used?
• GifImagePlugin._save_netpbm mentions making code changes to register it (see the bottom of PIL/GifImagePlugin.py). Should I insert an Image.register_save call at the beginning of the test to accomplish this?

Most of the functions where os.system is used are undocumented (PIL/JpegImagePlugin.py and PIL/GifImagePlugin.py). Does anybody with more knowledge about them want to document them?

[wiredfool]

The functions don't need to be registered to be called. That's part of the danger of them.

They're historical baggage. I've wanted to simply get rid of them for a while -- the chances that someone has djpeg but not libjpeg is pretty minimal. The use case for the gifs is a little better, but still not great.

[aclark4life]

Should I merge this, or wait? I would rather err on the side of security over backwards compatibility, for whatever it's worth.

[wiredfool]

Wait. It's been this way for 10 years, another day isn't going to hurt. We'll get a solution prior to 2.5.

[aclark4life]

@wiredfool Well if you are targeting 2.5 for this then you literally only have another few days 😄

[wiredfool]

Probably need to add libjpeg-turbo-progs to the apt-get list in .travis.yml, and skip the tests if they're not in the path. (though, we do need to check windows, and that's one of those likely places where they're not going to be installed)

[mbrown1413]

Any idea how to perform a cross platform check to see if an executable is in the path? Perhaps running, for example, "djpeg -h" before the test and skipping if an OSError is thrown (or catching the OSError thrown in load_djpeg)? It seems a bit of a hack, but a proper "which" function in Python would be a bit overkill (https://code.google.com/p/which/).

[coveralls]

Coverage increased (+0.36%) when pulling b140fa0 on mbrown1413:master into 9f0b688 on python-pillow:master.

[wiredfool]

We're doing something like that in EpsImagePlugin.py: https://github.com/python-pillow/Pillow/blob/master/PIL/EpsImagePlugin.py#L39 , where we're splitting out the windows check from the unix check.

In that case, we're checking the version of ghostscript, but we could execute a shell just for the which command as well.

[aclark4life]

I'm using sh to do some command line tasks in plock, but at this point in PIL's history I don't want to introduce install_requires so shutil is probably a better way to go

[wiredfool]

We're already implicitly using a shell in os.system. It's difference between os.system('which djpeg') and os.system('djpeg -v').

[aclark4life]

Right, os.system('djpeg -v') is enough to tell you if djpeg is installed and available from the PATH. No need to run which

[mbrown1413]

Rebased my commits. Let me know if there's anything else I need to do before a merge.

[coveralls]

Coverage increased (+0.34%) when pulling 8b365f5 on mbrown1413:master into b6d3983 on python-pillow:master.

[wiredfool]

See #748.