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

pkg_resources.resource_string allows absolute paths and paths with .. - contrary to docs #1635

Open
Mekk opened this Issue Jan 14, 2019 · 6 comments

Comments

Projects
None yet
2 participants
@Mekk
Copy link

Mekk commented Jan 14, 2019

The https://setuptools.readthedocs.io/en/latest/pkg_resources.html ("Basic Resource Access") page claims:

Note that resource names must be /-separated paths and cannot be absolute (i.e. no leading /)
or contain relative names like "..".

Let's see:

>>> pkg_resources.resource_string('multiprocessing', '/__init__.py')
'#\n# Package analogous …

>>> pkg_resources.resource_string('multiprocessing', '../../../../etc/passwd')
'root:x:0:0:root…

I'd say some validation is missing.

Tested on both python2.7 and python3.6, with pkg_resources as in Ubuntu 18.04

@Mekk

This comment has been minimized.

Copy link
Author

Mekk commented Jan 14, 2019

Note that this may have security impact… I already saw the code, which assumed that using pkg_resources guarantees that given path belongs to the package (so can be shown/safely used/…).

@jaraco jaraco added the bug label Jan 17, 2019

@jaraco jaraco self-assigned this Jan 17, 2019

jaraco added a commit that referenced this issue Jan 17, 2019

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jan 17, 2019

I've started working on this in https://github.com/pypa/setuptools/tree/bugfix/1635-disallow-parent-paths but with that change, I notice that there are many DeprecationWarnings in the setuptools codebase itself, including this call:

for subitem in metadata.resource_listdir('/'):

Due to the way the value is used, by splitting on / and passing the result to os.path.join, the leading / has no effect:

>>> '/'.split('/')
['', '']
>>> os.path.join('foo', '', '')
'foo/'
>>> os.path.join('foo', '', '', '')
'foo/'

So in the case of '/', I suggest just changing the docs to update the expectation.

jaraco added a commit that referenced this issue Jan 17, 2019

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jan 18, 2019

@Mekk Would you review the PR and share your thoughts?

@Mekk

This comment has been minimized.

Copy link
Author

Mekk commented Jan 18, 2019

  1. In my opinion leading / should be forbidden. It simply adds alternative confusing syntax. And there are various subtle implications.

    To give you a taste:

    • there exist real code which translates between url-s and pkg_resource apis, for example add_static_view in Pyramid. And in the context of url translation such extra slash may have non-trivial meaning…

    • such a slash may have meaning in case of non-filesystem-based resources, like various archives (some rar or another zip may be happy to extract some/file but not necessarily /some/file). Making restrictions consistent avoid such issues (this is my real case behind this bug, people used /slash/leading/paths, this worked for them on dev, then on true archive-based installation it failed)

    In general: pkg_resources should work the same whether package is on filesystem, in egg, or even provided by some custom loader (as long as that one fulfills specs).. Having some paths working on fs-based installation but not working elsewhere is painful.

  2. Regarding actual patch: what's going to happen on Windows, if \..\ happens (backslash-based illegal path)?

  3. Regarding docs: I'd add some note that currently invalid paths cause only warnings in case of filesystem-installed packages, but this may change into error in the future, and may fail with custom loaders even just now.

jaraco added a commit that referenced this issue Jan 21, 2019

jaraco added a commit that referenced this issue Jan 21, 2019

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jan 21, 2019

The latest patch also disallows the leading '/'. I imagine there are hundreds of cases that will be affected by this change, so a longer deprecation period will be required.

what's going to happen on Windows?

Possibly a similar issue. There was no protection for this issue before.

I'd add some note that currently invalid paths cause only warnings

I'd like to rely on the changelog to document transient aspects and allow the documentation to reflect the more permanent intentions/expectations.

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jan 27, 2019

what's going to happen on Windows?

On Windows, if a \ or C:\ is present at the beginning, it will give access to the whole system:

>>> ntpath.join('foo', 'C:\\bar.txt')                                                                                                         
'C:\\bar.txt'

So I've updated the PR to unconditionally disallow Windows-based absolute paths.

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