From cd7b45994b1b1a016a29401d7ab3faf9b7c7d054 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Wed, 25 Jun 2014 21:34:16 -0400 Subject: [PATCH 1/6] Prevent shell injection in load_djpeg --- PIL/JpegImagePlugin.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/PIL/JpegImagePlugin.py b/PIL/JpegImagePlugin.py index 1d300fc04..8a06cf5a5 100644 --- a/PIL/JpegImagePlugin.py +++ b/PIL/JpegImagePlugin.py @@ -34,12 +34,18 @@ __version__ = "0.6" +import sys import array import struct from PIL import Image, ImageFile, _binary from PIL.JpegPresets import presets from PIL._util import isStringType +if sys.version_info >= (3, 3): + from shlex import quote +else: + from pipes import quote + i8 = _binary.i8 o8 = _binary.o8 i16 = _binary.i16be @@ -359,7 +365,7 @@ class JpegImageFile(ImageFile.ImageFile): f, path = tempfile.mkstemp() os.close(f) if os.path.exists(self.filename): - os.system("djpeg '%s' >'%s'" % (self.filename, path)) + os.system("djpeg %s > '%s'" % (quote(self.filename), path)) else: raise ValueError("Invalid Filename") From d283f77884754660520005c086aa00cd46918cf9 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Thu, 26 Jun 2014 23:37:49 -0400 Subject: [PATCH 2/6] Tests for _save_netpbm, _save_cjpeg and load_djpeg --- Tests/test_file_gif.py | 15 +++++++++++ Tests/test_file_jpeg.py | 18 +++++++++++-- Tests/test_shell_injection.py | 51 +++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 Tests/test_shell_injection.py diff --git a/Tests/test_file_gif.py b/Tests/test_file_gif.py index 01ac1d95c..719847a96 100644 --- a/Tests/test_file_gif.py +++ b/Tests/test_file_gif.py @@ -1,6 +1,7 @@ from helper import unittest, PillowTestCase, tearDownModule, lena from PIL import Image +from PIL import GifImagePlugin codecs = dir(Image.core) @@ -89,6 +90,20 @@ class TestFileGif(PillowTestCase): reloaded = roundtrip(im)[1].convert('RGB') self.assert_image_equal(im, reloaded) + def test_save_netpbm_bmp_mode(self): + img = Image.open(file).convert("RGB") + + tempfile = self.tempfile("temp.gif") + GifImagePlugin._save_netpbm(img, 0, tempfile) + self.assert_image_similar(img, Image.open(tempfile).convert("RGB"), 0) + + def test_save_netpbm_l_mode(self): + img = Image.open(file).convert("L") + + tempfile = self.tempfile("temp.gif") + GifImagePlugin._save_netpbm(img, 0, tempfile) + self.assert_image_similar(img, Image.open(tempfile).convert("L"), 0) + if __name__ == '__main__': unittest.main() diff --git a/Tests/test_file_jpeg.py b/Tests/test_file_jpeg.py index dae1d0019..5b2ba45e7 100644 --- a/Tests/test_file_jpeg.py +++ b/Tests/test_file_jpeg.py @@ -5,6 +5,7 @@ from io import BytesIO from PIL import Image from PIL import ImageFile +from PIL import JpegImagePlugin codecs = dir(Image.core) @@ -273,8 +274,21 @@ class TestFileJpeg(PillowTestCase): qtables={0:standard_l_qtable, 1:standard_chrominance_qtable}), 30) - - + + def test_load_djpeg(self): + img = Image.open(test_file) + img.load_djpeg() + self.assert_image_similar(img, Image.open(test_file), 0) + + def test_save_cjpeg(self): + img = Image.open(test_file) + + tempfile = self.tempfile("temp.jpg") + JpegImagePlugin._save_cjpeg(img, 0, tempfile) + # Default save quality is 75%, so a tiny bit of difference is alright + self.assert_image_similar(img, Image.open(tempfile), 1) + + if __name__ == '__main__': unittest.main() diff --git a/Tests/test_shell_injection.py b/Tests/test_shell_injection.py new file mode 100644 index 000000000..d20f9b1a6 --- /dev/null +++ b/Tests/test_shell_injection.py @@ -0,0 +1,51 @@ +from helper import unittest, PillowTestCase, tearDownModule + +import shutil + +from PIL import Image, JpegImagePlugin, GifImagePlugin + +test_jpg = "Tests/images/lena.jpg" +test_gif = "Tests/images/lena.gif" + +test_filenames = ( + "temp_';", + "temp_\";", + "temp_'\"|", + "temp_'\"||", + "temp_'\"&&", +) + +class TestShellInjection(PillowTestCase): + + def assert_save_filename_check(self, src_img, save_func): + for filename in test_filenames: + dest_file = self.tempfile(filename) + save_func(src_img, 0, dest_file) + # If file can't be opened, shell injection probably occurred + Image.open(dest_file).load() + + def test_load_djpeg_filename(self): + for filename in test_filenames: + src_file = self.tempfile(filename) + shutil.copy(test_jpg, src_file) + + im = Image.open(src_file) + im.load_djpeg() + + def test_save_cjpeg_filename(self): + im = Image.open(test_jpg) + self.assert_save_filename_check(im, JpegImagePlugin._save_cjpeg) + + def test_save_netpbm_filename_bmp_mode(self): + im = Image.open(test_gif).convert("RGB") + self.assert_save_filename_check(im, GifImagePlugin._save_netpbm) + + def test_save_netpbm_filename_l_mode(self): + im = Image.open(test_gif).convert("L") + self.assert_save_filename_check(im, GifImagePlugin._save_netpbm) + + +if __name__ == '__main__': + unittest.main() + +# End of file From 34317edd8ace0f09d510b1b551034eab0a368c1b Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Fri, 27 Jun 2014 00:28:44 -0400 Subject: [PATCH 3/6] Change most uses of os.system to use subprocess The only places left that use os.system are in ImageShow and setup.py --- PIL/GifImagePlugin.py | 15 +++++++++++---- PIL/JpegImagePlugin.py | 10 +++++++--- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/PIL/GifImagePlugin.py b/PIL/GifImagePlugin.py index c6d449425..7f29ffe12 100644 --- a/PIL/GifImagePlugin.py +++ b/PIL/GifImagePlugin.py @@ -333,13 +333,20 @@ def _save_netpbm(im, fp, filename): # below for information on how to enable this. import os + from subprocess import Popen, check_call, PIPE file = im._dump() if im.mode != "RGB": - os.system("ppmtogif %s >%s" % (file, filename)) + with open(filename, 'wb') as f: + check_call(["ppmtogif", file], stdout=f) else: - os.system("ppmquant 256 %s | ppmtogif >%s" % (file, filename)) - try: os.unlink(file) - except: pass + with open(filename, 'wb') as f: + #TODO: What happens if ppmquant fails? + ppmquant_proc = Popen(["ppmquant", "256", file], stdout=PIPE) + check_call(["ppmtogif"], stdin=ppmquant_proc.stdout, stdout=f) + try: + os.unlink(file) + except: + pass # -------------------------------------------------------------------- diff --git a/PIL/JpegImagePlugin.py b/PIL/JpegImagePlugin.py index 8a06cf5a5..c159d61c3 100644 --- a/PIL/JpegImagePlugin.py +++ b/PIL/JpegImagePlugin.py @@ -360,12 +360,14 @@ class JpegImageFile(ImageFile.ImageFile): # ALTERNATIVE: handle JPEGs via the IJG command line utilities + import subprocess import tempfile import os f, path = tempfile.mkstemp() os.close(f) if os.path.exists(self.filename): - os.system("djpeg %s > '%s'" % (quote(self.filename), path)) + with open(path, 'wb') as f: + subprocess.check_call(["djpeg", self.filename], stdout=f) else: raise ValueError("Invalid Filename") @@ -608,8 +610,10 @@ def _save(im, fp, filename): def _save_cjpeg(im, fp, filename): # ALTERNATIVE: handle JPEGs via the IJG command line utilities. import os - file = im._dump() - os.system("cjpeg %s >%s" % (file, filename)) + import subprocess + tempfile = im._dump() + with open(filename, 'wb') as f: + subprocess.check_call(["cjpeg", tempfile], stdout=f) try: os.unlink(file) except: From b5ab5a49e79676e37e3323115e10fa581164a005 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Fri, 27 Jun 2014 08:53:34 -0400 Subject: [PATCH 4/6] Add libjpeg-turbo-progs to travis install --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 4de1fde99..cde26e9b9 100644 --- a/.travis.yml +++ b/.travis.yml @@ -14,7 +14,7 @@ python: - 3.4 install: - - "sudo apt-get -qq install libfreetype6-dev liblcms2-dev python-qt4 ghostscript libffi-dev cmake" + - "sudo apt-get -qq install libfreetype6-dev liblcms2-dev python-qt4 ghostscript libffi-dev libjpeg-turbo-progs cmake" - "pip install cffi" - "pip install coveralls nose" - if [ "$TRAVIS_PYTHON_VERSION" == "2.6" ]; then pip install unittest2; fi From a301d061fbff5c5ec4e6e1ae82a361deb295b7b6 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Fri, 27 Jun 2014 11:02:36 -0400 Subject: [PATCH 5/6] Better error checking in _save_netpbm --- PIL/GifImagePlugin.py | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/PIL/GifImagePlugin.py b/PIL/GifImagePlugin.py index 7f29ffe12..a195ff8c2 100644 --- a/PIL/GifImagePlugin.py +++ b/PIL/GifImagePlugin.py @@ -333,16 +333,32 @@ def _save_netpbm(im, fp, filename): # below for information on how to enable this. import os - from subprocess import Popen, check_call, PIPE + from subprocess import Popen, check_call, PIPE, CalledProcessError file = im._dump() if im.mode != "RGB": with open(filename, 'wb') as f: check_call(["ppmtogif", file], stdout=f) else: with open(filename, 'wb') as f: - #TODO: What happens if ppmquant fails? - ppmquant_proc = Popen(["ppmquant", "256", file], stdout=PIPE) - check_call(["ppmtogif"], stdin=ppmquant_proc.stdout, stdout=f) + + # Pipe ppmquant output into ppmtogif + # "ppmquant 256 %s | ppmtogif > %s" % (file, filename) + quant_cmd = ["ppmquant", "256", file] + togif_cmd = ["ppmtogif"] + quant_proc = Popen(quant_cmd, stdout=PIPE) + togif_proc = Popen(togif_cmd, stdin=quant_proc.stdout, stdout=f) + + # Allow ppmquant to receive SIGPIPE if ppmtogif exits + quant_proc.stdout.close() + + retcode = quant_proc.wait() + if retcode: + raise CalledProcessError(retcode, quant_cmd) + + retcode = togif_proc.wait() + if retcode: + raise CalledProcessError(retcode, togif_cmd) + try: os.unlink(file) except: From 8b365f542a7232cb324ba25ef93aa8079a3703b3 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Fri, 27 Jun 2014 18:12:37 -0400 Subject: [PATCH 6/6] Skip tests if external commands aren't found --- Tests/helper.py | 25 +++++++++++++++++++++++++ Tests/test_file_gif.py | 4 +++- Tests/test_file_jpeg.py | 3 +++ Tests/test_shell_injection.py | 5 +++++ 4 files changed, 36 insertions(+), 1 deletion(-) diff --git a/Tests/helper.py b/Tests/helper.py index 051912897..104b8cc3e 100644 --- a/Tests/helper.py +++ b/Tests/helper.py @@ -200,4 +200,29 @@ def lena(mode="RGB", cache={}): # cache[mode] = im return im + +def command_succeeds(cmd): + """ + Runs the command, which must be a list of strings. Returns True if the + command succeeds, or False if an OSError was raised by subprocess.Popen. + """ + import os + import subprocess + with open(os.devnull, 'w') as f: + try: + subprocess.Popen(cmd, stdout=f, stderr=subprocess.STDOUT).wait() + except OSError: + return False + return True + +def djpeg_available(): + return command_succeeds(['djpeg', '--help']) + +def cjpeg_available(): + return command_succeeds(['cjpeg', '--help']) + +def netpbm_available(): + return command_succeeds(["ppmquant", "--help"]) and \ + command_succeeds(["ppmtogif", "--help"]) + # End of file diff --git a/Tests/test_file_gif.py b/Tests/test_file_gif.py index 719847a96..e31779df0 100644 --- a/Tests/test_file_gif.py +++ b/Tests/test_file_gif.py @@ -1,4 +1,4 @@ -from helper import unittest, PillowTestCase, tearDownModule, lena +from helper import unittest, PillowTestCase, tearDownModule, lena, netpbm_available from PIL import Image from PIL import GifImagePlugin @@ -90,6 +90,7 @@ class TestFileGif(PillowTestCase): reloaded = roundtrip(im)[1].convert('RGB') self.assert_image_equal(im, reloaded) + @unittest.skipUnless(netpbm_available(), "netpbm not available") def test_save_netpbm_bmp_mode(self): img = Image.open(file).convert("RGB") @@ -97,6 +98,7 @@ class TestFileGif(PillowTestCase): GifImagePlugin._save_netpbm(img, 0, tempfile) self.assert_image_similar(img, Image.open(tempfile).convert("RGB"), 0) + @unittest.skipUnless(netpbm_available(), "netpbm not available") def test_save_netpbm_l_mode(self): img = Image.open(file).convert("L") diff --git a/Tests/test_file_jpeg.py b/Tests/test_file_jpeg.py index 5b2ba45e7..283c48eb7 100644 --- a/Tests/test_file_jpeg.py +++ b/Tests/test_file_jpeg.py @@ -1,4 +1,5 @@ from helper import unittest, PillowTestCase, tearDownModule, lena, py3 +from helper import djpeg_available, cjpeg_available import random from io import BytesIO @@ -275,11 +276,13 @@ class TestFileJpeg(PillowTestCase): 1:standard_chrominance_qtable}), 30) + @unittest.skipUnless(djpeg_available(), "djpeg not available") def test_load_djpeg(self): img = Image.open(test_file) img.load_djpeg() self.assert_image_similar(img, Image.open(test_file), 0) + @unittest.skipUnless(cjpeg_available(), "cjpeg not available") def test_save_cjpeg(self): img = Image.open(test_file) diff --git a/Tests/test_shell_injection.py b/Tests/test_shell_injection.py index d20f9b1a6..fd14e5f44 100644 --- a/Tests/test_shell_injection.py +++ b/Tests/test_shell_injection.py @@ -1,4 +1,5 @@ from helper import unittest, PillowTestCase, tearDownModule +from helper import djpeg_available, cjpeg_available, netpbm_available import shutil @@ -24,6 +25,7 @@ class TestShellInjection(PillowTestCase): # If file can't be opened, shell injection probably occurred Image.open(dest_file).load() + @unittest.skipUnless(djpeg_available(), "djpeg not available") def test_load_djpeg_filename(self): for filename in test_filenames: src_file = self.tempfile(filename) @@ -32,14 +34,17 @@ class TestShellInjection(PillowTestCase): im = Image.open(src_file) im.load_djpeg() + @unittest.skipUnless(cjpeg_available(), "cjpeg not available") def test_save_cjpeg_filename(self): im = Image.open(test_jpg) self.assert_save_filename_check(im, JpegImagePlugin._save_cjpeg) + @unittest.skipUnless(netpbm_available(), "netpbm not available") def test_save_netpbm_filename_bmp_mode(self): im = Image.open(test_gif).convert("RGB") self.assert_save_filename_check(im, GifImagePlugin._save_netpbm) + @unittest.skipUnless(netpbm_available(), "netpbm not available") def test_save_netpbm_filename_l_mode(self): im = Image.open(test_gif).convert("L") self.assert_save_filename_check(im, GifImagePlugin._save_netpbm)