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

Consider removing PyCryptodome from blacklist #319

Closed
Legrandin opened this issue Jun 19, 2018 · 9 comments
Closed

Consider removing PyCryptodome from blacklist #319

Legrandin opened this issue Jun 19, 2018 · 9 comments
Labels
enhancement New feature or request

Comments

@Legrandin
Copy link

As the maintainer of PyCryptodome, I have reservations on the library being blacklisted (B414).

Can you help me understand why you think it "has not fully addressed the issues inherent in PyCrypto"?

It shares none of the bugs, and it broke API compatibility with it by dropping the most dangerous ones (such as having ECB as the default cipher mode).

You can certainly shoot yourself in the foot with the low-level primitives, but the same can be achieved with cryptography (which is a great project, no doubts about it).

@lukehinds
Copy link
Member

I am not in the know enough to be arbiter here.

I am guessing you have fixed pycrypto/pycrypto#176 , if so this sounds reasonable to me, I can see the project is maintained / active.

@sigmavirus24 @ericwb any thoughts.

@lukehinds lukehinds added the enhancement New feature or request label Jun 19, 2018
@sigmavirus24
Copy link
Member

@Legrandin when this was added, PyCryptodome had all of the same vulnerabilities. It hasn't been re-evaluated yet and would require an unbiased evaluation (IMO).

@Legrandin
Copy link
Author

PyCryptodome had almost all PyCrypto vulnerabilities (like pycrypto/pycrypto#176) fixed right from the start, 4 years ago, whereas B414 was added one month ago as far as I can tell.

In 4 years, the whole C layer of PyCrypto has been rewritten and sanitized in stages (because honestly it was a potential dumpster fire) so it could be that more have been fixed along the road without me knowing it, but certainly that didn't happen after the evaluation that led to B414.

@ericwb
Copy link
Member

ericwb commented Jun 25, 2018

I think one of my concerns is all of the weak ciphers, hashes, etc are still included in PyCryptodome for backwards compatible purposes. But IMO they should really be removed to avoid users shooting themselves in the foot. Even for old designs, why are these okay to use?

@Legrandin
Copy link
Author

Old algorithms are still useful to access, verify and/or import old data (which would be lost otherwise) or interface with old system (say, not so recent embedded hardware).

So, even though I would also like to see only the latest and greatest algorithms being used, I find that approach only works in environments where code updates are cheap and data somewhat volatile (like web services, etc).

Additionally, the threat of people selecting outdated algorithms is probably one order of magnitude less serious than incorrect key generation and management (like using fixed keys and passwords, using bad RNG, storing keys in the clear or not wiping them) or unsafe compositions of otherwise safe algorithms, which are unfortunately more difficult to detect.

All in all, I think it would be more fair for bandit to blacklist individual modules in pycryptodome you feel strongly against (like Crypto.Cipher.DES), as opposed to entire packages. I also just noticed that cryptography too gives you access to weak algorithms (ECB mode, RSAES-PKCS1-v1_5, MD5, SHA1, TDES, etc) but bandit does not warn the user in that case, so that is certainly not showing consistency.

@Plazmaz
Copy link

Plazmaz commented Sep 28, 2018

+1.
I don't see why you would blacklist the entire module vs specific unsafe methods and algorithms. Pycryptodome is vastly improved compared to pycrypto, and is an easy transition for systems using pycrypto. Most encryption libraries have some form of support for legacy algorithms, which is just part of the reality of supporting older or legacy systems (as well as migrations from them). It's definitely worth flagging insecure function usage, but flagging the entire library is an unnecessarily nuclear approach.

EDIT: Also, as related but different isuse, pycryptodome triggers the pycrypto alert when using the Crypto imports (which makes sense)

@mchlnix
Copy link

mchlnix commented Nov 30, 2018

Is there a timeline for this? I'd also support bandit flagging individual modules. This would even promote the use of the newer safer algorithms in pycryptodome.

@ericwb
Copy link
Member

ericwb commented May 9, 2019

Fixed with #470

@ericwb ericwb closed this as completed May 9, 2019
@frispete
Copy link

Just a final note: this process much more document weaknesses in the bandit blacklist process than everything else!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants