From 10e3bc172bd7a2b97b2a98fcf423a3232d3bdd25 Mon Sep 17 00:00:00 2001 From: Sergey Date: Fri, 16 Dec 2016 10:33:27 -0800 Subject: [PATCH] GC and thread safety changes --- .../apex/rule/security/ApexBadCryptoRule.java | 3 + .../security/ApexInsecureEndpointRule.java | 6 +- .../rule/security/ApexSOQLInjectionRule.java | 3 + .../security/ApexXSSFromURLParamRule.java | 61 ++++++------------- 4 files changed, 28 insertions(+), 45 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexBadCryptoRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexBadCryptoRule.java index 624b99755e..ec78cd699e 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexBadCryptoRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexBadCryptoRule.java @@ -64,6 +64,9 @@ public class ApexBadCryptoRule extends AbstractApexRule { validateStaticIVorKey(methodCall, data); } } + + potentiallyStaticBlob.clear(); + return data; } diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexInsecureEndpointRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexInsecureEndpointRule.java index ddbffa0c90..8366c40a7a 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexInsecureEndpointRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexInsecureEndpointRule.java @@ -29,7 +29,7 @@ public class ApexInsecureEndpointRule extends AbstractApexRule { private static final String SET_ENDPOINT = "setEndpoint"; private static final Pattern PATTERN = Pattern.compile("^http://.+?$", Pattern.CASE_INSENSITIVE); - private static final Set HTTP_ENDPOINT_STRINGS = new HashSet<>(); + private final Set httpEndpointStrings = new HashSet<>(); public ApexInsecureEndpointRule() { setProperty(CODECLIMATE_CATEGORIES, new String[] { "Security" }); @@ -74,7 +74,7 @@ public class ApexInsecureEndpointRule extends AbstractApexRule { if (o instanceof String) { String literal = (String) o; if (PATTERN.matcher(literal).matches()) { - HTTP_ENDPOINT_STRINGS.add(Helper.getFQVariableName(variableNode)); + httpEndpointStrings.add(Helper.getFQVariableName(variableNode)); } } } @@ -114,7 +114,7 @@ public class ApexInsecureEndpointRule extends AbstractApexRule { ASTVariableExpression variableNode = node.getFirstChildOfType(ASTVariableExpression.class); if (variableNode != null) { - if (HTTP_ENDPOINT_STRINGS.contains(Helper.getFQVariableName(variableNode))) { + if (httpEndpointStrings.contains(Helper.getFQVariableName(variableNode))) { addViolation(data, variableNode); } diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSOQLInjectionRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSOQLInjectionRule.java index c48aea4a50..236313c816 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSOQLInjectionRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSOQLInjectionRule.java @@ -80,6 +80,9 @@ public class ApexSOQLInjectionRule extends AbstractApexRule { reportVariables(m, data); } } + + safeVariables.clear(); + selectContainingVariables.clear(); return data; } diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexXSSFromURLParamRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexXSSFromURLParamRule.java index c3050049be..c6c80f60da 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexXSSFromURLParamRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexXSSFromURLParamRule.java @@ -4,17 +4,14 @@ package net.sourceforge.pmd.lang.apex.rule.security; -import java.util.Arrays; import java.util.HashSet; import java.util.List; import java.util.Set; import net.sourceforge.pmd.lang.apex.ast.ASTAssignmentExpression; import net.sourceforge.pmd.lang.apex.ast.ASTBinaryExpression; -import net.sourceforge.pmd.lang.apex.ast.ASTDottedExpression; import net.sourceforge.pmd.lang.apex.ast.ASTFieldDeclaration; import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression; -import net.sourceforge.pmd.lang.apex.ast.ASTReferenceExpression; import net.sourceforge.pmd.lang.apex.ast.ASTReturnStatement; import net.sourceforge.pmd.lang.apex.ast.ASTVariableDeclaration; import net.sourceforge.pmd.lang.apex.ast.ASTVariableExpression; @@ -40,7 +37,7 @@ public class ApexXSSFromURLParamRule extends AbstractApexRule { private static final String[] DOUBLE_VALUEOF = new String[] { "Double", "valueOf" }; private static final String[] STRING_ISEMPTY = new String[] { "String", "isEmpty" }; - private static final Set URL_PARAMETER_STRINGS = new HashSet<>(); + private final Set urlParameterStrings = new HashSet<>(); public ApexXSSFromURLParamRule() { setProperty(CODECLIMATE_CATEGORIES, new String[] { "Security" }); @@ -91,7 +88,7 @@ public class ApexXSSFromURLParamRule extends AbstractApexRule { List nodes = node.findChildrenOfType(ASTVariableExpression.class); for (ASTVariableExpression varExpression : nodes) { - if (URL_PARAMETER_STRINGS.contains(Helper.getFQVariableName(varExpression))) { + if (urlParameterStrings.contains(Helper.getFQVariableName(varExpression))) { addViolation(data, nodes.get(0)); } } @@ -100,10 +97,13 @@ public class ApexXSSFromURLParamRule extends AbstractApexRule { } private boolean isEscapingMethod(ASTMethodCallExpression methodNode) { - return isMethodCallChain(methodNode, HTML_ESCAPING) || isMethodCallChain(methodNode, JS_ESCAPING) - || isMethodCallChain(methodNode, JSINHTML_ESCAPING) || isMethodCallChain(methodNode, URL_ESCAPING) - || isMethodCallChain(methodNode, INTEGER_VALUEOF) || isMethodCallChain(methodNode, DOUBLE_VALUEOF) - || isMethodCallChain(methodNode, STRING_ISEMPTY) || isMethodCallChain(methodNode, ID_VALUEOF); + return Helper.isMethodCallChain(methodNode, HTML_ESCAPING) || Helper.isMethodCallChain(methodNode, JS_ESCAPING) + || Helper.isMethodCallChain(methodNode, JSINHTML_ESCAPING) + || Helper.isMethodCallChain(methodNode, URL_ESCAPING) + || Helper.isMethodCallChain(methodNode, INTEGER_VALUEOF) + || Helper.isMethodCallChain(methodNode, DOUBLE_VALUEOF) + || Helper.isMethodCallChain(methodNode, STRING_ISEMPTY) + || Helper.isMethodCallChain(methodNode, ID_VALUEOF); } private void processInlineMethodCalls(ASTMethodCallExpression methodNode, Object data, final boolean isNested) { @@ -115,7 +115,7 @@ public class ApexXSSFromURLParamRule extends AbstractApexRule { } } - if (isMethodCallChain(methodNode, URL_PARAMETER_METHOD)) { + if (Helper.isMethodCallChain(methodNode, URL_PARAMETER_METHOD)) { if (isNested) { addViolation(data, methodNode); } @@ -129,11 +129,11 @@ public class ApexXSSFromURLParamRule extends AbstractApexRule { // ApexPages.currentPage().getParameters().get(..) if (right != null) { - if (isMethodCallChain(right, URL_PARAMETER_METHOD)) { + if (Helper.isMethodCallChain(right, URL_PARAMETER_METHOD)) { ASTVariableExpression left = node.getFirstChildOfType(ASTVariableExpression.class); if (left != null) { - URL_PARAMETER_STRINGS.add(Helper.getFQVariableName(left)); + urlParameterStrings.add(Helper.getFQVariableName(left)); } } @@ -152,12 +152,14 @@ public class ApexXSSFromURLParamRule extends AbstractApexRule { if (variable != null) { // safe method - if (isMethodCallChain(methodNode, INTEGER_VALUEOF) || isMethodCallChain(methodNode, ID_VALUEOF) - || isMethodCallChain(methodNode, DOUBLE_VALUEOF) || isMethodCallChain(methodNode, STRING_ISEMPTY)) { + if (Helper.isMethodCallChain(methodNode, INTEGER_VALUEOF) + || Helper.isMethodCallChain(methodNode, ID_VALUEOF) + || Helper.isMethodCallChain(methodNode, DOUBLE_VALUEOF) + || Helper.isMethodCallChain(methodNode, STRING_ISEMPTY)) { return; } - if (URL_PARAMETER_STRINGS.contains(Helper.getFQVariableName(variable))) { + if (urlParameterStrings.contains(Helper.getFQVariableName(variable))) { if (!isEscapingMethod(methodNode)) { addViolation(data, variable); } @@ -190,7 +192,7 @@ public class ApexXSSFromURLParamRule extends AbstractApexRule { // Look for: foo = bar; final ASTVariableExpression right = reverseOrder ? nodes.get(0) : nodes.get(1); - if (URL_PARAMETER_STRINGS.contains(Helper.getFQVariableName(right))) { + if (urlParameterStrings.contains(Helper.getFQVariableName(right))) { addViolation(data, right); } } @@ -215,35 +217,10 @@ public class ApexXSSFromURLParamRule extends AbstractApexRule { final List nodes = node.findChildrenOfType(ASTVariableExpression.class); for (ASTVariableExpression n : nodes) { - if (URL_PARAMETER_STRINGS.contains(Helper.getFQVariableName(n))) { + if (urlParameterStrings.contains(Helper.getFQVariableName(n))) { addViolation(data, n); } } } - private boolean isMethodCallChain(ASTMethodCallExpression methodNode, final String... methodNames) { - String methodName = methodNames[methodNames.length - 1]; - if (Helper.isMethodName(methodNode, methodName)) { - ASTReferenceExpression reference = methodNode.getFirstChildOfType(ASTReferenceExpression.class); - if (reference != null) { - ASTDottedExpression dottedExpression = reference.getFirstChildOfType(ASTDottedExpression.class); - if (dottedExpression != null) { - ASTMethodCallExpression nestedMethod = dottedExpression - .getFirstChildOfType(ASTMethodCallExpression.class); - if (nestedMethod != null) { - String[] newMethodNames = Arrays.copyOf(methodNames, methodNames.length - 1); - return isMethodCallChain(nestedMethod, newMethodNames); - } else { - String[] newClassName = Arrays.copyOf(methodNames, methodNames.length - 1); - if (newClassName.length == 1) { - return Helper.isMethodName(methodNode, newClassName[0], methodName); - } - } - } - - } - } - - return false; - } }