diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java new file mode 100644 index 00000000000..819bd30f83d --- /dev/null +++ b/pmd-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 varToTypeMapping = new HashMap<>(); + private final HashMap typeToDMLOperationMapping = new HashMap<>(); + private final HashSet 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 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 listIdentifiers, final String method, + final String definingType) { + final List 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 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 index fadd31cdf4a..749514a17f9 100644 --- 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 index bd1fb3f5e08..bf9935669f4 100644 --- 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 { ]]> + + + + Validate CRUD permission before DML operation + + 3 + + + + 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 index cb33469f05b..3903ad4a342 100644 --- 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"); } diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml new file mode 100644 index 00000000000..9eeeb0ec959 --- /dev/null +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml @@ -0,0 +1,141 @@ + + + + + + Proper CRUD,FLS via ESAPI + 0 + fieldsToView = new List { 'name' }; + if (ESAPI.accessController().isAuthorizedToView(Contact.sObject, fieldsToView)) { + c.Name = newName; + update c; + } + } +} ]]> + + + + Improper CRUD,FLS via ESAPI + 1 + fieldsToView = new List { 'name' }; + if (ESAPI.accessController().isAuthorizedToView(Lead.sObject, fieldsToView)) { + c.Name = newName; + update c; + } + } +} ]]> + + + + CRUD,FLS check for update + 0 + + + + + + No CRUD,FLS check for update + 1 + + + + + + Proper CRUD,FLS check for update + 0 + + + + + No CRUD check for insert + 1 + + + + + Proper CRUD check for insert + 0 + + + + + No CRUD check for delete + 1 + + + + + Proper CRUD check for delete + 0 + + + + +