Adding Apex CRUD violations

pmd · Nov 23, 2016 · 8ed7481 · 8ed7481
1 parent c2cd2fb
commit 8ed7481
Show file tree

Hide file tree

Showing 5 changed files with 418 additions and 0 deletions.
diff --git a/...apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java b/...apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java
@@ -0,0 +1,222 @@
+package net.sourceforge.pmd.lang.apex.rule.security;
+
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+
+import apex.jorje.data.ast.Identifier;
+import net.sourceforge.pmd.lang.apex.ast.ASTDmlDeleteStatement;
+import net.sourceforge.pmd.lang.apex.ast.ASTDmlInsertStatement;
+import net.sourceforge.pmd.lang.apex.ast.ASTDmlUpdateStatement;
+import net.sourceforge.pmd.lang.apex.ast.ASTDottedExpression;
+import net.sourceforge.pmd.lang.apex.ast.ASTMethod;
+import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression;
+import net.sourceforge.pmd.lang.apex.ast.ASTReferenceExpression;
+import net.sourceforge.pmd.lang.apex.ast.ASTSoqlExpression;
+import net.sourceforge.pmd.lang.apex.ast.ASTUserClass;
+import net.sourceforge.pmd.lang.apex.ast.ASTVariableDeclaration;
+import net.sourceforge.pmd.lang.apex.ast.ASTVariableExpression;
+import net.sourceforge.pmd.lang.apex.ast.AbstractApexNode;
+import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule;
+
+/**
+ * Finding missed CRUD checks.
+ * 
+ * @author sergey.gorbaty
+ *
+ */
+public class ApexCRUDViolationRule extends AbstractApexRule {
+	private final HashMap<String, String> varToTypeMapping = new HashMap<>();
+	private final HashMap<String, String> typeToDMLOperationMapping = new HashMap<>();
+	private final HashSet<String> esapiCheckedTypes = new HashSet<>();
+
+	private static final String IS_CREATEABLE = "isCreateable";
+	private static final String IS_DELETABLE = "isDeletable";
+	private static final String IS_UPDATEABLE = "isUpdateable";
+
+	private static final String S_OBJECT_TYPE = "sObjectType";
+	private static final String GET_DESCRIBE = "getDescribe";
+
+	// ESAPI.accessController().isAuthorizedToView(Lead.sObject, fields)
+	private static final String[] ESAPI_ISAUTHORIZED = new String[] { "ESAPI", "accessController",
+			"isAuthorizedToView" };
+	private static final String[] RESERVED_KEYS_FLS = new String[] { "Schema", S_OBJECT_TYPE };
+
+	// private static final String FIELDS = "fields";
+	// private static final String GET_MAP = "getMap";
+
+	public ApexCRUDViolationRule() {
+		setProperty(CODECLIMATE_CATEGORIES, new String[] { "Security" });
+		setProperty(CODECLIMATE_REMEDIATION_MULTIPLIER, 100);
+		setProperty(CODECLIMATE_BLOCK_HIGHLIGHTING, false);
+	}
+
+	@Override
+	public Object visit(ASTMethodCallExpression node, Object data) {
+		final String method = node.getNode().getMethodName();
+		final ASTReferenceExpression ref = node.getFirstChildOfType(ASTReferenceExpression.class);
+		if (ref == null) {
+			return data;
+		}
+
+		List<Identifier> a = ref.getNode().getJadtIdentifiers();
+		if (!a.isEmpty()) {
+			extractObjectAndFields(a, method, node.getNode().getDefiningType().getApexName());
+		} else {
+			// see if ESAPI
+			if (Helper.isMethodCallChain(node, ESAPI_ISAUTHORIZED)) {
+				extractObjectTypeFromESAPI(node);
+			}
+
+			// see if getDescribe()
+			final ASTDottedExpression dottedExpr = ref.getFirstChildOfType(ASTDottedExpression.class);
+			if (dottedExpr != null) {
+				final ASTMethodCallExpression nestedMethodCall = dottedExpr
+						.getFirstChildOfType(ASTMethodCallExpression.class);
+				if (nestedMethodCall != null) {
+					if (isLastMethodName(nestedMethodCall, S_OBJECT_TYPE, GET_DESCRIBE)) {
+						String resolvedType = getType(nestedMethodCall);
+						typeToDMLOperationMapping.put(resolvedType, method);
+					}
+				}
+			}
+
+		}
+
+		return data;
+	}
+
+	@Override
+	public Object visit(ASTDmlInsertStatement node, Object data) {
+		checkForCRUD(node, data, IS_CREATEABLE);
+		return data;
+	}
+
+	@Override
+	public Object visit(ASTDmlDeleteStatement node, Object data) {
+		checkForCRUD(node, data, IS_DELETABLE);
+		return data;
+	}
+
+	@Override
+	public Object visit(ASTDmlUpdateStatement node, Object data) {
+		checkForCRUD(node, data, IS_UPDATEABLE);
+		return data;
+	}
+
+	@Override
+	public Object visit(ASTSoqlExpression node, Object data) {
+		final ASTVariableDeclaration variable = node.getFirstParentOfType(ASTVariableDeclaration.class);
+		if (variable != null) {
+			String type = variable.getNode().getLocalInfo().getType().getApexName();
+			StringBuilder sb = new StringBuilder().append(variable.getNode().getDefiningType().getApexName())
+					.append(":").append(variable.getNode().getLocalInfo().getName());
+			varToTypeMapping.put(sb.toString(), type);
+		}
+		return data;
+	}
+
+	@Override
+	public Object visit(final ASTVariableDeclaration node, Object data) {
+		String type = node.getNode().getLocalInfo().getType().getApexName();
+		StringBuilder sb = new StringBuilder().append(node.getNode().getDefiningType().getApexName()).append(":")
+				.append(node.getNode().getLocalInfo().getName());
+		varToTypeMapping.put(sb.toString(), type);
+
+		return data;
+
+	}
+
+	private boolean isLastMethodName(final ASTMethodCallExpression methodNode, final String className,
+			final String methodName) {
+		final ASTReferenceExpression reference = methodNode.getFirstChildOfType(ASTReferenceExpression.class);
+		if (reference.getNode().getJadtIdentifiers().size() > 0) {
+			if (reference.getNode().getJadtIdentifiers().get(reference.getNode().getJadtIdentifiers().size() - 1).value
+					.equalsIgnoreCase(className) && Helper.isMethodName(methodNode, methodName)) {
+				return true;
+			}
+		}
+
+		return false;
+	}
+
+	private String getType(final ASTMethodCallExpression methodNode) {
+		final ASTReferenceExpression reference = methodNode.getFirstChildOfType(ASTReferenceExpression.class);
+		if (reference.getNode().getJadtIdentifiers().size() > 0) {
+			return new StringBuilder().append(reference.getNode().getDefiningType().getApexName()).append(":")
+					.append(reference.getNode().getJadtIdentifiers().get(0).value).toString();
+		}
+		return "";
+	}
+
+	private void extractObjectAndFields(final List<Identifier> listIdentifiers, final String method,
+			final String definingType) {
+		final List<String> strings = listIdentifiers.stream().map(id -> id.value).collect(Collectors.toList());
+
+		int flsIndex = Collections.lastIndexOfSubList(strings, Arrays.asList(RESERVED_KEYS_FLS));
+		if (flsIndex != -1) {
+			String objectTypeName = strings.get(flsIndex + RESERVED_KEYS_FLS.length);
+			typeToDMLOperationMapping.put(definingType + ":" + objectTypeName, method);
+			// TODO: add real FLS check
+			// if (FIELDS.equalsIgnoreCase(strings.get(++flsIndex +
+			// RESERVED_KEYS_FLS.length))) {
+			// String fieldName = strings.get(++flsIndex +
+			// RESERVED_KEYS_FLS.length);
+			// }
+
+		}
+	}
+
+	private void checkForCRUD(final AbstractApexNode<?> node, final Object data, final String CRUDMethod) {
+		final ASTMethod wrappingMethod = node.getFirstParentOfType(ASTMethod.class);
+		final ASTUserClass wrappingClass = node.getFirstParentOfType(ASTUserClass.class);
+
+		if (Helper.isTestMethodOrClass(wrappingClass) || Helper.isTestMethodOrClass(wrappingMethod)) {
+			return;
+		}
+
+		final ASTVariableExpression variable = node.getFirstChildOfType(ASTVariableExpression.class);
+		if (variable != null) {
+			StringBuilder sb = new StringBuilder().append(node.getNode().getDefiningType().getApexName()).append(":")
+					.append(variable.getNode().getIdentifier().value);
+
+			String type = varToTypeMapping.get(sb.toString());
+			if (type != null) {
+				StringBuilder typeCheck = new StringBuilder().append(node.getNode().getDefiningType()).append(":")
+						.append(type);
+
+				if (!typeToDMLOperationMapping.containsKey(typeCheck.toString())) {
+					if (!esapiCheckedTypes.contains(typeCheck.toString())) {
+						addViolation(data, node);
+					}
+				} else {
+					boolean properChecksHappened = typeToDMLOperationMapping.get(typeCheck.toString())
+							.equalsIgnoreCase(CRUDMethod);
+					if (!properChecksHappened) {
+						addViolation(data, node);
+					}
+				}
+			}
+		}
+	}
+
+	private void extractObjectTypeFromESAPI(final ASTMethodCallExpression node) {
+		final ASTVariableExpression var = node.getFirstChildOfType(ASTVariableExpression.class);
+		if (var != null) {
+			final ASTReferenceExpression reference = var.getFirstChildOfType(ASTReferenceExpression.class);
+			if (reference != null) {
+				List<Identifier> identifiers = reference.getNode().getJadtIdentifiers();
+				if (identifiers.size() == 1) {
+					StringBuilder sb = new StringBuilder().append(node.getNode().getDefiningType().getApexName())
+							.append(":").append(identifiers.get(0).value);
+					esapiCheckedTypes.add(sb.toString());
+				}
+
+			}
+		}
+
+	}
+}
diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/Helper.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/Helper.java
@@ -1,5 +1,6 @@
 package net.sourceforge.pmd.lang.apex.rule.security;
 
+import java.util.Arrays;
 import java.util.List;
 
 import apex.jorje.semantic.ast.expression.MethodCallExpression;
@@ -9,6 +10,7 @@
 import net.sourceforge.pmd.lang.apex.ast.ASTDmlUndeleteStatement;
 import net.sourceforge.pmd.lang.apex.ast.ASTDmlUpdateStatement;
 import net.sourceforge.pmd.lang.apex.ast.ASTDmlUpsertStatement;
+import net.sourceforge.pmd.lang.apex.ast.ASTDottedExpression;
 import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression;
 import net.sourceforge.pmd.lang.apex.ast.ASTModifierNode;
 import net.sourceforge.pmd.lang.apex.ast.ASTReferenceExpression;
@@ -87,4 +89,31 @@ static boolean isMethodName(final ASTMethodCallExpression m, final String method
 	static boolean isMethodName(final MethodCallExpression m, final String methodName) {
 		return m.getMethodName().equalsIgnoreCase(methodName);
 	}
+
+	static boolean isMethodCallChain(final ASTMethodCallExpression methodNode, final String... methodNames) {
+		String methodName = methodNames[methodNames.length - 1];
+		if (Helper.isMethodName(methodNode, methodName)) {
+			final ASTReferenceExpression reference = methodNode.getFirstChildOfType(ASTReferenceExpression.class);
+			if (reference != null) {
+				final ASTDottedExpression dottedExpression = reference.getFirstChildOfType(ASTDottedExpression.class);
+				if (dottedExpression != null) {
+					final 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;
+	}
+
 }
diff --git a/pmd-apex/src/main/resources/rulesets/apex/security.xml b/pmd-apex/src/main/resources/rulesets/apex/security.xml
@@ -163,5 +163,30 @@ public class Foo {
     ]]>
         </example>
 	</rule>
+
+	<rule name="ApexCRUDViolation" since="5.5.3"
+		message="Validate CRUD permission before DML operation"
+		class="net.sourceforge.pmd.lang.apex.rule.security.ApexCRUDViolationRule"
+		externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexCRUDViolationRule">
+		<description>
+            Validate CRUD permission before DML operation
+        </description>
+		<priority>3</priority>
+		<example>
+            <![CDATA[
+public class Foo {
+	public Contact foo(String status, String ID) {
+		Contact c = [SELECT Status__c FROM Contact WHERE Id=:ID];
+		if (!Schema.sObjectType.Contact.fields.Name.isUpdateable()){
+      		return null;
+    	}
+    	c.Status__c = status;
+    	update c;
+    	return c;
+	}
+}
+    ]]>
+        </example>
+	</rule>
 
 </ruleset>
diff --git a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/security/SecurityRulesTest.java b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/security/SecurityRulesTest.java
@@ -16,6 +16,7 @@ public void setUp() {
 		addRule(RULESET, "ApexSOQLInjection");
 		addRule(RULESET, "ApexSharingViolations");
 		addRule(RULESET, "ApexInsecureEndpoint");
+		addRule(RULESET, "ApexCRUDViolation");
 
 	}