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

Provide security advisory for PyCrypto #56095

Merged
merged 2 commits into from Feb 10, 2020
Merged

Conversation

waynew
Copy link
Contributor

@waynew waynew commented Feb 8, 2020

Fixes #56080

@cachedout @dmurphy18 @sagetherage @frogunder

Does this look reasonable? Anything I should add/take away/adjust?

PyCrypto is broken, but comes with most distros. PyCryptodome(x) or
M2Crypto are preferred.

Fixes saltstack#56080
@waynew waynew requested a review from as a code owner Feb 8, 2020
@ghost ghost requested a review from DmitryKuzmenko Feb 8, 2020
@sagetherage
Copy link
Collaborator

@sagetherage sagetherage commented Feb 8, 2020

This looks pretty good, the pip download page is what I wrote in the ticket. I am not sure if that is a page I am familiar with unless that is the repo dot saltstack dot com. If it is that page, or another dot com page then we will put the same security wording there (once approved and merged) and review it as we stage then promote and that will be the 3. Please correct me if I am wrong @cachedout @dwoz @dmurphy18 @frogunder

@waynew
Copy link
Contributor Author

@waynew waynew commented Feb 8, 2020

The README.rst file gets read in our setup.py, which... if I understand correctly, is what gets set in the package build that's uploaded to PyPI. At least that's how typical packages work 🙂

@sagetherage
Copy link
Collaborator

@sagetherage sagetherage commented Feb 8, 2020

got it, pypi = pip install page, I am all set

@cachedout
Copy link
Contributor

@cachedout cachedout commented Feb 8, 2020

Hi @waynew. Thanks for doing this!

There are a few ways I think this could be improved:

  1. Documentation in the installation instructions.

  2. A mention of this caveat in the bootstrap instructions for cases where people are installing from git or passing the -P flag.

  3. Fedora instructions which use pip.

  4. OS X instructions which use pip

  5. RHEL instructions which use pip

  6. It may be worth mentioning this in the developer documentation? (Up to you.)

  7. It should probably be added to the documentation which describes how to harden salt.

  8. There is an old guide on packaging for Salt that should probably include this, or the page should be updated/removed.

It's not quite clear to me whether any packages in the repo will use pycrypto out of the box. I know that @dmurphy18 mentioned a few RHEL cases where a supported crypto library is used but just so I can understand are there any packages delivered from the Salt repo which would install a pycrypto dependency and not a supported crypto lib?

Thanks again!

@dmurphy18
Copy link
Contributor

@dmurphy18 dmurphy18 commented Feb 10, 2020

@cachedout Here are the following platforms which still have pycrypto installed, noting that this was the same for the previous Salt 2019.2.3 release, therefore no change in what Salt was installing.

Python 2:
Debian 8 (inc. Raspbian 8)
Debian 9 (inc. Raspbian 9)
Ubuntu 16.04
Ubuntu 18.04
RHEL 7
RHEL 6
Amazon Linux 1
Amazon Linux 2

Python 3:
Debian 9 (inc. Raspbian 9)
Ubuntu 16.04
Ubuntu 18.04
Amazon Linux 2

Python 3 RHEL 7 and RHEL 8 utilize M2Crypto
Python 3 Amazon Linux 2 provides pycryptodomex but still makes use of pycrypto, an oversight that will be fixed in next point release.

Will also adjust Python 2 & 3 Debian and Ubuntu to support minimum of pycryptodome in the next point release, where possible and time allowing.

@cachedout
Copy link
Contributor

@cachedout cachedout commented Feb 10, 2020

Here are the following platforms which still have pycrypto installed

@dmurphy18 Thanks! This is very helpful! I just want to understand what you mean by the above statement. For each item in the list you provided, will an out-of-the-box install of the package from the Salt repo install pycrypto and use it as the default, assuming that neither m2crypto nor pycryptodome were previously installed?

@waynew
Copy link
Contributor Author

@waynew waynew commented Feb 10, 2020

If I understand correctly, the python-crypto package dependency from the vendor repos will provide pycrypto.

@dmurphy18
Copy link
Contributor

@dmurphy18 dmurphy18 commented Feb 10, 2020

see saltstack/salt-pack#724
saltstack/salt-pack-py3#189

And yes, clean box will install pycrypto, noting that as before in Salt releases that if pycryptodomex or M2crypto is present , then they will be used, order of preference:

Lowest:. pycrypto
Medium: pycryptodomex
Highest: M2Crypto

This is unchanged from the last few years since I ported pycryptodomex to RHEL 7.
Note that RHEL 6 32-bit does not work with pycryptodomex v3.6.3 that Salt supplies (due to 64-bit assumptions in their code), this may have changed in later versions, recalling that SaltStack provides minimum versions of a package such that it will function with Salt, expecting that later OS versions of the package will supersede the version supplied by SaltStack.

@cachedout
Copy link
Contributor

@cachedout cachedout commented Feb 10, 2020

If I understand correctly, the python-crypto package dependency from the vendor repos will provide pycrypto.

Gotcha. But the end result in the same, no? That in cases for the machine listed above, stock installs of the Salt package on a clean machine result in Salt defaulting to using pycrypto during runtime. Is that correct?

@dmurphy18
Copy link
Contributor

@dmurphy18 dmurphy18 commented Feb 10, 2020

@cachedout Yes, unchanged from previous releases, for the last few years

@cachedout
Copy link
Contributor

@cachedout cachedout commented Feb 10, 2020

Thanks for the clarification.

It seems like this notice needs to be expanded to a few other places. Would it make sense to put it in the repo?

https://repo.saltstack.com/#ubuntu
https://repo.saltstack.com/#rhel
https://repo.saltstack.com/#amzn
https://repo.saltstack.com/#debian
https://repo.saltstack.com/#raspbian

@waynew What do you think about putting up changes in those locations, correlating to the specific targets that @dmurphy18 has mentioned?

@dwoz dwoz added the Documentation label Feb 10, 2020
dwoz
dwoz approved these changes Feb 10, 2020
waynew added a commit to waynew/salt-bootstrap that referenced this issue Feb 10, 2020
This will help address comments on saltstack/salt#56095
@dwoz dwoz merged commit 9adc221 into saltstack:master Feb 10, 2020
8 of 49 checks passed
@waynew
Copy link
Contributor Author

@waynew waynew commented Feb 10, 2020

@cachedout since each of those OS/pip install instructions come from the overall installation instructions I only added the warning to that. I've also opened a PR against bootstrap's readme to link to the hardening Salt docs.

@waynew waynew deleted the crypto-warning branch Feb 10, 2020
@cachedout
Copy link
Contributor

@cachedout cachedout commented Feb 11, 2020

@cachedout since each of those OS/pip install instructions come from the overall installation instructions

I'm not sure I understand. The front page of the documentation site links directly to the repo under the section "Installing Salt". Why wouldn't the per-distribution installation instructions which follow include this advisory?

install_salt

I don't recall exactly, either, but if memory serves, this page might also be where users are directed to after filling out SaltStack's marketing collection form for downloading Salt.

I've also opened a PR against bootstrap's readme to link to the hardening Salt docs.

Excellent! Thank you!

@waynew
Copy link
Contributor Author

@waynew waynew commented Feb 11, 2020

https://docs.saltstack.com/en/latest/topics/installation/index.html links to the platform-specific pip instructions, e.g.

  1. Fedora instructions which use pip.

For the repo.saltstack.com, I haven't touched those docs yet, but IIUC they're in another repo, right? It would be good to update those to warn that the platform ships a broken version of PyCrypto, which we will use, but we strongly recommend upgrading to something like pycryptodome, for instance.

@cachedout
Copy link
Contributor

@cachedout cachedout commented Feb 11, 2020

IIUC they're in another repo, right?

Yes, that is correct. I don't remember offhand where they are but @Ch3LL might remember.

It would be good to update those to warn that the platform ships a broken version of PyCrypto, which we will use, but we strongly recommend upgrading to something like pycryptodome, for instance.

Sounds good. Thanks!

@Ch3LL
Copy link
Contributor

@Ch3LL Ch3LL commented Feb 11, 2020

yeah @waynew feel free to reach out to me or @frogunder and we can point you in the right direction

@sagetherage sagetherage added the ZRELEASED - Neon label May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation ZRELEASED - Neon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants