From d4e7b617ff79a228a527d11d7cf033df7ce0799c Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 28 Jan 2021 10:26:33 +0100 Subject: [PATCH] [java] Update InsecureCryptoIvRule --- .../rule/security/HardCodedCryptoKeyRule.java | 8 ++- .../rule/security/InsecureCryptoIvRule.java | 70 ++++++++----------- .../rule/security/InsecureCryptoIvTest.java | 3 +- .../rule/security/xml/InsecureCryptoIv.xml | 4 +- 4 files changed, 38 insertions(+), 47 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/HardCodedCryptoKeyRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/HardCodedCryptoKeyRule.java index 197e067723..935a51e3e4 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/HardCodedCryptoKeyRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/HardCodedCryptoKeyRule.java @@ -51,11 +51,13 @@ public class HardCodedCryptoKeyRule extends AbstractJavaRulechainRule { // named variable if (firstArgumentExpression instanceof ASTVariableAccess) { ASTVariableAccess varAccess = (ASTVariableAccess) firstArgumentExpression; - ASTVariableDeclaratorId varDecl = varAccess.getSignature().getSymbol().tryGetNode(); - validateProperKeyArgument(data, varDecl.getInitializer()); + if (varAccess.getSignature() != null && varAccess.getSignature().getSymbol() != null) { + ASTVariableDeclaratorId varDecl = varAccess.getSignature().getSymbol().tryGetNode(); + validateProperKeyArgument(data, varDecl.getInitializer()); + } } - // hard coded array ASTArrayAllocation + // hard coded array if (firstArgumentExpression instanceof ASTArrayAllocation) { ASTArrayInitializer arrayInit = ((ASTArrayAllocation) firstArgumentExpression).getArrayInitializer(); if (arrayInit != null) { diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/InsecureCryptoIvRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/InsecureCryptoIvRule.java index 7dbb8bde5c..fe08fe852a 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/InsecureCryptoIvRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/security/InsecureCryptoIvRule.java @@ -1,21 +1,18 @@ -/** +/* * BSD-style license; for more info see http://pmd.sourceforge.net/license.html */ package net.sourceforge.pmd.lang.java.rule.security; -import net.sourceforge.pmd.lang.ast.Node; -import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression; import net.sourceforge.pmd.lang.java.ast.ASTArgumentList; -import net.sourceforge.pmd.lang.java.ast.ASTArguments; +import net.sourceforge.pmd.lang.java.ast.ASTArrayAllocation; import net.sourceforge.pmd.lang.java.ast.ASTArrayInitializer; -import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType; -import net.sourceforge.pmd.lang.java.ast.ASTLiteral; -import net.sourceforge.pmd.lang.java.ast.ASTName; -import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; -import net.sourceforge.pmd.lang.java.ast.ASTVariableInitializer; -import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; -import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTConstructorCall; +import net.sourceforge.pmd.lang.java.ast.ASTExpression; +import net.sourceforge.pmd.lang.java.ast.ASTStringLiteral; +import net.sourceforge.pmd.lang.java.ast.ASTVariableAccess; +import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId; +import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule; import net.sourceforge.pmd.lang.java.types.TypeTestUtil; /** @@ -34,58 +31,51 @@ import net.sourceforge.pmd.lang.java.types.TypeTestUtil; * @since 6.3.0 * */ -public class InsecureCryptoIvRule extends AbstractJavaRule { +public class InsecureCryptoIvRule extends AbstractJavaRulechainRule { + + private static final Class IV_PARAMETER_SPEC = javax.crypto.spec.IvParameterSpec.class; public InsecureCryptoIvRule() { - addRuleChainVisit(ASTAllocationExpression.class); + super(ASTConstructorCall.class); } @Override - public Object visit(ASTAllocationExpression node, Object data) { - if (TypeTestUtil.isA(javax.crypto.spec.IvParameterSpec.class, node.getFirstChildOfType(ASTClassOrInterfaceType.class))) { - Node firstArgument = null; - - ASTArguments arguments = node.getFirstChildOfType(ASTArguments.class); + public Object visit(ASTConstructorCall node, Object data) { + if (TypeTestUtil.isA(IV_PARAMETER_SPEC, node)) { + ASTArgumentList arguments = node.getArguments(); if (arguments.size() > 0) { - firstArgument = arguments.getFirstChildOfType(ASTArgumentList.class).getChild(0); - } - - if (firstArgument != null) { - ASTPrimaryPrefix prefix = firstArgument.getFirstDescendantOfType(ASTPrimaryPrefix.class); - validateProperIv(data, prefix); + validateProperIv(data, arguments.get(0)); } } return data; } - private void validateProperIv(Object data, ASTPrimaryPrefix firstArgumentExpression) { + private void validateProperIv(Object data, ASTExpression firstArgumentExpression) { if (firstArgumentExpression == null) { return; } // named variable - ASTName namedVar = firstArgumentExpression.getFirstDescendantOfType(ASTName.class); - if (namedVar != null) { - // find where it's declared, if possible - if (namedVar != null && namedVar.getNameDeclaration() instanceof VariableNameDeclaration) { - VariableNameDeclaration varDecl = (VariableNameDeclaration) namedVar.getNameDeclaration(); - ASTVariableInitializer initializer = varDecl.getAccessNodeParent().getFirstDescendantOfType(ASTVariableInitializer.class); - if (initializer != null) { - validateProperIv(data, initializer.getFirstDescendantOfType(ASTPrimaryPrefix.class)); - } + if (firstArgumentExpression instanceof ASTVariableAccess) { + ASTVariableAccess varAccess = (ASTVariableAccess) firstArgumentExpression; + if (varAccess.getSignature() != null && varAccess.getSignature().getSymbol() != null) { + ASTVariableDeclaratorId varDecl = varAccess.getSignature().getSymbol().tryGetNode(); + validateProperIv(data, varDecl.getInitializer()); } } // hard coded array - ASTArrayInitializer arrayInit = firstArgumentExpression.getFirstDescendantOfType(ASTArrayInitializer.class); - if (arrayInit != null) { - addViolation(data, firstArgumentExpression); + if (firstArgumentExpression instanceof ASTArrayAllocation) { + ASTArrayInitializer arrayInit = ((ASTArrayAllocation) firstArgumentExpression).getArrayInitializer(); + if (arrayInit != null) { + addViolation(data, arrayInit); + } } // string literal - ASTLiteral literal = firstArgumentExpression.getFirstDescendantOfType(ASTLiteral.class); - if (literal != null && literal.isStringLiteral()) { - addViolation(data, firstArgumentExpression); + ASTStringLiteral literal = firstArgumentExpression.descendants(ASTStringLiteral.class).first(); + if (literal != null) { + addViolation(data, literal); } } } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/security/InsecureCryptoIvTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/security/InsecureCryptoIvTest.java index f80916c1de..c6132a5d0d 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/security/InsecureCryptoIvTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/security/InsecureCryptoIvTest.java @@ -1,4 +1,4 @@ -/** +/* * BSD-style license; for more info see http://pmd.sourceforge.net/license.html */ @@ -6,7 +6,6 @@ package net.sourceforge.pmd.lang.java.rule.security; import net.sourceforge.pmd.testframework.PmdRuleTst; -@org.junit.Ignore("Rule has not been updated yet") public class InsecureCryptoIvTest extends PmdRuleTst { // no additional unit tests } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/security/xml/InsecureCryptoIv.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/security/xml/InsecureCryptoIv.xml index 7ab7bd2f70..5ad7713a11 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/security/xml/InsecureCryptoIv.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/security/xml/InsecureCryptoIv.xml @@ -101,9 +101,9 @@ import java.security.SecureRandom; public class Foo { void encrypt() { - byte[] iv = new byte[16]; + byte[] ivBytes = new byte[16]; SecureRandom sprng = new SecureRandom(); - sprng.nextBytes(iv); + sprng.nextBytes(ivBytes); IvParameterSpec ivs = new IvParameterSpec(ivBytes); } }