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

urlopen without context in config validation #3575

Closed
jaraco opened this issue Sep 1, 2022 · 3 comments · Fixed by #3576
Closed

urlopen without context in config validation #3575

jaraco opened this issue Sep 1, 2022 · 3 comments · Fixed by #3576
Labels

Comments

@jaraco
Copy link
Member

jaraco commented Sep 1, 2022

I got this report through our security reports from an anonymous individual:

Running a static code analyser over a codebase including setuptools has identified that urlopen is used without passing an SSL config here https://github.com/pypa/setuptools/blob/main/setuptools/config/_validate_pyproject/formats.py#L118 – which means that the SSL certificate isn’t verified.

This isn’t an area I’m an expert in, but documentation suggests that we should be passing e.g. context=ssl.create_default_context() - see https://docs.python.org/3.5/library/urllib.request.html and https://docs.python.org/3.5/library/ssl.html#ssl.create_default_context

I agree there may be a security concern, but I deem it low impact (worst case, a malicious actor could inject invalid classifiers or remove valid ones, merely causing disruption to a build in a local context), so safe to disclose.

@abravalheri
Copy link
Contributor

Oh that is a good tip! I will have a look and probably change it. Thank you very much Jason.

In principle this should not affect setuptools, because the download is disabled:

trove_classifier._disable_download() # type: ignore

@bluetech
Copy link

bluetech commented Sep 8, 2022

which means that the SSL certificate isn’t verified.

I don't think this is true, at least not in recent Python versions. I checked the oldest version supported by setuptools, 3.7, and if a context isn't passed, it ends up using ssl.create_default_context (goes through urllib -> http.client.HTTPSConnection -> ssl).

https://github.com/python/cpython/blob/3.7/Lib/ssl.py#L634-L635

Can also verify by seeing that the following fails: python -C 'import urllib.request; urllib.request.urlopen('https://expired.badssl.com')'

@jaraco
Copy link
Member Author

jaraco commented Sep 8, 2022

Thanks @bluetech for checking the assumptions. Based on your finding, I’m closing this issue.

@jaraco jaraco closed this as completed Sep 8, 2022
@jaraco jaraco added the invalid label Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants