Merge branch 'pr-148' into pmd/5.5.x

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 static final String DATABASE = "Database";
-	private final static String QUERY = "query";
+	private static final String QUERY = "query";
-	final private Set<String> safeVariables = new HashSet<>();
+	private final HashSet<String> safeVariables = new HashSet<>();
+	private final HashMap<String, Boolean> 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<ASTBinaryExpression> binaryExpr = m.findChildrenOfType(ASTBinaryExpression.class);
-		for (ASTBinaryExpression b : binaryExpr) {
-			List<ASTVariableExpression> 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<ASTAssignmentExpression> assignmentCalls = node.findDescendantsOfType(ASTAssignmentExpression.class);
-
 		for (ASTAssignmentExpression a : assignmentCalls) {
 			findSanitizedVariables(a);
+			findSelectContainingVariables(a);
 		}
 
 		final List<ASTFieldDeclaration> fieldExpr = node.findDescendantsOfType(ASTFieldDeclaration.class);
 		for (ASTFieldDeclaration a : fieldExpr) {
 			findSanitizedVariables(a);
+			findSelectContainingVariables(a);
 		}
 
 		// String foo = String.escapeSignleQuotes(...);
 		final List<ASTVariableDeclaration> variableDecl = node.findDescendantsOfType(ASTVariableDeclaration.class);
 		for (ASTVariableDeclaration a : variableDecl) {
 			findSanitizedVariables(a);
+			findSelectContainingVariables(a);
 		}
 
-		// Database.query(...)
+		// Database.query(...) check
 		final List<ASTMethodCallExpression> 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<ASTBinaryExpression> binaryExpr = m.findChildrenOfType(ASTBinaryExpression.class);
+		for (ASTBinaryExpression b : binaryExpr) {
+			List<ASTVariableExpression> 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);
+				}
+			}
+		}
+	}
+
 }

pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexSOQLInjection.xml

@@ -1,7 +1,63 @@
 <?xml version="1.0" encoding="UTF-8"?>
 
 <test-data>
+	<test-code>
+		<description>Potentially unsafe SOQL on concatenation of variables 1
+		</description>
+		<expected-problems>1</expected-problems>
+		<code><![CDATA[
+public class Foo {
+	public void test1() {
+		String field1 = getSomeID(); 
+		String field2 = 'SELECT Id FROM Account WHERE Id =';
+		Database.query(field2 + field1);
+	}		
+}
+		]]></code>
+	</test-code>
 
+	<test-code>
+		<description>Potentially unsafe SOQL on concatenation of variables 2
+		</description>
+		<expected-problems>1</expected-problems>
+		<code><![CDATA[
+public class Foo {
+	public void test1() {
+		String field1 = getSomeID(); 
+		String field2 = 'SELECT Id FROM Account WHERE Id =' + field1;
+		Database.query(field2);
+	}		
+}
+		]]></code>
+	</test-code>
+	<test-code>
+		<description>Safe SOQL concatenation of hardcoded
+			variables
+		</description>
+		<expected-problems>0</expected-problems>
+		<code><![CDATA[
+public class Foo {
+	public void test1() {
+		String field1 = 'someIDhere'; 
+		String field2 = 'SELECT Id FROM Account WHERE Id =';
+		Database.query(field2 + field1);
+	}		
+}
+		]]></code>
+	</test-code>
+	<test-code>
+		<description>Safe SOQL on concatenation of variables</description>
+		<expected-problems>0</expected-problems>
+		<code><![CDATA[
+public class Foo {
+	public void test1() {
+		String field1 = String.escapeSingleQuotes('yo'); 
+		String field2 = 'SELECT Id FROM Account WHERE Id =' + field1;
+		Database.query(field2);
+	}		
+}
+		]]></code>
+	</test-code>
 	<test-code>
 		<description>Safe SOQL + merged variable from a literal</description>
 		<expected-problems>0</expected-problems>
@@ -55,7 +111,8 @@ public class Foo {
 	</test-code>
 
 	<test-code>
-		<description>Potentially unsafe SOQL with multiple variable concatenation
+		<description>Potentially unsafe SOQL with multiple variable
+			concatenation
 		</description>
 		<expected-problems>2</expected-problems>
 		<code><![CDATA[
@@ -132,5 +189,4 @@ public class Foo {
 }
 		]]></code>
 	</test-code>
-
 </test-data>

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:**