hiifong/pmd
1
0
Fork 0
forked from phoedos/pmd
Code Pull Requests Activity

@W-8680425@: Refactored escape detection in script context. Multiple false positives resolved.

Browse Source
This commit is contained in:
Joshua Feingold 2021-01-25 14:39:25 -06:00
parent aa8524ca4a
commit c14398d19f
No known key found for this signature in database
GPG Key ID: 4A461D4C54A752ED
3 changed files with 344 additions and 78 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

69
pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/VfUnescapeElRule.java
View File

@ -57,78 +57,33 @@ public class VfUnescapeElRule extends AbstractVfRule {
}
private void checkIfCorrectlyEscaped(ASTHtmlScript node, Object data) {
ASTText prevText = null;
// churn thru every child just once instead of twice
for (int i = 0; i < node.getNumChildren(); i++) {
Node n = node.getChild(i);
if (n instanceof ASTText) {
prevText = (ASTText) n;
continue;
}
if (n instanceof ASTElExpression) {
processElInScriptContext((ASTElExpression) n, prevText, data);
processElInScriptContext((ASTElExpression) n, data);
}
}
}
private void processElInScriptContext(ASTElExpression elExpression, ASTText prevText, Object data) {
boolean quoted = false;
boolean jsonParse = false;
if (prevText != null) {
jsonParse = isJsonParse(prevText);
if (isUnbalanced(prevText.getImage(), '\'') || isUnbalanced(prevText.getImage(), '\"')) {
quoted = true;
}
}
if (quoted) {
// check escaping too
if (!(jsonParse
|| ElEscapeDetector.startsWithSafeResource(elExpression)
|| ElEscapeDetector.containsSafeFields(elExpression))) {
if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(elExpression,
JSENCODE_JSINHTMLENCODE)) {
addViolation(data, elExpression);
}
}
} else {
if (!(ElEscapeDetector.startsWithSafeResource(elExpression)
|| ElEscapeDetector.containsSafeFields(elExpression))) {
final boolean hasUnscaped = ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(elExpression,
JSENCODE_JSINHTMLENCODE);
if (!(jsonParse && !hasUnscaped)) {
addViolation(data, elExpression);
}
}
private void processElInScriptContext(ASTElExpression elExpression, Object data) {
if (!properlyEscaped(elExpression)) {
addViolation(data, elExpression);
}
}
private boolean isJsonParse(ASTText prevText) {
final String text = prevText.getImage().endsWith("'")
? prevText.getImage().substring(0, prevText.getImage().length() - 1) : prevText.getImage();
private boolean properlyEscaped(ASTElExpression el) {
// Find the first Expression-type child of this top-level node.
ASTExpression expression = el.getFirstChildOfType(ASTExpression.class);
return text.endsWith("JSON.parse(") || text.endsWith("jQuery.parseJSON(") || text.endsWith("$.parseJSON(");
}
private boolean isUnbalanced(String image, char pattern) {
char[] array = image.toCharArray();
boolean foundPattern = false;
for (int i = array.length - 1; i > 0; i--) {
if (array[i] == pattern) {
foundPattern = true;
}
if (array[i] == ';') {
return foundPattern;
}
// If no such node was found, then there's nothing to escape, so we're fine.
if (expression == null) {
return true;
}
return foundPattern;
// Otherwise, we should pass the expression node into our recursive checker.
return ElEscapeDetector.expressionRecursivelyValid(expression, JSENCODE_JSINHTMLENCODE);
}
@Override

241
pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/internal/ElEscapeDetector.java
View File

@ -4,6 +4,7 @@
package net.sourceforge.pmd.lang.vf.rule.security.internal;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.EnumSet;
import java.util.HashSet;
@ -11,6 +12,7 @@ import java.util.List;
import java.util.Locale;
import java.util.Set;
import net.sourceforge.pmd.lang.ast.Node;
import net.sourceforge.pmd.lang.vf.DataType;
import net.sourceforge.pmd.lang.vf.ast.ASTArguments;
import net.sourceforge.pmd.lang.vf.ast.ASTDotExpression;
@ -28,22 +30,227 @@ import net.sourceforge.pmd.lang.vf.ast.VfTypedNode;
public final class ElEscapeDetector {
private static final Set<String> SAFE_EXPRESSIONS = new HashSet<>(Arrays.asList("id", "size", "caseNumber"));
private static final Set<String> NON_EMPTY_ARG_SAFE_RESOURCE = new HashSet<>(Arrays.asList("urlfor", "casesafeid",
"begins", "contains", "len", "getrecordids", "linkto", "sqrt", "round", "mod", "log", "ln", "exp", "abs",
"floor", "ceiling", "nullvalue", "isnumber", "isnull", "isnew", "isblank", "isclone", "year", "month",
"day", "datetimevalue", "datevalue", "date", "now", "today"));
private static final Set<String> EMPTY_ARG_SAFE_RESOURCE = new HashSet<>(Arrays.asList("$action", "$page", "$site",
private static final Set<String> SAFE_PROPERTIES = new HashSet<>(Arrays.asList("id", "size", "caseNumber"));
private static final Set<String> SAFE_BUILTIN_FUNCTIONS = new HashSet<>(Arrays.asList(
// These DateTime functions accept or return dates, and therefore don't need escaping.
"addmonths", "date", "datevalue", "datetimevalue", "day", "hour", "millisecond", "minute", "month", "now",
"second", "timenow", "timevalue", "today", "weekday", "year",
// These Logical functions accept or return booleans, and therefore don't need escaping.
"and", "isblank", "isclone", "isnew", "isnull", "isnumber", "not", "or",
// These Math functions return numbers, and therefore don't require escaping.
"abs", "ceiling", "exp", "floor", "ln", "log", "max", "mceiling", "mfloor", "min", "mod", "round", "sqrt",
// These Text functions are safe, either because of what they accept or what they return.
"begins", "br", "casesafeid", "contains", "find", "getsessionid", "ispickval", "len",
// These Advanced functions are safe because of what they accept or what they return.
"currencyrate", "getrecordids", "ischanged", "junctionidlist", "regex", "urlfor"
));
private static final Set<String> FUNCTIONS_WITH_XSSABLE_ARG0 = new HashSet<>(Arrays.asList(
// For these methods, the first argument is a string that must be escaped.
"left", "lower", "lpad", "mid", "right", "rpad", "upper"
));
private static final Set<String> FUNCTIONS_WITH_XSSABLE_ARG2 = new HashSet<>(Arrays.asList(
// For these methods, the third argument is a string that must be escaped.
"lpad", "rpad"
));
private static final Set<String> SAFE_GLOBAL_VARS = new HashSet<>(Arrays.asList("$action", "$page", "$site",
"$resource", "$label", "$objecttype", "$component", "$remoteaction", "$messagechannel"));
private ElEscapeDetector() {
// utility class
}
/**
* Given an ASTExpression node, determines whether that expression and any expressions under it are properly escaped.
* @param expression - Represents a VF expression
* @param escapes - The escape operations that are acceptable in this context
* @return - True if the expression is properly escaped, otherwise false.
*/
public static boolean expressionRecursivelyValid(final ASTExpression expression, final EnumSet<Escaping> escapes) {
// We'll want to iterate over all of this expression's children.
int childCount = expression.getNumChildren();
String prevId = "";
List<ASTExpression> relevantChildren = new ArrayList<>();
for (int i = 0; i < childCount; i++) {
Node child = expression.getChild(i);
if (child instanceof ASTIdentifier) {
// How we deal with an Identifier depends on what the next node after it is.
if (i < childCount - 1) {
VfNode nextNode = expression.getChild(i + 1);
// If the next node is Arguments or a Dot expression, that means this Identifier represents the name
// of a function, or some kind of object. In that case, we might be okay. So we'll store the name
// and keep going.
if (nextNode instanceof ASTArguments || nextNode instanceof ASTDotExpression) {
prevId = child.getImage();
continue;
}
}
// If there's no next node, or the next node isn't one of those types, then this Identifier is a raw variable.
if (typedNodeIsSafe((ASTIdentifier) child)) {
// If the raw variable is of an inherently safe type, we can keep going.
continue;
}
// If the raw variable isn't inherently safe, then it's dangerous, and must be escaped.
return false;
} else if (child instanceof ASTArguments) {
// An Arguments node means we're looking at some kind of function call.
// If it's one of our designated escape functions, we're in the clear and we can keep going.
// Also, some built-in functions are inherently safe, which would mean we're good to continue.
if (functionIsEscape(prevId, escapes) || functionInherentlySafe(prevId)) {
continue;
}
// Otherwise, identify the argument expressions that must be checked, and add them to the list.
relevantChildren.addAll(getXssableArguments(prevId, (ASTArguments) child));
} else if (child instanceof ASTDotExpression) {
// Dot expressions mean we're doing accessing properties of variables.
// If the variable is one of the definitely-safe global variables, then we're in the clear.
if (isSafeGlobal(prevId)) {
continue;
}
// If the node after this one is also a Dot expression, then this is a chained access, and we can't make
// any final judgements.
if (i < childCount - 1 && expression.getChild(i + 1) instanceof ASTDotExpression) {
continue;
}
// If none of those things are true, then we need to determine whether the field being accessed is
// definitely safe.
ASTIdentifier propId = child.getFirstChildOfType(ASTIdentifier.class);
// If there's an identifier for a field/property, we need to check whether that property is inherently safe,
// either because it corresponds to a safe field or because its data type is known to be safe.
if (propId != null && !isSafeProperty(propId.getImage()) && !typedNodeIsSafe(propId)) {
// If the node isn't definitely safe, it ought to be escaped. Return false.
return false;
}
} else if (child instanceof ASTExpression) {
// Expressions should always be added to the list.
relevantChildren.add((ASTExpression) child);
}
}
// Just because there's nothing immediately wrong with this node doesn't mean its children are guaranteed to be
// fine. Iterate over all of the children and make a recursive call. If any of those calls return false, we need
// to relay that back up the chain.
for (ASTExpression e : relevantChildren) {
if (!expressionRecursivelyValid(e, escapes)) {
return false;
}
}
// If we didn't find a reason to return false, we're good. Return true.
return true;
}
/**
* Indicates whether the provided function name corresponds to any of the provided escape functions.
* @param functionName - The name of a VF function
* @param escapes - A set of acceptable escape functions (e.g., JSENCODE, HTMLENCODE, etc)
* @return - True if the function is a viable escape.
*/
private static boolean functionIsEscape(String functionName, EnumSet<Escaping> escapes) {
// If one of the escapes we were passed is ANY, then we should replace the provided set with one that contains
// all possible escapes.
EnumSet<Escaping> handledEscapes = escapes.contains(Escaping.ANY) ? EnumSet.allOf(Escaping.class) : escapes;
for (Escaping e : handledEscapes) {
if (functionName.equalsIgnoreCase(e.toString())) {
return true;
}
}
return false;
}
/**
* Certain built-in functions are inherently safe and don't require any escaping. This method determines whether the
* function name provided corresponds to one of those methods.
* @param functionName - The name of a VF function
* @return - True if the function is an inherently safe built-in function
*/
private static boolean functionInherentlySafe(String functionName) {
String lowerCaseName = functionName.toLowerCase(Locale.ROOT);
return SAFE_BUILTIN_FUNCTIONS.contains(lowerCaseName);
}
/**
* Given a function name and a node containing its arguments, returns the ASTExpression nodes corresponding to arguments
* that require escaping. Frequently, this will be all arguments, but not always.
* @param functionName - The name of a function being called
* @param arguments - Contains the ASTExpression nodes representing the function's arguments
* @return - ASTExpression list containing all arguments that are vulnerable to XSS.
*/
private static List<ASTExpression> getXssableArguments(String functionName, ASTArguments arguments) {
List<ASTExpression> exprs = new ArrayList<>();
int argCount = arguments.getNumChildren();
if (argCount != 0) {
String lowerCaseName = functionName.toLowerCase(Locale.ROOT);
List<Integer> indicesToCheck = new ArrayList<>();
// See if the function name corresponds to one of the built-in functions that don't require us to examine
// every argument.
if ("case".equals(lowerCaseName)) {
// CASE accepts (exx, val1, res1, val2, res2, ...else_res). We want resX and else_res.
for (int i = 2; i < argCount; i += 2) {
indicesToCheck.add(i);
}
indicesToCheck.add(argCount - 1);
} else if ("if".equals(lowerCaseName)) {
// IF accepts (test, if_true, if_false). We care about if_true and if_false.
indicesToCheck.add(1);
indicesToCheck.add(2);
} else {
boolean checkAllArgs = true;
// If this is a function with an XSSable first arg, add 0 to the array.
if (FUNCTIONS_WITH_XSSABLE_ARG0.contains(lowerCaseName)) {
checkAllArgs = false;
indicesToCheck.add(0);
}
// If this is a function that has at least 3 args, and the third arg is XSSable, add 2 to the array.
if (argCount > 2 && FUNCTIONS_WITH_XSSABLE_ARG2.contains(lowerCaseName)) {
checkAllArgs = false;
indicesToCheck.add(2);
}
// If the function has no known pattern for argument checking, all arguments must be checked.
if (checkAllArgs) {
for (int i = 0; i < argCount; i++) {
indicesToCheck.add(i);
}
}
}
// Add each of the targeted arguments to the return array if they represent an Expression node. (They always
// should, but better safe than sorry.)
for (int i : indicesToCheck) {
VfNode ithArg = arguments.getChild(i);
if (ithArg instanceof ASTExpression) {
exprs.add((ASTExpression) ithArg);
}
}
}
return exprs;
}
/**
* VF has global variables prefixed with a '$'. Some of those are inherently safe to access, and this method determines
* whether the provided ID corresponds to one of those globals.
* @param id - Identifier of some variable.
* @return - True if the global is inherently safe.
*/
private static boolean isSafeGlobal(String id) {
String lowerCaseId = id.toLowerCase(Locale.ROOT);
return SAFE_GLOBAL_VARS.contains(lowerCaseId);
}
/**
* Determines whether the property being referenced is inherently safe, or if it requires XSS escaping.
* @param propertyName - The name of a field or property being referenced.
* @return - True if that field/property is inherently safe
*/
private static boolean isSafeProperty(String propertyName) {
String lowerCaseName = propertyName.toLowerCase(Locale.ROOT);
return SAFE_PROPERTIES.contains(lowerCaseName);
}
private static boolean innerContainsSafeFields(final VfNode expression) {
for (VfNode child : expression.children()) {
if (child instanceof ASTIdentifier && SAFE_EXPRESSIONS.contains(child.getImage().toLowerCase(Locale.ROOT))) {
if (child instanceof ASTIdentifier && isSafeProperty(child.getImage())) {
return true;
}
@ -80,17 +287,13 @@ public final class ElEscapeDetector {
final ASTIdentifier id = expression.getFirstChildOfType(ASTIdentifier.class);
if (id != null) {
String lowerCaseId = id.getImage().toLowerCase(Locale.ROOT);
List<ASTArguments> args = expression.findChildrenOfType(ASTArguments.class);
if (!args.isEmpty() && NON_EMPTY_ARG_SAFE_RESOURCE.contains(lowerCaseId)) {
return true;
if (!args.isEmpty()) {
return functionInherentlySafe(id.getImage());
} else {
return isSafeGlobal(id.getImage());
}
if (args.isEmpty() && EMPTY_ARG_SAFE_RESOURCE.contains(lowerCaseId)) {
return true;
}
}
}
return false;
@ -158,8 +361,7 @@ public final class ElEscapeDetector {
private static boolean expressionContainsSafeDataNodes(ASTExpression expression) {
try {
for (VfTypedNode node : expression.getDataNodes().keySet()) {
DataType dataType = node.getDataType();
if (dataType == null || dataType.requiresEscaping) {
if (!typedNodeIsSafe(node)) {
return false;
}
}
@ -170,6 +372,11 @@ public final class ElEscapeDetector {
}
}
private static boolean typedNodeIsSafe(VfTypedNode node) {
DataType dataType = node.getDataType();
return dataType != null && !dataType.requiresEscaping;
}
public enum Escaping {
HTMLENCODE("HTMLENCODE"),
URLENCODE("URLENCODE"),

112
pmd-visualforce/src/test/resources/net/sourceforge/pmd/lang/vf/rule/security/xml/VfUnescapeEl.xml
View File

@ -437,22 +437,126 @@
</test-code>
<test-code>
<description>NOT method evaluates to safe boolean</description>
<description>IF() with literals in returns is safe</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
<apex:page>
<script>
if({!NOT(yes)}) { maskFormEls(); }
if({!NOT foo(yes)}) { maskFormEls(); }
console.log({!IF(SomeObj.Field=='1', '2', '3')});
</script>
</apex:page>
]]></code>
</test-code>
<test-code>
<description>JSON.parse method evaluates quoted EL to safe JSON</description>
<description>IF() with escaped XSS in returns is safe</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
<apex:page>
<script>
console.log({!IF(SomeObj.Field=='1', JSENCODE(SomeObj.Text__c), '2')});
console.log({!IF(SomeObj.Field=='1', 'foo', JSENCODE(SomeObj.Text__c))});
</script>
</apex:page>
]]></code>
</test-code>
<test-code>
<description>IF() with unescaped XSS in returns is unsafe</description>
<expected-problems>2</expected-problems>
<code><![CDATA[
<apex:page>
<script>
console.log({!IF(SomeObj.Field=='1', SomeObj.Text__c, '2')});
console.log({!IF(SomeObj.Field=='1', 'foo', SomeObj.Text__c)});
</script>
</apex:page>
]]></code>
</test-code>
<test-code>
<description>CASE() with XSS in conditions is safe</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
<apex:page>
<script>
console.log({!CASE(SomeObj.Name, SomeObj.text__c, 'res1', SomeObj.Text2__c, 'res2', 'else-res')});
</script>
</apex:page>
]]></code>
</test-code>
<test-code>
<description>CASE() with escaped XSS in results is safe</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
<apex:page>
<script>
console.log({!CASE(SomeObj.Name, 'val1', JSENCODE(SomeObj.Text__c), JSENCODE(SomeObj.Text2__c))});
</script>
</apex:page>
]]></code>
</test-code>
<test-code>
<description>CASE() with unescaped XSS in results is unsafe</description>
<expected-problems>1</expected-problems>
<code><![CDATA[
<apex:page>
<script>
console.log({!CASE(SomeObj.Name, 'val1', SomeObj.Text__c, SomeObj.Text2__c)});
</script>
</apex:page>
]]></code>
</test-code>
<test-code>
<description>LPAD() is not inherently safe</description>
<expected-problems>3</expected-problems>
<code><![CDATA[
<apex:page>
<script>
console.log({!LPAD(SomeObj.Text__c, 100)});
console.log({!LPAD(SomeObj.Text__c, 100, JSENCODE(SomeObj.OtherText__c))});
console.log({!LPAD(JSENCODE(SomeObj.Text__c), 100, SomeObj.OtherText__c)});
</script>
</apex:page>
]]></code>
</test-code>
<test-code>
<description>Escaped LPAD() call is fine</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
<apex:page>
<script>
console.log({!JSENCODE(LPAD(SomeObj.Text__c, 100))});
console.log({!LPAD(JSENCODE(SomeObj.Text__c), 100)});
console.log({!LPAD(JSENCODE(SomeObj.Text__c), 100, JSENCODE(SomeObj.OtherText__c))});
</script>
</apex:page>
]]></code>
</test-code>
<test-code>
<description>NOT, ISBLANK, BEGINS evaluate to safe booleans</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
<apex:page>
<script>
if({!NOT(yes)}) { maskFormEls(); }
if({!NOT foo(yes)}) { maskFormEls(); }
console.log({!ISBLANK(safeXss)});
console.log({!BEGINS(safeXss, 'foo')});
</script>
</apex:page>
]]></code>
</test-code>
<test-code>
<description>JSON.parse method evaluates quoted EL to unsafe XSS</description>
<expected-problems>3</expected-problems>
<code><![CDATA[
<apex:page>
<script>
var x = JSON.parse('{!yes}');