From eeaa64d25da3b030dd9459069eaf7d6109bf531d Mon Sep 17 00:00:00 2001 From: David Date: Sat, 24 Nov 2012 00:11:49 +1100 Subject: [PATCH 01/11] Fix #725 and #729. Signed-off-by: David --- pip/locations.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/pip/locations.py b/pip/locations.py index d378dd73997..96359a9863c 100644 --- a/pip/locations.py +++ b/pip/locations.py @@ -4,6 +4,7 @@ import site import os import tempfile +import getpass from pip.backwardcompat import get_python_lib @@ -25,6 +26,24 @@ def virtualenv_no_global(): if running_under_virtualenv() and os.path.isfile(no_global_file): return True +def _get_build_prefix(): + """ Returns a safe build_prefix """ + path = os.path.join(tempfile.gettempdir(), 'pip-build-%s' % \ + getpass.getuser()) + if sys.platform == 'win32': + return path + try: + os.mkdir(path) + except OSError: + fd = os.open(path, os.O_RDONLY) + stat = os.fstat(fd) + if os.getuid() != stat.st_uid: + print ("The temporary folder for building (%s) " % path + + "is not owned by your user!") + print("pip will not work until the temporary folder is " + \ + "either deleted or owned by your user account.") + sys.exit(1) + return path if running_under_virtualenv(): build_prefix = os.path.join(sys.prefix, 'build') @@ -33,7 +52,7 @@ def virtualenv_no_global(): # Use tempfile to create a temporary folder for build # Note: we are NOT using mkdtemp so we can have a consistent build dir # Note: using realpath due to tmp dirs on OSX being symlinks - build_prefix = os.path.realpath(os.path.join(tempfile.gettempdir(), 'pip-build')) + build_prefix = os.path.realpath(_get_build_prefix()) ## FIXME: keep src in cwd for now (it is not a temporary folder) try: From e7bf29e239b24f7d13c7b423457b4ae06b445bc1 Mon Sep 17 00:00:00 2001 From: David Date: Sat, 24 Nov 2012 10:50:59 +1100 Subject: [PATCH 02/11] Clean up the _get_build_prefix code - close the open fd and print the error message if the fd could not be opened (denied). Signed-off-by: David --- pip/locations.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/pip/locations.py b/pip/locations.py index 96359a9863c..454fbdbf5c7 100644 --- a/pip/locations.py +++ b/pip/locations.py @@ -35,9 +35,14 @@ def _get_build_prefix(): try: os.mkdir(path) except OSError: - fd = os.open(path, os.O_RDONLY) - stat = os.fstat(fd) - if os.getuid() != stat.st_uid: + file_uid = None + try: + fd = os.open(path, os.O_RDONLY) + file_uid = os.fstat(fd).st_uid + os.close(fd) + except OSError: + file_uid = None + if file_uid != os.getuid(): print ("The temporary folder for building (%s) " % path + "is not owned by your user!") print("pip will not work until the temporary folder is " + \ From 61c444e491f79f138c9433cf36fba790c69747c2 Mon Sep 17 00:00:00 2001 From: David Date: Sat, 24 Nov 2012 14:32:20 +1100 Subject: [PATCH 03/11] Update the code _get_build_prefix to raise an exception instead of sys.exit()'ing and also document that on windows user temp directories are already isolated. Signed-off-by: David --- pip/locations.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/pip/locations.py b/pip/locations.py index 454fbdbf5c7..00e720e1571 100644 --- a/pip/locations.py +++ b/pip/locations.py @@ -6,6 +6,7 @@ import tempfile import getpass from pip.backwardcompat import get_python_lib +import pip.exceptions def running_under_virtualenv(): @@ -31,6 +32,7 @@ def _get_build_prefix(): path = os.path.join(tempfile.gettempdir(), 'pip-build-%s' % \ getpass.getuser()) if sys.platform == 'win32': + """ on windows(tested on 7) temp dirs are isolated """ return path try: os.mkdir(path) @@ -43,11 +45,12 @@ def _get_build_prefix(): except OSError: file_uid = None if file_uid != os.getuid(): - print ("The temporary folder for building (%s) " % path + - "is not owned by your user!") + msg = "The temporary folder for building (%s) is not owned by your user!" \ + % path + print (msg) print("pip will not work until the temporary folder is " + \ "either deleted or owned by your user account.") - sys.exit(1) + raise pip.exceptions.InstallationError(msg) return path if running_under_virtualenv(): From dc3a359671944dc5c922004197e51cd48ecb8c5b Mon Sep 17 00:00:00 2001 From: David Date: Wed, 28 Nov 2012 22:30:56 +1100 Subject: [PATCH 04/11] In the os.open call to get the fd to check if the user specific build directory is in fact owned by another user - add the os.O_NOFOLLOW flag to not follow symbolic links. Signed-off-by: David --- pip/locations.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pip/locations.py b/pip/locations.py index 00e720e1571..36efdcaa9e5 100644 --- a/pip/locations.py +++ b/pip/locations.py @@ -39,7 +39,7 @@ def _get_build_prefix(): except OSError: file_uid = None try: - fd = os.open(path, os.O_RDONLY) + fd = os.open(path, os.O_RDONLY | os.O_NOFOLLOW) file_uid = os.fstat(fd).st_uid os.close(fd) except OSError: From 8baeebae318b868864a4693e8407e0b2e22f557f Mon Sep 17 00:00:00 2001 From: David Date: Sun, 13 Jan 2013 23:16:46 +1100 Subject: [PATCH 05/11] Provide a test to cover the changes to pip/locations.py regarding the use of the_get_build_prefix method to create a user specific build_prefix directory. Signed-off-by: David --- tests/test_locations.py | 80 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 tests/test_locations.py diff --git a/tests/test_locations.py b/tests/test_locations.py new file mode 100644 index 00000000000..2c5232a1c1a --- /dev/null +++ b/tests/test_locations.py @@ -0,0 +1,80 @@ +""" +locations.py tests + +""" +import os +import sys +import shutil +import tempfile +import getpass +from mock import Mock +import pip + +class TestLocations: + def setup(self): + self.tempdir = tempfile.mkdtemp() + self.st_uid = 9999 + self.username = "example" + self.patch() + + def tearDown(self): + self.revert_patch() + shutil.rmtree(self.tempdir, ignore_errors=True) + + def patch(self): + """ first store and then patch python methods pythons """ + self.tempfile_gettempdir = tempfile.gettempdir + self.old_os_fstat = os.fstat + self.old_os_getuid = os.getuid + self.old_getpass_getuser = getpass.getuser + + # now patch + tempfile.gettempdir = lambda : self.tempdir + getpass.getuser = lambda : self.username + os.getuid = lambda : self.st_uid + os.fstat = lambda fd : self.get_mock_fstat(fd) + + def revert_patch(self): + """ revert the patches to python methods """ + tempfile.gettempdir = self.tempfile_gettempdir + os.getuid = self.old_os_getuid + os.fstat = self.old_os_fstat + + def get_mock_fstat(self, fd): + """ returns a basic mock fstat call result. + Currently only the st_uid attribute has been set. + """ + result = Mock() + result.st_uid = self.st_uid + return result + + def get_build_dir_location(self): + """ returns a string pointing to the + current build_prefix. + """ + return os.path.join(self.tempdir, 'pip-build-%s' % self.username) + + def test_dir_created(self): + """ test that the build_prefix directory is generated when + _get_build_prefix is called. + """ + + assert not os.path.exists(self.get_build_dir_location() ), \ + "the build_prefix directory should not exist yet!" + from pip import locations + locations._get_build_prefix() + assert os.path.exists(self.get_build_dir_location() ), \ + "the build_prefix directory should now exist!" + + def test_error_raised_when_owned_by_another(self): + """ test calling _get_build_prefix when there is a temporary + directory owned by another user raises an InstallationError. + """ + from pip import locations + os.getuid = lambda : 1111 + os.mkdir(self.get_build_dir_location() ) + try: + locations._get_build_prefix() + raise AssertionError("An InstallationError should have been raised!") + except pip.exceptions.InstallationError: + pass From 7f1362412f1e81e91de5e4cc8683a68aba601209 Mon Sep 17 00:00:00 2001 From: David Date: Sun, 13 Jan 2013 23:20:38 +1100 Subject: [PATCH 06/11] Also un-patch the patched getpass.getuser method. Signed-off-by: David --- tests/test_locations.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_locations.py b/tests/test_locations.py index 2c5232a1c1a..50b9cdf0a82 100644 --- a/tests/test_locations.py +++ b/tests/test_locations.py @@ -37,6 +37,7 @@ def patch(self): def revert_patch(self): """ revert the patches to python methods """ tempfile.gettempdir = self.tempfile_gettempdir + getpass.getuser = self.old_getpass_getuser os.getuid = self.old_os_getuid os.fstat = self.old_os_fstat From 9868bc38b01cc095967fd05feae6291f4ad328b2 Mon Sep 17 00:00:00 2001 From: Marcus Smith Date: Fri, 25 Jan 2013 23:08:09 -0800 Subject: [PATCH 07/11] setup.py read function fix --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index cfeaccbbbc7..27692b3787a 100644 --- a/setup.py +++ b/setup.py @@ -8,7 +8,7 @@ here = os.path.abspath(os.path.dirname(__file__)) def read(*parts): - return codecs.open(os.path.join(here, *parts), 'r', 'utf8').read() + return codecs.open(os.path.join(here, *parts), 'r').read() def find_version(*file_paths): version_file = read(*file_paths) From 26c050a08638df3d7d69ba71bea5cdbbc0b72124 Mon Sep 17 00:00:00 2001 From: Marcus Smith Date: Fri, 25 Jan 2013 23:08:16 -0800 Subject: [PATCH 08/11] update changelog and authors --- AUTHORS.txt | 1 + CHANGES.txt | 3 +++ 2 files changed, 4 insertions(+) diff --git a/AUTHORS.txt b/AUTHORS.txt index fcd1240dd02..b5b3d61d1e7 100644 --- a/AUTHORS.txt +++ b/AUTHORS.txt @@ -14,6 +14,7 @@ Clay McClure Cody Soyland Daniel Holth Dave Abrahams +David (d1b) Dmitry Gladkov Donald Stufft Francesco diff --git a/CHANGES.txt b/CHANGES.txt index 773cc662bb8..dd17c64936c 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -7,6 +7,9 @@ develop (unreleased) * Added "pip list" for listing installed packages and the latest version available. Thanks Rafael Caricio, Miguel Araujo, Dmitry Gladkov (Pull #752) +* Fixed security issues with pip's use of temp build directories. + Thanks David (d1b) and Thomas Güttler. (Pull #779) + * Improvements to sphinx docs and cli help. (Pull #773) * Fixed issue #707, dealing with OS X temp dir handling, which was causing From 68d983d69abb0dcc6987fa7434fd2e4c3615141e Mon Sep 17 00:00:00 2001 From: Marcus Smith Date: Fri, 25 Jan 2013 23:08:29 -0800 Subject: [PATCH 09/11] update docs with new build dir --- docs/cookbook.txt | 2 +- pip/commands/install.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/cookbook.txt b/docs/cookbook.txt index 1b9a9c17e30..777b39e9860 100644 --- a/docs/cookbook.txt +++ b/docs/cookbook.txt @@ -72,7 +72,7 @@ pip allows you to *just* unpack archives to a build directory without installing $ pip install --no-install SomePackage -If you're in a virtualenv, the build dir is ``/build``. Otherwise, it's ``/pip-build`` +If you're in a virtualenv, the build dir is ``/build``. Otherwise, it's ``/pip-build-`` Afterwards, to finish the job of installing unpacked archives, run:: diff --git a/pip/commands/install.py b/pip/commands/install.py index 61b1541d6ce..959e8900f92 100644 --- a/pip/commands/install.py +++ b/pip/commands/install.py @@ -70,7 +70,7 @@ def __init__(self, *args, **kw): default=build_prefix, help='Directory to unpack packages into and build in. ' 'The default in a virtualenv is "/build". ' - 'The default for global installs is "/pip-build".') + 'The default for global installs is "/pip-build-".') cmd_opts.add_option( '-t', '--target', From 459561351e49fa4be4d73a2aa05aaec79a9215d0 Mon Sep 17 00:00:00 2001 From: Marcus Smith Date: Fri, 25 Jan 2013 23:09:46 -0800 Subject: [PATCH 10/11] windows and other misc test updates --- tests/test_locations.py | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/tests/test_locations.py b/tests/test_locations.py index 50b9cdf0a82..0db0f972a6e 100644 --- a/tests/test_locations.py +++ b/tests/test_locations.py @@ -8,6 +8,8 @@ import tempfile import getpass from mock import Mock +from nose import SkipTest +from nose.tools import assert_raises import pip class TestLocations: @@ -25,7 +27,9 @@ def patch(self): """ first store and then patch python methods pythons """ self.tempfile_gettempdir = tempfile.gettempdir self.old_os_fstat = os.fstat - self.old_os_getuid = os.getuid + if sys.platform != 'win32': + # os.getuid not implemented on windows + self.old_os_getuid = os.getuid self.old_getpass_getuser = getpass.getuser # now patch @@ -38,7 +42,9 @@ def revert_patch(self): """ revert the patches to python methods """ tempfile.gettempdir = self.tempfile_gettempdir getpass.getuser = self.old_getpass_getuser - os.getuid = self.old_os_getuid + if sys.platform != 'win32': + # os.getuid not implemented on windows + os.getuid = self.old_os_getuid os.fstat = self.old_os_fstat def get_mock_fstat(self, fd): @@ -55,11 +61,19 @@ def get_build_dir_location(self): """ return os.path.join(self.tempdir, 'pip-build-%s' % self.username) + def test_dir_path(self): + """ test the path name for the build_prefix + """ + from pip import locations + assert locations._get_build_prefix() == self.get_build_dir_location() + def test_dir_created(self): """ test that the build_prefix directory is generated when _get_build_prefix is called. """ - + #skip on windows, build dir is not created + if sys.platform == 'win32': + raise SkipTest() assert not os.path.exists(self.get_build_dir_location() ), \ "the build_prefix directory should not exist yet!" from pip import locations @@ -71,11 +85,18 @@ def test_error_raised_when_owned_by_another(self): """ test calling _get_build_prefix when there is a temporary directory owned by another user raises an InstallationError. """ + #skip on windows; this exception logic only runs on linux + if sys.platform == 'win32': + raise SkipTest() from pip import locations os.getuid = lambda : 1111 os.mkdir(self.get_build_dir_location() ) - try: - locations._get_build_prefix() - raise AssertionError("An InstallationError should have been raised!") - except pip.exceptions.InstallationError: - pass + assert_raises(pip.exceptions.InstallationError, locations._get_build_prefix) + + def test_no_error_raised_when_owned_by_you(self): + """ test calling _get_build_prefix when there is a temporary + directory owned by you raise no InstallationError. + """ + from pip import locations + os.mkdir(self.get_build_dir_location()) + locations._get_build_prefix() From 1af3f2b7140dd240d0c5836f59b5d589e94c7bb6 Mon Sep 17 00:00:00 2001 From: Marcus Smith Date: Fri, 25 Jan 2013 23:18:25 -0800 Subject: [PATCH 11/11] changelog fix --- CHANGES.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.txt b/CHANGES.txt index dd17c64936c..91e7c29cf6f 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -8,7 +8,7 @@ develop (unreleased) available. Thanks Rafael Caricio, Miguel Araujo, Dmitry Gladkov (Pull #752) * Fixed security issues with pip's use of temp build directories. - Thanks David (d1b) and Thomas Güttler. (Pull #779) + Thanks David (d1b) and Thomas Güttler. (Pull #780) * Improvements to sphinx docs and cli help. (Pull #773)