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.

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

Fixed 2 problems on Jython and older POSIX environments #935

Closed
wants to merge 1 commit into from

Conversation

yyuu
Copy link

@yyuu yyuu commented May 7, 2013

Fixed 2 problems on Jython and older POSIX environments. This is a patch for #849.

  1. Stop using os.O_NOFOLLOW since it might not exist if the platform is not GNU or POSIX prior to 2008.
  2. Jython 2.5.x/2.7.x cannot handle directories by os.open()

(1) Stop using os.O_NOFOLLOW since it might not exist if the platform
    is not GNU or POSIX prior to 2008.
(2) Jython 2.5.x/2.7.x cannot handle directories by os.open()
@@ -61,9 +61,10 @@ def _get_build_prefix():
except OSError:
file_uid = None
try:
fd = os.open(path, os.O_RDONLY | os.O_NOFOLLOW)
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)

@yyuu yyuu closed this Jul 25, 2013
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants