Skip to content

Conversation

luchua-bc
Copy link
Contributor

@luchua-bc luchua-bc commented Dec 16, 2020

Using broken or weak cryptographic algorithms can leave data vulnerable to being decrypted.

The ECB encryption mode is vulnerable to replay attacks and the CBC mode of operation with PKCS#5 (or PKCS#7) padding is vulnerable to padding oracle attacks. These two types of algorithms are widely treated as insecure/broken cryptographic algorithms in modern standards.

Some references are:

  1. Test cases for risky or broken cryptographic algorithm erroneously labeled as not vulnerable #92 - OWASP Test Cases
  2. codeql/csharp/ql/src/Security Features/Encryption using ECB.ql - C# Query for Encryption using ECB
  3. AES encryption algorithm should be used with secured mode - SonarSource Rule RSPEC-4432
  4. CWE-327 - Use of a Broken or Risky Cryptographic Algorithm

This query adds these two categories to the list of insecure ciphers.

Please consider to merge this PR. Thanks.

aschackmull
aschackmull previously approved these changes Dec 16, 2020
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@smowton
Copy link
Contributor

smowton commented Dec 16, 2020

Looks like you got a real test failure in ql/java/ql/test/library-tests/Encryption/cryptoalgospec.ql

@aschackmull
Copy link
Contributor

Looks like there are actually 2 test failures:
ql/java/ql/test/library-tests/Encryption/cryptoalgospec.ql: Test failure (Expected output does not match)
ql/java/ql/test/library-tests/Encryption/secure.ql: Test failure (Expected output does not match)

@luchua-bc
Copy link
Contributor Author

Sorry I should have checked the other two test cases as well instead of insecure.ql only. I'm trying to exclude those two new patterns from the regex match in getASecureAlgorithmName() so that secure.ql can pass.

Please advise if there is a better approach to resolve this issue. Thanks.

@luchua-bc
Copy link
Contributor Author

I've removed those algorithms from the list of secure algorithms. Please review.

Thanks,
@luchua-bc

@aschackmull aschackmull merged commit 5106d5d into github:main Dec 18, 2020
@luchua-bc luchua-bc deleted the java-broken-crypto-algorithms branch December 18, 2020 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants