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

Merged
merged 16 commits into from Jan 26, 2013

Projects

None yet

4 participants

@d1b
Contributor
d1b commented Nov 23, 2012

#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 Nov 23, 2012
Closed

/tmp/pip-build not secure #725

@pnasrat pnasrat and 1 other commented on an outdated diff Nov 23, 2012
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
pnasrat Nov 23, 2012 Contributor

Maybe use logger.fatal

@d1b
d1b Nov 23, 2012 Contributor

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.

@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 Nov 24, 2012
@qwcode qwcode reopened this Nov 24, 2012
@qwcode
Contributor
qwcode commented Nov 24, 2012

woops, sorry, clicked the wrong button : )

@d1b
Contributor
d1b commented Nov 24, 2012

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

@qwcode qwcode and 1 other commented on an outdated diff Nov 24, 2012
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
qwcode Nov 24, 2012 Contributor

help me out, why the quick return for windows?

@d1b
d1b Nov 24, 2012 Contributor

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

@qwcode
qwcode Nov 24, 2012 Contributor

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 Nov 24, 2012 Contributor

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

@qwcode qwcode and 1 other commented on an outdated diff Nov 24, 2012
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
qwcode Nov 24, 2012 Contributor

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

@d1b
d1b Nov 24, 2012 Contributor

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
qwcode Nov 24, 2012 Contributor

ok, good, got it.

@qwcode
Contributor
qwcode commented Nov 24, 2012

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

@d1b
Contributor
d1b commented Nov 24, 2012

@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
Contributor
d1b commented Nov 24, 2012

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

@qwcode
Contributor
qwcode commented Nov 24, 2012

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
Contributor
d1b commented Nov 24, 2012

@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
Contributor
qwcode commented Nov 24, 2012

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
Contributor
qwcode commented Nov 26, 2012

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

@guettli
guettli commented Nov 26, 2012

@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
Contributor
qwcode commented Nov 26, 2012

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

@d1b
Contributor
d1b commented Nov 28, 2012

@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
guettli commented Nov 28, 2012

@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
Contributor
d1b commented Nov 28, 2012

@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
guettli commented Nov 28, 2012

@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
Contributor
d1b commented Nov 28, 2012

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

@guettli
guettli commented Dec 10, 2012

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

@qwcode
Contributor
qwcode commented Dec 10, 2012
  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
guettli commented Dec 11, 2012

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
Contributor
qwcode commented Dec 12, 2012

@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
Contributor
d1b commented Dec 12, 2012

@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
guettli commented Jan 8, 2013

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
Contributor
d1b commented Jan 9, 2013

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

d1b added some commits Jan 13, 2013
@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
Contributor
d1b commented Jan 19, 2013

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

@guettli
guettli commented Jan 21, 2013

I looked at your patch, and it looks good.

@qwcode
Contributor
qwcode commented Jan 21, 2013

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
Contributor
d1b commented Jan 22, 2013

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

@qwcode
Contributor
qwcode commented Jan 22, 2013

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
Contributor
pnasrat commented Jan 24, 2013

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

@d1b
Contributor
d1b commented Jan 25, 2013

@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 Nov 23, 2012
@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
Contributor
pnasrat commented Jan 25, 2013

If you do this I or Marcus will merge
On Jan 25, 2013 7:01 AM, "David" notifications@github.com wrote:

@pnasrat https://github.com/pnasrat I'd rather do that when it is about
to be merged so that I don't have to continually keep it "mergable".


Reply to this email directly or view it on GitHubhttps://github.com/pypa/pip/pull/734#issuecomment-12698161.

@d1b
Contributor
d1b commented Jan 25, 2013

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

@qwcode
Contributor
qwcode commented Jan 25, 2013

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
Contributor
pnasrat commented Jan 25, 2013

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
Contributor
qwcode commented Jan 25, 2013

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

@qwcode
Contributor
qwcode commented Jan 26, 2013

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
Contributor
qwcode commented Jan 26, 2013

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 merged commit 5c10fc5 into pypa:develop Jan 26, 2013

1 check passed

default The Travis build passed
Details
@qwcode
Contributor
qwcode commented Jan 26, 2013

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 Jan 26, 2013
Merged

/tmp/pip-build fixes #780

@d1b
Contributor
d1b commented Jan 27, 2013

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

@qwcode
Contributor
qwcode commented Jan 27, 2013

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