Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix #725 and #729. /tmp/pip-build issues #734

Merged
merged 16 commits into from

4 participants

@d1b

#725: /tmp/pip-build not secure
#729: /tmp/pip-build can't be shared by several linux users

Signed-off-by: David db@d1b.org

@d1b d1b Fix #725 and #729.
Signed-off-by: David <db@d1b.org>
61cc16d
@d1b d1b referenced this pull request
Closed

/tmp/pip-build not secure #725

pip/locations.py
@@ -26,6 +27,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 +
@pnasrat Owner
pnasrat added a note

Maybe use logger.fatal

@d1b
d1b added a note

logger isn't used in that module at all at present. Also there is already a case where sys.exit() is called in the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@d1b d1b 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 <db@d1b.org>
9c5b58f
@qwcode qwcode closed this
@qwcode qwcode reopened this
@qwcode
Owner

woops, sorry, clicked the wrong button : )

@d1b

@qwcode ha! so does that mean you meant to press merge ;) ?

pip/locations.py
@@ -26,6 +27,29 @@ 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
@qwcode Owner
qwcode added a note

help me out, why the quick return for windows?

@d1b
d1b added a note

os.getuid() and other calls used later in the function do not work on windows.

@qwcode Owner
qwcode added a note

windows temp dirs are user isolated/protected anyway, atleast on some versions of windows.
on windows 7, you get something like this:

tempfile.gettempfir()
c:\users\me\appdata\local\temp

how about a comment mentioning that.

@d1b
d1b added a note

Okay. I don't use windows so I had no idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
pip/locations.py
@@ -26,6 +27,29 @@ 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:
@qwcode Owner
qwcode added a note

why this try/except ? why not just if os.path.isdir(path)?

@d1b
d1b added a note

Because I am ensuring there isn't a TOCTOU[0] bug here. If you did a os.path.isdir(path) that would would be vulnerable. [0] http://en.wikipedia.org/wiki/Time_of_check_to_time_of_use

@qwcode Owner
qwcode added a note

ok, good, got it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@qwcode
Owner

as for tests, unit tests of _get_build_prefix would be sufficient I think (with mocking for fstat/getuid)

@d1b

@qwcode if you point me where I should add a test for _get_build_prefix I will write one. As an aside I am not that familiar with the pip code base. I intend to help out with a few security related issues that are present in pip.

@d1b

@qwcode also is there an existing appropriate exception I could raise instead of doing sys.exit(1) ?

@qwcode
Owner

you could create a new tests/test_locations.py for your tests.

there's no unit tests yet for anything in pip.locations.
fyi, there's no class decorators in py2.5, if you end up trying that for your mock patching.
see tests/test_util.py for a rather obsessive mock/unittest example for a function in pip.util
btw, most of our test coverage is functional using scripttest and virtualenv.

our exceptions are in pip.exceptions. the closest one for this is "InstallationError" I guess, although it could occur on any pip command at import, but the code that's running relates to installation prep.

@d1b d1b Update the code _get_build_prefix to raise an exception instead of sy…
…s.exit()'ing and also document that on windows user temp directories are already isolated.

Signed-off-by: David <db@d1b.org>
d89b580
@d1b

@qwcode I might get around to writing tests one day. For now I do not think one is required. If you think one is - you write it :-)

@qwcode
Owner

this code is being added to prevent a certain security flaw.
whether you end up writing it or not, we need a test case that covers that scenario.

@qwcode
Owner

@guettli, since you opened the issue this relates to, just curious if you have any comments on this fix.

@guettli

@qwcode, your does not work if /tmp/pip-build-username is a symbolic link. I changed the code to this:

            file_uid=os.lstat(path).st_uid

Look at this example. User "evil" is bad and he wants to modify some files of tguettler, if tguettler uses pip:
The user "evil" created a symbolic link to a directory of tguettler

===> ls -dl /tmp/pip-build-tguettler
lrwxrwxrwx 1 evil evil  27 2012-11-26 09:47 /tmp/pip-build-tguettler -> /home/tguettler/tmp/hidden/

===> ls -dl /tmp/pip-build-tguettler/
drwxr-xr-x 2 tguettler tguettler 6 2012-11-26 09:46 /tmp/pip-build-tguettler/

That's why you need to use lstat().

@qwcode
Owner

@guettli , thanks.
btw, this is @d1b 's pull, just trying to rally feedback

@d1b

@guettli the change you have suggest using os.lstat(path) is incorrect - as it would be vulnerable to a time of check vs time of use vulnerability. However, you are correct that I am checking the target and not the link's uid, as I forgot to include the os.O_NOFOLLOW flag in the os.open() call.

@d1b d1b 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 <db@d1b.org>
502ae9b
@guettli

@d1b I am not a security expert. Can you please explain why your solution is not vulnerable to a time of check vs time of use vulnerability?

@d1b

@guettli I have updated the code so that the os.open() call includes the os.O_NOFOLLOW flag.

Responding to to your latest question regarding why this code is not vulnerable to a time of check vs time of use vulnerability: Using file descriptors returned by the operating system is a safe way to check the property of a file. Specifying and checking against a path in general not a safe approach as there could be a 'race' between the path check(with lstat) and later using the checked path.

@guettli

@d1b thank you for your explanation. But I think that the current pull request has the same problem like os.lstat(): there could be a 'race' between the path check(with os.open(...)) and later using the checked path. But I don't care about this race condition. The initial bug I found can be closed.

@d1b

@guettli there is no such race - unless a daemon or user rm's their temporary build directory :-)

@guettli

The current changes look good. What can I do to get this into the main line?

@qwcode
Owner

1) see my comment above about adding a test(s)
2) I'd personally prefer the check/creation of this dir not happening at import. i.e. what the path should be, should live in pip.locations, but the creation should happen when needed. the security logic could be factored out into pip.utils function. but that's me, maybe other maintainers wont care.
3) and lastly, this needs one or more other maintainers to look at it I think.

@guettli

Is it possible to create a file with a different user-id in the unittest? On linux you need root access to change the user-id of a file. I guess that's not possible, but maybe I am wrong.

You are right: it is not good to create the build_prefix during import time. But this was done before (at least the file name was created at import time). If creating the build_prefix should be refactored, then it can be done in a different pull request.

@qwcode
Owner

@guettli

we'd use the "mock" library to mock the fstat and getuid calls, i.e. we wouldn't actually need files owned by other users. we have a few pip tests using mock

you can certainly open another pull requests that resuses his commits up to this point, and then add to them.
i.e. 1) add his repo as a remote 2) check out his branch, 3) then branch off that, 4) then make your changes, and tests, 5) submit a pull from your branch.

@d1b

@guettli @qwcode I'll write some tests for this change soonish :-)

Regarding creating the directory at import time, it shouldn't be that big of an issue.

@guettli

BTW, I work around the problem by settings TMPDIR=$HOME/tmp. Then the pip-build directory is not shared between users.
@qwcode you wanted to write the tests....

@d1b
d1b commented

Sorry, I have been busy and haven't had a chance to finish the tests yet.

d1b added some commits
@d1b d1b 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 <db@d1b.org>
d3d817c
@d1b d1b Also un-patch the patched getpass.getuser method.
Signed-off-by: David <db@d1b.org>
bfd651d
@d1b

@guettli @qwcode I have added tests to cover the fix for this issue.

@guettli

I looked at your patch, and it looks good.

@qwcode
Owner

thanks, will need a rebase to be mergable. working on trying to get a 1.2.2beta out, so not trying to add much right now til after 1.2.2 comes out.

@qwcode qwcode Merge pull request #774 from qwcode/win_fixes
another case for our shutil.rmtree onerror function
485f87d
@d1b

@qwcode do you want me to rebase on this branch ?

@qwcode
Owner

it's not auto-mergable right now thru github, so the pt of you rebasing on the latest "develop" is to resolve whatever the conflict is.

@pnasrat
Owner

@d1b you should just go ahead and make it mergeble. Start by pulling develop and fixing any conflicts locally then repush to github.

@d1b

@pnasrat Sure I'll update it now, but if it becomes un-mergable before it is merged I'll wait until it is about to be merged to fix it again.

d1b added some commits
@d1b d1b Fix #725 and #729.
Signed-off-by: David <db@d1b.org>
eeaa64d
@d1b d1b 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 <db@d1b.org>
e7bf29e
@d1b d1b Update the code _get_build_prefix to raise an exception instead of sy…
…s.exit()'ing and also document that on windows user temp directories are already isolated.

Signed-off-by: David <db@d1b.org>
61c444e
@d1b d1b 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 <db@d1b.org>
dc3a359
@d1b d1b 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 <db@d1b.org>
8baeeba
@d1b d1b Also un-patch the patched getpass.getuser method.
Signed-off-by: David <db@d1b.org>
7f13624
@d1b d1b Merge branch 'fix_pip_build_directory' of github.com:d1b/pip into fix…
…_pip_build_directory

Conflicts:
	pip/locations.py

Signed-off-by: David <db@d1b.org>
5c10fc5
@pnasrat
Owner
@d1b

@pnasrat I have re-based the code against develop :-)

@qwcode
Owner

I wasn't looking to merge until after cutting a 1.3 branch and beta (which is soon.)
all my focus right now is on test fixes in and handling any blockers.

@pnasrat
Owner

This is a security fix, which I feel probably should go in to 1.3, but you're doing most of the work on the release so I'll defer to your judgement.

@qwcode
Owner

ok, I hear you, will look at the new tests later tonight .

@qwcode
Owner

ok, the 2 tests make sense to me.
going to go confirm tests work on windows now before merging...

a few thoughts that I won't hold up the merge for (but may fix later):

  • there could be a 3rd test for when the dir exists and it's owned by the current user
  • our suite for the most part uses nose.tools.assert_raises instead of the try/except pattern in your 2nd test
  • since there's just 2 tests, I'm inclined to just use the @patch decorator instead of the manual patching (I used manual patching in test_util because we can't use class decorators in py25, and there were so many tests with the same patch)
@qwcode
Owner

tests fail on windows. http://jenkins.qwcode.com/job/pip_win_33/10/console
some of the patching needs to be guarded and not run on windows.
I can fix it up later tonight or tomorrow. gotta run for the moment.

@qwcode qwcode referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@qwcode qwcode merged commit 5c10fc5 into from
@qwcode
Owner

the message from github here is a little misleading.
it almost appears like merged using this pull, but I didn't, I merged pull #779.
I guess because my branch fully subsumes this branch it appears like this.

@qwcode qwcode referenced this pull request
Merged

/tmp/pip-build fixes #780

@d1b

@qwcode thank you for you help and work in getting my fix for #725 and #729 merged :-)

@qwcode
Owner

sure, np. thanks for being persistent and working on the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 23, 2012
  1. @d1b

    Fix #725 and #729.

    d1b authored
    Signed-off-by: David <db@d1b.org>
  2. @d1b

    Clean up the _get_build_prefix code - close the open fd and print the…

    d1b authored
    … error message if the fd could not be opened (denied).
    
    Signed-off-by: David <db@d1b.org>
Commits on Nov 24, 2012
  1. @d1b

    Update the code _get_build_prefix to raise an exception instead of sy…

    d1b authored
    …s.exit()'ing and also document that on windows user temp directories are already isolated.
    
    Signed-off-by: David <db@d1b.org>
Commits on Nov 28, 2012
  1. @d1b

    In the os.open call to get the fd to check if the user specific build…

    d1b authored
    … directory
    
    is in fact owned by another user - add the os.O_NOFOLLOW flag to not follow symbolic links.
    
    Signed-off-by: David <db@d1b.org>
Commits on Jan 13, 2013
  1. @d1b

    Provide a test to cover the changes to pip/locations.py regarding the…

    d1b authored
    … use of
    
    the_get_build_prefix method to create a user specific build_prefix directory.
    
    Signed-off-by: David <db@d1b.org>
  2. @d1b

    Also un-patch the patched getpass.getuser method.

    d1b authored
    Signed-off-by: David <db@d1b.org>
Commits on Jan 22, 2013
  1. @qwcode

    Merge pull request #774 from qwcode/win_fixes

    qwcode authored
    another case for our shutil.rmtree onerror function
Commits on Jan 23, 2013
  1. @qwcode

    Merge pull request #776 from qwcode/win_fixes

    qwcode authored
    windows test fixes
Commits on Jan 25, 2013
  1. @qwcode

    update next release date

    qwcode authored
  2. @d1b

    Fix #725 and #729.

    d1b authored
    Signed-off-by: David <db@d1b.org>
  3. @d1b

    Clean up the _get_build_prefix code - close the open fd and print the…

    d1b authored
    … error message if the fd could not be opened (denied).
    
    Signed-off-by: David <db@d1b.org>
  4. @d1b

    Update the code _get_build_prefix to raise an exception instead of sy…

    d1b authored
    …s.exit()'ing and also document that on windows user temp directories are already isolated.
    
    Signed-off-by: David <db@d1b.org>
  5. @d1b

    In the os.open call to get the fd to check if the user specific build…

    d1b authored
    … directory
    
    is in fact owned by another user - add the os.O_NOFOLLOW flag to not follow symbolic links.
    
    Signed-off-by: David <db@d1b.org>
  6. @d1b

    Provide a test to cover the changes to pip/locations.py regarding the…

    d1b authored
    … use of
    
    the_get_build_prefix method to create a user specific build_prefix directory.
    
    Signed-off-by: David <db@d1b.org>
  7. @d1b

    Also un-patch the patched getpass.getuser method.

    d1b authored
    Signed-off-by: David <db@d1b.org>
  8. @d1b

    Merge branch 'fix_pip_build_directory' of github.com:d1b/pip into fix…

    d1b authored
    …_pip_build_directory
    
    Conflicts:
    	pip/locations.py
    
    Signed-off-by: David <db@d1b.org>
This page is out of date. Refresh to see the latest.
Showing with 110 additions and 2 deletions.
  1. +1 −1  docs/news.txt
  2. +28 −1 pip/locations.py
  3. +81 −0 tests/test_locations.py
View
2  docs/news.txt
@@ -5,7 +5,7 @@ News
Next Release
============
-Beta and final releases of 1.3 are planned for the end of 2012.
+Beta and final releases of 1.3 are planned for Feb 2013.
.. include:: ../CHANGES.txt
View
29 pip/locations.py
@@ -4,7 +4,9 @@
import site
import os
import tempfile
+import getpass
from pip.backwardcompat import get_python_lib
+import pip.exceptions
def running_under_virtualenv():
@@ -25,6 +27,31 @@ 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':
+ """ on windows(tested on 7) temp dirs are isolated """
+ return path
+ try:
+ os.mkdir(path)
+ except OSError:
+ file_uid = None
+ try:
+ fd = os.open(path, os.O_RDONLY | os.O_NOFOLLOW)
+ file_uid = os.fstat(fd).st_uid
+ os.close(fd)
+ except OSError:
+ file_uid = None
+ if file_uid != os.getuid():
+ 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.")
+ raise pip.exceptions.InstallationError(msg)
+ return path
if running_under_virtualenv():
build_prefix = os.path.join(sys.prefix, 'build')
@@ -33,7 +60,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:
View
81 tests/test_locations.py
@@ -0,0 +1,81 @@
+"""
+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
+ getpass.getuser = self.old_getpass_getuser
+ 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
Something went wrong with that request. Please try again.