From 4edc5526921a3bf04cbd0185134d61657e2006c7 Mon Sep 17 00:00:00 2001 From: Sergey Gorbaty Date: Thu, 1 Dec 2016 14:51:59 -0800 Subject: [PATCH 1/2] Improving detection of SOQL injection with variables flowing into Database.query --- .../rule/security/ApexSOQLInjectionRule.java | 192 ++++++++++++------ .../rule/security/xml/ApexSOQLInjection.xml | 66 +++++- 2 files changed, 196 insertions(+), 62 deletions(-) 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 7a4f89a78f..92b73274fe 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 @@ -1,8 +1,9 @@ package net.sourceforge.pmd.lang.apex.rule.security; +import java.util.HashMap; import java.util.HashSet; import java.util.List; -import java.util.Set; +import java.util.regex.Pattern; import apex.jorje.semantic.ast.expression.VariableExpression; import net.sourceforge.pmd.lang.apex.ast.ASTAssignmentExpression; @@ -27,9 +28,11 @@ public class ApexSOQLInjectionRule extends AbstractApexRule { private static final String JOIN = "join"; private static final String ESCAPE_SINGLE_QUOTES = "escapeSingleQuotes"; private static final String STRING = "String"; - private final static String DATABASE = "Database"; - private final static String QUERY = "query"; - final private Set safeVariables = new HashSet<>(); + private static final String DATABASE = "Database"; + private static final String QUERY = "query"; + private final HashSet safeVariables = new HashSet<>(); + private final HashMap selectContainingVariables = new HashMap<>(); + private static final Pattern pattern = Pattern.compile("^select[\\s]+?.+?$", Pattern.CASE_INSENSITIVE); public ApexSOQLInjectionRule() { setProperty(CODECLIMATE_CATEGORIES, new String[] { "Security" }); @@ -37,55 +40,6 @@ public class ApexSOQLInjectionRule extends AbstractApexRule { setProperty(CODECLIMATE_BLOCK_HIGHLIGHTING, false); } - private void findSanitizedVariables(AbstractApexNode m) { - final ASTVariableExpression left = m.getFirstChildOfType(ASTVariableExpression.class); - final ASTLiteralExpression literal = m.getFirstChildOfType(ASTLiteralExpression.class); - final ASTMethodCallExpression right = m.getFirstChildOfType(ASTMethodCallExpression.class); - - if (literal != null) { - if (left != null) { - final VariableExpression l = left.getNode(); - StringBuilder sb = new StringBuilder().append(l.getDefiningType()).append(":") - .append(l.getIdentifier().value); - safeVariables.add(sb.toString()); - } - } - - if (right != null) { - if (Helper.isMethodName(right, STRING, ESCAPE_SINGLE_QUOTES)) { - if (left != null) { - final VariableExpression l = left.getNode(); - StringBuilder sb = new StringBuilder().append(l.getDefiningType()).append(":") - .append(l.getIdentifier().value); - safeVariables.add(sb.toString()); - } - } - } - } - - private void reportChildren(ASTMethodCallExpression m, Object data) { - final List binaryExpr = m.findChildrenOfType(ASTBinaryExpression.class); - for (ASTBinaryExpression b : binaryExpr) { - List vars = b.findDescendantsOfType(ASTVariableExpression.class); - for (ASTVariableExpression v : vars) { - final VariableExpression l = v.getNode(); - StringBuilder sb = new StringBuilder().append(l.getDefiningType()).append(":") - .append(l.getIdentifier().value); - - if (safeVariables.contains(sb.toString())) { - continue; - } - ASTMethodCallExpression parentCall = v.getFirstParentOfType(ASTMethodCallExpression.class); - boolean isSafeMethod = Helper.isMethodName(parentCall, STRING, ESCAPE_SINGLE_QUOTES) - || Helper.isMethodName(parentCall, STRING, JOIN); - - if (!isSafeMethod) { - addViolation(data, v); - } - } - } - } - @Override public Object visit(ASTUserClass node, Object data) { @@ -95,34 +49,158 @@ public class ApexSOQLInjectionRule extends AbstractApexRule { // baz = String.escapeSignleQuotes(...); final List assignmentCalls = node.findDescendantsOfType(ASTAssignmentExpression.class); - for (ASTAssignmentExpression a : assignmentCalls) { findSanitizedVariables(a); + findSelectContainingVariables(a); } final List fieldExpr = node.findDescendantsOfType(ASTFieldDeclaration.class); for (ASTFieldDeclaration a : fieldExpr) { findSanitizedVariables(a); + findSelectContainingVariables(a); } // String foo = String.escapeSignleQuotes(...); final List variableDecl = node.findDescendantsOfType(ASTVariableDeclaration.class); for (ASTVariableDeclaration a : variableDecl) { findSanitizedVariables(a); + findSelectContainingVariables(a); } - // Database.query(...) + // Database.query(...) check final List potentialDbQueryCalls = node .findDescendantsOfType(ASTMethodCallExpression.class); for (ASTMethodCallExpression m : potentialDbQueryCalls) { - if (!Helper.isTestMethodOrClass(m) && Helper.isMethodName(m, DATABASE, QUERY)) { - reportChildren(m, data); + reportStrings(m, data); + reportVariables(m, data); } } return data; } + private void findSanitizedVariables(AbstractApexNode node) { + final ASTVariableExpression left = node.getFirstChildOfType(ASTVariableExpression.class); + final ASTLiteralExpression literal = node.getFirstChildOfType(ASTLiteralExpression.class); + final ASTMethodCallExpression right = node.getFirstChildOfType(ASTMethodCallExpression.class); + + // look for String a = 'b'; + if (literal != null) { + if (left != null) { + final VariableExpression l = left.getNode(); + StringBuilder sb = new StringBuilder().append(l.getDefiningType()).append(":") + .append(l.getIdentifier().value); + Object o = literal.getNode().getLiteral(); + if (o instanceof String) { + if (pattern.matcher((String) o).matches()) { + selectContainingVariables.put(sb.toString(), Boolean.TRUE); + } else { + safeVariables.add(sb.toString()); + } + } + } + } + + // look for String a = String.escapeSingleQuotes(foo); + if (right != null) { + if (Helper.isMethodName(right, STRING, ESCAPE_SINGLE_QUOTES)) { + if (left != null) { + final VariableExpression var = left.getNode(); + StringBuilder sb = new StringBuilder().append(var.getDefiningType().getApexName()).append(":") + .append(var.getIdentifier().value); + safeVariables.add(sb.toString()); + } + } + } + } + + private void findSelectContainingVariables(AbstractApexNode node) { + final ASTVariableExpression left = node.getFirstChildOfType(ASTVariableExpression.class); + final ASTBinaryExpression right = node.getFirstChildOfType(ASTBinaryExpression.class); + if (left != null && right != null) { + recursivelyCheckForSelect(left, right); + } + } + + private void recursivelyCheckForSelect(final ASTVariableExpression var, final ASTBinaryExpression node) { + final ASTBinaryExpression right = node.getFirstChildOfType(ASTBinaryExpression.class); + if (right != null) { + recursivelyCheckForSelect(var, right); + } + + final ASTVariableExpression concatenatedVar = node.getFirstChildOfType(ASTVariableExpression.class); + boolean isSafeVariable = false; + + if (concatenatedVar != null) { + StringBuilder sb = new StringBuilder().append(concatenatedVar.getNode().getDefiningType().getApexName()) + .append(":").append(concatenatedVar.getNode().getIdentifier().value); + if (safeVariables.contains(sb.toString())) { + isSafeVariable = true; + } + } + + final ASTLiteralExpression literal = node.getFirstChildOfType(ASTLiteralExpression.class); + if (literal != null) { + + Object o = literal.getNode().getLiteral(); + if (o instanceof String) { + if (pattern.matcher((String) o).matches()) { + StringBuilder sb = new StringBuilder().append(var.getNode().getDefiningType().getApexName()) + .append(":").append(var.getNode().getIdentifier().value); + if (!isSafeVariable) { + // select literal + other unsafe vars + selectContainingVariables.put(sb.toString(), Boolean.FALSE); + } + } + } + } + } + + private void reportStrings(ASTMethodCallExpression m, Object data) { + final List binaryExpr = m.findChildrenOfType(ASTBinaryExpression.class); + for (ASTBinaryExpression b : binaryExpr) { + List vars = b.findDescendantsOfType(ASTVariableExpression.class); + for (ASTVariableExpression v : vars) { + final VariableExpression var = v.getNode(); + StringBuilder sb = new StringBuilder().append(var.getDefiningType().getApexName()).append(":") + .append(var.getIdentifier().value); + + if (selectContainingVariables.containsKey(sb.toString())) { + boolean isLiteral = selectContainingVariables.get(sb.toString()); + if (isLiteral) { + continue; + } + } + + if (safeVariables.contains(sb.toString())) { + continue; + } + + final ASTMethodCallExpression parentCall = v.getFirstParentOfType(ASTMethodCallExpression.class); + boolean isSafeMethod = Helper.isMethodName(parentCall, STRING, ESCAPE_SINGLE_QUOTES) + || Helper.isMethodName(parentCall, STRING, JOIN); + + if (!isSafeMethod) { + addViolation(data, v); + } + } + } + } + + private void reportVariables(final ASTMethodCallExpression m, Object data) { + final ASTVariableExpression var = m.getFirstChildOfType(ASTVariableExpression.class); + if (var != null) { + StringBuilder sb = new StringBuilder().append(var.getNode().getDefiningType().getApexName()).append(":") + .append(var.getNode().getIdentifier().value); + if (selectContainingVariables.containsKey(sb.toString())) { + boolean isLiteral = selectContainingVariables.get(sb.toString()); + if (!isLiteral) { + addViolation(data, var); + } + } + } + } + } diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSOQLInjection.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSOQLInjection.xml index bbdc001d7a..57822cc380 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSOQLInjection.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSOQLInjection.xml @@ -1,7 +1,63 @@ + + Potentially unsafe SOQL on concatenation of variables 1 + + 1 + + + + Potentially unsafe SOQL on concatenation of variables 2 + + 1 + + + + Safe SOQL concatenation of hardcoded + variables + + 0 + + + + Safe SOQL on concatenation of variables + 0 + + Safe SOQL + merged variable from a literal 0 @@ -53,9 +109,10 @@ public class Foo { } ]]> - + - Potentially unsafe SOQL with multiple variable concatenation + Potentially unsafe SOQL with multiple variable + concatenation 2 - + Safe SOQL with List concatenation @@ -94,7 +151,7 @@ public class Foo { ]]> - SOQL with merged with field variable + SOQL with merged with field variable 0 - From 41cc401cd0b31b2493a59d7cc61bd50d969417eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Fri, 2 Dec 2016 17:13:35 -0300 Subject: [PATCH 2/2] Update changelog --- src/site/markdown/overview/changelog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index 68930bfde3..3ae1d4dde5 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -38,6 +38,7 @@ * [#137](https://github.com/pmd/pmd/pull/137): \[apex] Adjusted remediation points * [#146](https://github.com/pmd/pmd/pull/146): \[apex] Detection of missing Apex CRUD checks for SOQL/DML operations * [#147](https://github.com/pmd/pmd/pull/147): \[apex] Adding XSS detection to return statements +* [#148](https://github.com/pmd/pmd/pull/148): \[apex] Improving detection of SOQL injection * [#149](https://github.com/pmd/pmd/pull/149): \[apex] Whitelisting String.isEmpty and casting **Bugfixes:**