[java] New security rule for finding hard-coded keys used for cryptographic operations #1046

sgorbaty · 2018-04-23T21:27:21Z

This change is to identify and report hard-coded keys used in HMAC and encryption/decryption operations.

sgorbaty · 2018-04-25T23:11:37Z

@jsotuyod heads up, one more rule.

sgorbaty · 2018-05-01T17:49:05Z

Hey @adangel @jsotuyod,

Just wondered when would be the next PMD release and if this PR would make it.
Thanks in advance!

Sergey

jsotuyod · 2018-05-01T18:09:54Z

@sgorbaty the next release (PMD 6.4.0) is scheduled for May 27th. You can check all our next-release schedules (roughly once a month) on https://github.com/pmd/pmd/milestones

This PR will definitely be part of 6.4.0, Andreas already scheduled it.

jsotuyod

Sorry for the delay, but I think I've some good feedback. We are still on plan to make it to the next release (6.4.0)

jsotuyod · 2018-05-11T00:43:38Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/HardCodedCryptoKeyRule.java

+
+    private <T> void extractPrimitiveTypesInner(Set<T> retVal, T field, List<ASTPrimitiveType> types) {
+        for (ASTPrimitiveType type : types) {
+            if (type.hasImageEqualTo("byte") || type.hasImageEqualTo("String")) {


a primitive type can never be a string

jsotuyod · 2018-05-11T00:44:14Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/HardCodedCryptoKeyRule.java

+                        ASTPrimaryPrefix prefix = init.getFirstChildOfType(ASTPrimaryPrefix.class);
+                        if (prefix != null) {
+                            ASTLiteral literal = prefix.getFirstChildOfType(ASTLiteral.class);
+                            if (literal != null) {


this check seems too relaxed... maybe also check if literal.isStringLiteral()?

jsotuyod · 2018-05-11T00:45:40Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/HardCodedCryptoKeyRule.java

+        List<ASTAllocationExpression> allocations = node.findDescendantsOfType(ASTAllocationExpression.class);
+        for (ASTAllocationExpression allocation : allocations) {
+
+            ASTClassOrInterfaceType declClassName = allocation.getFirstDescendantOfType(ASTClassOrInterfaceType.class);


rather than ask the rulechain to visit all ASTClassOrInterfaceBodyDeclaration and then manually look for ASTAllocationExpression, you could ask the rulechain to visit ASTAllocationExpression for you and move this hole block to visit(ASTAllocationExpression node, Object data), which will probably be cleaner / easier to read

jsotuyod · 2018-05-11T00:58:49Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/Util.java

+        // intentionally
+    }
+
+    public static Set<String> findVariablesPassedToAnyParam(ASTClassOrInterfaceBodyDeclaration node,


I find this utility class a little troubling for a number of reasons

The name Util is overly generic

The name is misleading findVariablesPassedToAnyParam actually finds any variables passed to a constructor of the given type that's allocated anywhere within this node. I for one didn't expect that from the call in HardCodedCryptoKeyRule until I read this...

I'm unsure this is common enough to require a utility or even considering moving it somewhere else (the AST?)

jsotuyod · 2018-05-11T04:28:08Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/HardCodedCryptoKeyRule.java

+        }
+
+        for (ASTLocalVariableDeclaration foundLocalVar : foundLocalVars) {
+            if (passedInIvVarNames.contains(foundLocalVar.getVariableName())) {


ASTLocalVariableDeclaration.getVariableName() returns the name of the first variable declarator id, but there may be more in the case of byte var1, var2, var3; and even the types may be slightly different thanks to arrays (ie: byte var1, var2[], var3[][];).

The Javadoc for getVariableName says this, but I fail to find a use-case where only the first one is going to be needed... I think ASTLocalVariableDeclaration should implement Iterable<ASTVariableDeclarator> and we should just use those...

jsotuyod · 2018-05-11T04:28:40Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/Util.java

+            Class<?> classToFind) {
+        Set<String> passedInIvVarNames = new HashSet<>();
+
+        // find new javax.crypto.spec.SecretKeySpec(...)


left over from copy-paste?

jsotuyod · 2018-05-11T04:29:05Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/Util.java

+
+    public static Set<String> findVariablesPassedToAnyParam(ASTClassOrInterfaceBodyDeclaration node,
+            Class<?> classToFind) {
+        Set<String> passedInIvVarNames = new HashSet<>();


the name remains too attached to a single case to be a generic utility

jsotuyod · 2018-05-11T04:36:43Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/HardCodedCryptoKeyRule.java

+                    validateProperKey(data, var.getFirstDescendantOfType(ASTVariableInitializer.class));
+                }
+            }
+        }


these 3 for loops are somewhat redundant and fail to deal with nested scopes properly (specially in the presence of lambdas and nested classes).

I'd just keep this second loop, but making sure to also iterate over the parent scopes.

If reworking this to start directly from the allocation instead of ASTClassOrInterfaceBodyDeclaration you automatically start from the inner-most scope and can easily work your way up

jsotuyod · 2018-05-21T20:12:30Z

Thanks @adangel for performing the changes. The code is way more succinct now, should be easier to maintain.

@sgorbaty thanks again for the contribution, as anticipated this will be part of 6.4.0 to be released next Sunday.

Sergey Gorbaty added 6 commits April 19, 2018 14:13

HardCoded keys used as secrets

ec86fd2

Removing comment

6f88111

comments

2378444

merging upstream

fcac918

Style fix

ac8135c

Final util

98fe1d3

adangel added this to the 6.4.0 milestone Apr 28, 2018

adangel added the a:new-rule Proposal to add a new built-in rule label Apr 28, 2018

jsotuyod reviewed May 11, 2018

View reviewed changes

jsotuyod added the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label May 12, 2018

adangel self-assigned this May 19, 2018

adangel changed the title ~~[Java][Security] Finding hard-coded keys used for cryptographic operations.~~ [java] New security rule for finding hard-coded keys used for cryptographic operations May 21, 2018

adangel removed the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label May 21, 2018

adangel merged commit 98fe1d3 into pmd:master May 21, 2018

adangel added a commit that referenced this pull request May 21, 2018

Update release notes, refs #1046

4302381

jsotuyod mentioned this pull request May 21, 2018

[java] Improve ASTLocalVariableDeclaration #1128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[java] New security rule for finding hard-coded keys used for cryptographic operations #1046

[java] New security rule for finding hard-coded keys used for cryptographic operations #1046

sgorbaty commented Apr 23, 2018

sgorbaty commented Apr 25, 2018

sgorbaty commented May 1, 2018

jsotuyod commented May 1, 2018

jsotuyod left a comment

jsotuyod May 11, 2018

jsotuyod May 11, 2018 •

edited

jsotuyod May 11, 2018

jsotuyod May 11, 2018 •

edited

jsotuyod May 11, 2018 •

edited

jsotuyod May 11, 2018

jsotuyod May 11, 2018

jsotuyod May 11, 2018

jsotuyod commented May 21, 2018

[java] New security rule for finding hard-coded keys used for cryptographic operations #1046

[java] New security rule for finding hard-coded keys used for cryptographic operations #1046

Conversation

sgorbaty commented Apr 23, 2018

sgorbaty commented Apr 25, 2018

sgorbaty commented May 1, 2018

jsotuyod commented May 1, 2018

jsotuyod left a comment

Choose a reason for hiding this comment

jsotuyod May 11, 2018

Choose a reason for hiding this comment

jsotuyod May 11, 2018 • edited

Choose a reason for hiding this comment

jsotuyod May 11, 2018

Choose a reason for hiding this comment

jsotuyod May 11, 2018 • edited

Choose a reason for hiding this comment

jsotuyod May 11, 2018 • edited

Choose a reason for hiding this comment

jsotuyod May 11, 2018

Choose a reason for hiding this comment

jsotuyod May 11, 2018

Choose a reason for hiding this comment

jsotuyod May 11, 2018

Choose a reason for hiding this comment

jsotuyod commented May 21, 2018

jsotuyod May 11, 2018 •

edited

jsotuyod May 11, 2018 •

edited

jsotuyod May 11, 2018 •

edited