CryptoGuardOSS / cryptoguard

GNU General Public License v3.0
106 stars 30 forks source link

Case insensitive misuse rule checking for improving detection accuracy #6

Closed LordAmit closed 4 years ago

LordAmit commented 4 years ago

CryptoGuard currently has this in checkForMatchInternal method under PatternMatcherRuleChecker class:

for (String regex : getPatternsToMatch()) {
                    if (usebox.getValue().toString().matches(regex)) {
                        putIntoMap(predictableSourcMap, e, usebox.getValue().toString());
                        found = true;
                        break;
                    }
                }

Whereas the getPatternsToMatch() comes from PatternMatcherRuleChecker abstract class; and further implemented in BrokenCryptoFinder, BrokenHashFinder and HttpUrlFinder.

The core issue is that the patterns are being checked in a case sensitive manner. As a result, even though Cipher.getInstance("DES") will trigger a Rule 1 violation warning, Cipher.getInstance("des") will result in a different warning related to Rule 1a. Java Cipher class accepts all possible cases as it internally converts it to Upper Case. For example, Cipher class under JDK 7 has this line that allows converting user-defined transformation string to UpperCase.

private static Cipher.Transform getTransform(Service var0, List<Cipher.Transform> var1) {
String var2 = var0.getAlgorithm().toUpperCase(Locale.ENGLISH);
...
        }

One way to solve this is to redefine regular expressions to handle case sensitivity. However, that approach will result in redefining all regular expressions.

The other way is to change how the regular expression is being checked; which is what I am proposing through the pull request here.

for (String regex : getPatternsToMatch()) {
                    if(Pattern.compile(regex,Pattern.CASE_INSENSITIVE).matcher(usebox.getValue().toString()).matches()) {
                        putIntoMap(predictableSourcMap, e, usebox.getValue().toString());
                        found = true;
                        break;
                    }
                }

The change now allows checking the predefined patterns while ignoring case of the defined pattern.

sazzad114 commented 4 years ago

Thanks for your interest, Amit! I will merge your changes soon. :)

LordAmit commented 4 years ago

Thanks! Glad to contribute ✌️