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

The default .python-eggs mode doesn't satisfy pkg_resources's security check #254

Closed
bb-migration opened this Issue Sep 8, 2014 · 11 comments

Comments

Projects
None yet
1 participant
@bb-migration

bb-migration commented Sep 8, 2014

Originally reported by: amluto (Bitbucket: amluto, GitHub: amluto)


$ rm ~/.python-eggs/ -rf
$ [anything that uses resource_filename on a zipped egg]
/usr/lib/python3.3/site-packages/pkg_resources.py:979: UserWarning: /home/username/.python-eggs is writable by group/others and vulnerable to attack when used with get_resource_filename. Consider a more secure location (set with .set_extraction_path or the PYTHON_EGG_CACHE environment variable).
  warnings.warn(msg, UserWarning)

The directory creation code appears to be:

#!python

            _bypass_ensure_directory(target_path)

in get_cache_path. _bypass_ensure_directory had a default mode of 0777 (!).

The check in _warn_unsafe_extraction_path is:

#!python

if mode & stat.S_IWOTH or mode & stat.S_IWGRP:
    [warn about it]

In other words, get_cache_path is, indeed, unsafe, and _warn_unsafe_extraction_path correctly warns about it.

Presumably get_cache_path should be fixed.

(This is a real security problem depending on a user's group and umask.)


@bb-migration

This comment has been minimized.

bb-migration commented Sep 8, 2014

Original comment by amluto (Bitbucket: amluto, GitHub: amluto):


Fixed formatting error in the description.

@bb-migration

This comment has been minimized.

bb-migration commented Sep 9, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


What would be a reasonable default? o755?

Reading back through the history, it looks as if the o777 flag has been there since the function was created. Curiously, it was created as a "sandbox-bypassing version of ensure_directory" which pays no consideration to the mode. I'm inclined to just drop the mode altogether, as nothing is using it, and let the OS decide what are appropriate permissions.

Would that solve the warning in the common case?

@bb-migration

This comment has been minimized.

bb-migration commented Sep 9, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


I think I see now why _bypass_ensure_directory uses the mode - it's trying to mimic the makedirs function, which accepts a mode parameter and defaults to 0o777. makedirs also apparently masks out the umask.

I've just confirmed that os.mkdir also honors the umask, and that if the umask is set to a suitable value, setuptools will honor that.

jaraco@vdev-jaraco:~$ python
Python 2.7.3 (default, Feb 27 2014, 19:58:35)
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import pkg_resources
>>> pkg_resources._bypass_ensure_directory('issue254/test.txt')
>>>
jaraco@vdev-jaraco:~$ ls -lad issue254
drwxrwxr-x 2 jaraco jaraco 4096 Sep  9 06:49 issue254
jaraco@vdev-jaraco:~$ umask
0002
@bb-migration

This comment has been minimized.

bb-migration commented Sep 9, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


The behavior is by design. Setuptools/pkg_resources will honor the umask, which is the appropriate place to supply default permissions for new files/directories.

If there is a use-case where setuptools should be more restrictive than the umask, the project may consider that, but as written, this behavior is consistent with the behavior of loads of other apps.

@bb-migration

This comment has been minimized.

bb-migration commented Sep 9, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


On further consideration, the title of this ticket was well-written, and perhaps still valid - if the behavior is by design (honoring the umask), should the warning be posted at all? I'm not sure the library is in a good position to assess whether a given location is "secure" or not. After all, pkg_resources could be locked in a vault without Internet and only one user allowed to ever run it.

@bb-migration

This comment has been minimized.

bb-migration commented Sep 9, 2014

Original comment by amluto (Bitbucket: amluto, GitHub: amluto):


I would argue that the mode really should be 0755. I think that umask is traditionally for explicit file creation. That is, if I do:

echo text >filename

or

#!python

open('filename', 'w')

then I expect the file to be created with mode 0777 modified by my umask.

Calling resource_filename, on the other hand, isn't really explicit file creation; it's a request for a cached file. So I might reasonably set my umask to 0000 and then call resource_filename, not expecting to end up in an insecure situation, because I didn't expect resource_filename to create any files or directories at all.

If a user or administrator really wants multiple uids to be able to write to .python_eggs, then I think that they should set the mode on .python_eggs manually.

@bb-migration

This comment has been minimized.

bb-migration commented Feb 28, 2015

Original comment by vbraun (Bitbucket: vbraun, GitHub: vbraun):


IMHO The two sane behaviors are

  • Use umask, warn if the directory does not satisfy umask
  • Use 755, warn if directory is not 755

But the current behavior (use umask, warn if directory is not 755) is crap. Either trust me to set up my umask or don't. But don't spam everybody whose umask allows group-writeable by default. If you use a google search you'll see that thousands of users end up with spurious warnings.

@bb-migration

This comment has been minimized.

bb-migration commented Feb 28, 2015

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Volker, that does crystallize the issue. My preference would be for the former of the two. Could one of you put together a pull request?

@bb-migration

This comment has been minimized.

bb-migration commented Mar 5, 2015

Original comment by vbraun (Bitbucket: vbraun, GitHub: vbraun):


I've opened pull request #124 to implement the second version. Sorry, can't read ;-) But even if I had read it, I don't see the point in messing around with the umask. Just make it only user-writeable. Easy => safe. And nobody is sharing their egg directory with others in their primary group, that would be weird. So there is no use case for any extra complexity.

@bb-migration

This comment has been minimized.

bb-migration commented Mar 15, 2015

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Update changelog. Fixes #254.

@bb-migration

This comment has been minimized.

bb-migration commented Mar 15, 2015

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Released as 14.3.

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