Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed 2 problems on Jython and older POSIX environments #935

Closed
wants to merge 1 commit into from
Closed

Fixed 2 problems on Jython and older POSIX environments #935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions pip/locations.py
View file
Open in desktop
Expand Up @@ -61,9 +61,10 @@ def _get_build_prefix():
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)
if not os.path.islink(path):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason this code is using os.O_NOFOLLOW in the first place is to avoid a time of check to time of use security flaw with the pip location directory. This change re-introduces the old vulnerable behaviour on platforms that do support os.O_NOFOLLOW.

e.g.
At time 0: there is a folder in /tmp/$victim_user owned by another user so os.mkdir fails with an OSError. Then at time 1: just after the if not os.path.islink(path): check the attacker (due to the race condition now exposed) creates a symbolic link pointing to a data directory owned by $victim_user and the os.stat(path).st_uid check passes because the stat is done through the symbolic link!

Also, the O_NOFOLLOW flag was around before 2008 and according to a quick google recent versions of AIX have support for it. IMHO on Jython and posix like platforms perhaps we should just use tempfile.mkdtemp if they lack O_NOFOLLOW or os.fstat.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for review. I didn't know about old vulnerable behaviour.

As you described, using tempfile.mktemp might be suitable in this case. I'd like to update the code base (pip 1.4 now has been released) and re-send PR again. Thanks again :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't simply switch to using tempfile.mktemp for just one platform. pip has had the fixed build dir from the beginning. It would need proper deprecation, before we make the switch. (specifically, it would break the use of pip install --no-download)

# Use `os.stat()` instead of `os.fstat()` since Jython 2.5 lacks `os.fstat()`
# http://bugs.jython.org/issue1736
file_uid = os.stat(path).st_uid
except OSError:
file_uid = None
if file_uid != os.getuid():
Expand Down
28 changes: 17 additions & 11 deletions tests/unit/test_locations.py
View file
Open in desktop
Expand Up @@ -26,7 +26,6 @@ def tearDown(self):
def patch(self):
""" first store and then patch python methods pythons """
self.tempfile_gettempdir = tempfile.gettempdir
self.old_os_fstat = os.fstat
if sys.platform != 'win32':
# os.getuid not implemented on windows
self.old_os_getuid = os.getuid
Expand All @@ -36,7 +35,6 @@ def patch(self):
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 """
Expand All @@ -45,10 +43,9 @@ def revert_patch(self):
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):
""" returns a basic mock fstat call result.
def get_mock_stat(self, path):
""" returns a basic mock stat call result.
Currently only the st_uid attribute has been set.
"""
result = Mock()
Expand All @@ -61,11 +58,21 @@ def get_build_dir_location(self):
"""
return os.path.join(self.tempdir, 'pip-build-%s' % self.username)

def _get_build_prefix(self):
try:
# Patching `os.stat()` during just calling `locations._get_build_prefix()`.
# Since `os.stat()` is broadly used, patching while whole tests is problematic.
self.old_os_stat = os.stat
os.stat = lambda path : self.get_mock_stat(path)
from pip import locations
return locations._get_build_prefix()
finally:
os.stat = self.old_os_stat

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()
assert self._get_build_prefix() == self.get_build_dir_location()

def test_dir_created(self):
""" test that the build_prefix directory is generated when
Expand All @@ -76,8 +83,7 @@ def test_dir_created(self):
raise SkipTest()
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()
self._get_build_prefix()
assert os.path.exists(self.get_build_dir_location() ), \
"the build_prefix directory should now exist!"

Expand All @@ -91,12 +97,12 @@ def test_error_raised_when_owned_by_another(self):
from pip import locations
os.getuid = lambda : 1111
os.mkdir(self.get_build_dir_location() )
assert_raises(pip.exceptions.InstallationError, locations._get_build_prefix)
assert_raises(pip.exceptions.InstallationError, self._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()
self._get_build_prefix()