Java: Add RSA/ECB/OEAP ciphers to the list of secure algorithms

[grakshith]

This PR adds the ciphers RSA/ECB/OAEPWithSHA-1AndMGF1Padding and RSA/ECB/OAEPWithSHA-256AndMGF1Padding to the list of secure algorithms.

CodeQL flags the uses of these ciphers as risky/weak because it sees ECB in the cipher name and matches the regex from the getInsecureAlgorithmRegex() function.

The ciphers listed above are part of the Java Cryptographic Architecture and Java Security Standard, and after a quick google search it appears that none of the security providers operate RSA in the block mode. So ECB in the context of RSA is a misnomer.

I did omit the other algorithm RSA/ECB/PKCS1Padding as I believe PKCS1 padding is weaker compared to OAEP.

References

List of supported ciphers from the Java Security API
Java Security Spec
Stack overflow thread
Another thread

EDIT: Instead of adding these cipher to getASecureAlgorithmName we are now excluding ciphers matching the regex RSA/ECB/.* from the query.

[atorralba]

Hey @grakshith, thanks for your contribution.

I made a small comment below with a suggestion to keep things cohesive.

Your approach has a problem, though. I ran the updated query on the 1k most popular projects on GitHub and I saw that some unexpected algorithms would be discarded by your change, e.g.:

• SHA1withRSA (and its variants)
• ECIESwithSHA1 (and its variants)
• Blowfish/ECB/PKCS5Padding (and its variants)

So, instead of using the secure algorithm regex to discard results in this query (which also overlaps a bit with java/potentially-weak-cryptographic-algorithm), I think the safest thing here would be to add a specific exclusion for the RSA/ECB string.

Something like:

    not this.getValue().regexpMatch("RSA/ECB.*") and

What do you think?

[atorralba]

This ought to be fixed as follows to make sense with regexpMatch (getASecureAlgorithmName contains, perhaps a bit surprisingly, regexes):

Suggested change

	not this.getValue().regexpMatch(getASecureAlgorithmName()) and
	not this.getValue().regexpMatch(getSecureAlgorithmRegex()) and

Nonetheless see my comment above for the full picture.

[grakshith]

Ah, thanks for the callout. Didn't realize there was getSecureAlgorithmRegex

[grakshith]

Hey @grakshith, thanks for your contribution.

I made a small comment below with a suggestion to keep things cohesive.

Your approach has a problem, though. I ran the updated query on the 1k most popular projects on GitHub and I saw that some unexpected algorithms would be discarded by your change, e.g.:

• SHA1withRSA (and its variants)
• ECIESwithSHA1 (and its variants)
• Blowfish/ECB/PKCS5Padding (and its variants)

So, instead of using the secure algorithm regex to discard results in this query (which also overlaps a bit with java/potentially-weak-cryptographic-algorithm), I think the safest thing here would be to add a specific exclusion for the RSA/ECB string.

Something like:

    not this.getValue().regexpMatch("RSA/ECB.*") and

What do you think?

Hey @atorralba, thanks a lot for the review and your comments. Thanks for also running integration tests on the most popular repos. Your comment makes sense and I'll push out the change.

[grakshith]

Thanks @atorralba, I forgot to revert my previous changes 😅