From dc4cdf696e22ce630295df84c601eb1903063f1a Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 21 Jan 2021 15:01:58 +0100 Subject: [PATCH] Fixups for #3005 - ElEscapeDetector is utility class now - Improved description and example of new rule --- .../rule/security/VfHtmlStyleTagXssRule.java | 24 +++++++------- .../vf/rule/security/VfUnescapeElRule.java | 33 ++++++++++--------- .../security/internal/ElEscapeDetector.java | 32 +++++++++--------- .../main/resources/category/vf/security.xml | 28 +++++++++++----- 4 files changed, 66 insertions(+), 51 deletions(-) diff --git a/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/VfHtmlStyleTagXssRule.java b/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/VfHtmlStyleTagXssRule.java index 14dd489e79..fed109fdcc 100644 --- a/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/VfHtmlStyleTagXssRule.java +++ b/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/VfHtmlStyleTagXssRule.java @@ -1,4 +1,4 @@ -/** +/* * BSD-style license; for more info see http://pmd.sourceforge.net/license.html */ @@ -23,8 +23,6 @@ public class VfHtmlStyleTagXssRule extends AbstractVfRule { private static final EnumSet ANY_ENCODE = EnumSet.of(ElEscapeDetector.Escaping.ANY); private static final Pattern URL_METHOD_PATTERN = Pattern.compile("url\\s*\\([^)]*$", Pattern.CASE_INSENSITIVE); - private final ElEscapeDetector escapeDetector = new ElEscapeDetector(); - public VfHtmlStyleTagXssRule() { addRuleChainVisit(ASTElExpression.class); } @@ -34,12 +32,13 @@ public class VfHtmlStyleTagXssRule extends AbstractVfRule { * placed inside an ASTContent, which in turn is placed inside * an ASTElement, where the element is not an inbuilt vf tag. * + * 
{@code
      * 
      *     
      *         
      *     
      * 
-     *
+     * }
*/ @Override public Object visit(ASTElExpression node, Object data) { @@ -81,7 +80,7 @@ public class VfHtmlStyleTagXssRule extends AbstractVfRule { ASTElement elementNode, Object data) { final String previousText = getPreviousText(contentNode, node); - final boolean isWithinSafeResource = escapeDetector.startsWithSafeResource(node); + final boolean isWithinSafeResource = ElEscapeDetector.startsWithSafeResource(node); // if El is inside a tag // and is not surrounded by a safe resource, check for violations @@ -104,7 +103,7 @@ public class VfHtmlStyleTagXssRule extends AbstractVfRule { private void verifyEncodingWithinUrl(ASTElExpression elExpressionNode, Object data) { // only allow URLENCODING or JSINHTMLENCODING - if (escapeDetector.doesElContainAnyUnescapedIdentifiers( + if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers( elExpressionNode, URLENCODE_JSINHTMLENCODE)) { addViolationWithMessage( @@ -116,7 +115,7 @@ public class VfHtmlStyleTagXssRule extends AbstractVfRule { } private void verifyEncodingWithoutUrl(ASTElExpression elExpressionNode, Object data) { - if (escapeDetector.doesElContainAnyUnescapedIdentifiers( + if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers( elExpressionNode, ANY_ENCODE)) { addViolationWithMessage( @@ -132,15 +131,18 @@ public class VfHtmlStyleTagXssRule extends AbstractVfRule { } /** - * Get text content within style tag that leads upto the ElExpression. + * Get text content within style tag that leads up to the ElExpression. * For example, in this snippet: - * + * </style> + * * - * getPreviousText(...) would return "\n div {\n background: url(" + * {@code getPreviousText(...)} would return "\n div {\n background: url(". * */ private String getPreviousText(ASTContent content, ASTElExpression elExpressionNode) { diff --git a/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/VfUnescapeElRule.java b/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/VfUnescapeElRule.java index 7a321abd1e..283d1918c9 100644 --- a/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/VfUnescapeElRule.java +++ b/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/VfUnescapeElRule.java @@ -49,8 +49,6 @@ public class VfUnescapeElRule extends AbstractVfRule { private static final EnumSet JSENCODE_JSINHTMLENCODE = EnumSet.of(ElEscapeDetector.Escaping.JSENCODE, ElEscapeDetector.Escaping.JSINHTMLENCODE); private static final EnumSet ANY_ENCODE = EnumSet.of(ElEscapeDetector.Escaping.ANY); - private final ElEscapeDetector escapeDetector = new ElEscapeDetector(); - @Override public Object visit(ASTHtmlScript node, Object data) { checkIfCorrectlyEscaped(node, data); @@ -88,15 +86,18 @@ public class VfUnescapeElRule extends AbstractVfRule { } if (quoted) { // check escaping too - if (!(jsonParse || escapeDetector.startsWithSafeResource(elExpression) || escapeDetector.containsSafeFields(elExpression))) { - if (escapeDetector.doesElContainAnyUnescapedIdentifiers(elExpression, + if (!(jsonParse + || ElEscapeDetector.startsWithSafeResource(elExpression) + || ElEscapeDetector.containsSafeFields(elExpression))) { + if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(elExpression, JSENCODE_JSINHTMLENCODE)) { addViolation(data, elExpression); } } } else { - if (!(escapeDetector.startsWithSafeResource(elExpression) || escapeDetector.containsSafeFields(elExpression))) { - final boolean hasUnscaped = escapeDetector.doesElContainAnyUnescapedIdentifiers(elExpression, + if (!(ElEscapeDetector.startsWithSafeResource(elExpression) + || ElEscapeDetector.containsSafeFields(elExpression))) { + final boolean hasUnscaped = ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(elExpression, JSENCODE_JSINHTMLENCODE); if (!(jsonParse && !hasUnscaped)) { addViolation(data, elExpression); @@ -181,11 +182,11 @@ public class VfUnescapeElRule extends AbstractVfRule { break; } - if (escapeDetector.startsWithSafeResource(el)) { + if (ElEscapeDetector.startsWithSafeResource(el)) { break; } - if (escapeDetector.doesElContainAnyUnescapedIdentifiers(el, ElEscapeDetector.Escaping.URLENCODE)) { + if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(el, ElEscapeDetector.Escaping.URLENCODE)) { isEL = true; toReport.add(el); } @@ -216,12 +217,11 @@ public class VfUnescapeElRule extends AbstractVfRule { if (ON_EVENT.matcher(name).matches()) { final List elsInVal = attr.findDescendantsOfType(ASTElExpression.class); for (ASTElExpression el : elsInVal) { - if (escapeDetector.startsWithSafeResource(el)) { + if (ElEscapeDetector.startsWithSafeResource(el)) { continue; } - if (escapeDetector.doesElContainAnyUnescapedIdentifiers(el, - ANY_ENCODE)) { + if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(el, ANY_ENCODE)) { isEL = true; toReport.add(el); } @@ -280,11 +280,12 @@ public class VfUnescapeElRule extends AbstractVfRule { final List elsInVal = attr.findDescendantsOfType(ASTElExpression.class); for (ASTElExpression el : elsInVal) { - if (escapeDetector.startsWithSafeResource(el)) { + if (ElEscapeDetector.startsWithSafeResource(el)) { continue; } - if (escapeDetector.doesElContainAnyUnescapedIdentifiers(el, ElEscapeDetector.Escaping.HTMLENCODE)) { + if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(el, + ElEscapeDetector.Escaping.HTMLENCODE)) { isEL = true; toReport.add(el); } @@ -347,11 +348,12 @@ public class VfUnescapeElRule extends AbstractVfRule { for (ASTAttribute attrib : innerAttributes) { final List elsInVal = attrib.findDescendantsOfType(ASTElExpression.class); for (final ASTElExpression el : elsInVal) { - if (escapeDetector.startsWithSafeResource(el)) { + if (ElEscapeDetector.startsWithSafeResource(el)) { continue; } - if (escapeDetector.doesElContainAnyUnescapedIdentifiers(el, ElEscapeDetector.Escaping.HTMLENCODE)) { + if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(el, + ElEscapeDetector.Escaping.HTMLENCODE)) { toReturn.add(el); } @@ -363,5 +365,4 @@ public class VfUnescapeElRule extends AbstractVfRule { return toReturn; } - } diff --git a/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/internal/ElEscapeDetector.java b/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/internal/ElEscapeDetector.java index b92a88f5f9..cea7bbd6c5 100644 --- a/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/internal/ElEscapeDetector.java +++ b/pmd-visualforce/src/main/java/net/sourceforge/pmd/lang/vf/rule/security/internal/ElEscapeDetector.java @@ -1,4 +1,4 @@ -/** +/* * BSD-style license; for more info see http://pmd.sourceforge.net/license.html */ @@ -18,7 +18,6 @@ import net.sourceforge.pmd.lang.vf.ast.ASTElExpression; import net.sourceforge.pmd.lang.vf.ast.ASTExpression; import net.sourceforge.pmd.lang.vf.ast.ASTIdentifier; import net.sourceforge.pmd.lang.vf.ast.ASTNegationExpression; -import net.sourceforge.pmd.lang.vf.ast.AbstractVFNode; import net.sourceforge.pmd.lang.vf.ast.VfNode; import net.sourceforge.pmd.lang.vf.ast.VfTypedNode; @@ -30,14 +29,18 @@ import net.sourceforge.pmd.lang.vf.ast.VfTypedNode; public final class ElEscapeDetector { private static final Set SAFE_EXPRESSIONS = new HashSet<>(Arrays.asList("id", "size", "caseNumber")); - private static final Set 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 EMPTY_ARG_SAFE_RESOURCE = new HashSet<>(Arrays.asList("$action", "$page", "$site", + private static final Set 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 EMPTY_ARG_SAFE_RESOURCE = new HashSet<>(Arrays.asList("$action", "$page", "$site", "$resource", "$label", "$objecttype", "$component", "$remoteaction", "$messagechannel")); - public boolean innerContainsSafeFields(final VfNode expression) { + private ElEscapeDetector() { + // utility class + } + + private static boolean innerContainsSafeFields(final VfNode expression) { for (VfNode child : expression.children()) { if (child instanceof ASTIdentifier && SAFE_EXPRESSIONS.contains(child.getImage().toLowerCase(Locale.ROOT))) { @@ -61,14 +64,13 @@ public final class ElEscapeDetector { return false; } - public boolean containsSafeFields(final AbstractVFNode expression) { + public static boolean containsSafeFields(final VfNode expression) { final ASTExpression ex = expression.getFirstChildOfType(ASTExpression.class); return ex != null && innerContainsSafeFields(ex); - } - public boolean startsWithSafeResource(final ASTElExpression el) { + public static boolean startsWithSafeResource(final ASTElExpression el) { final ASTExpression expression = el.getFirstChildOfType(ASTExpression.class); if (expression != null) { final ASTNegationExpression negation = expression.getFirstChildOfType(ASTNegationExpression.class); @@ -94,12 +96,11 @@ public final class ElEscapeDetector { return false; } - public boolean doesElContainAnyUnescapedIdentifiers(final ASTElExpression elExpression, Escaping escape) { + public static boolean doesElContainAnyUnescapedIdentifiers(final ASTElExpression elExpression, Escaping escape) { return doesElContainAnyUnescapedIdentifiers(elExpression, EnumSet.of(escape)); - } - public boolean doesElContainAnyUnescapedIdentifiers(final ASTElExpression elExpression, + public static boolean doesElContainAnyUnescapedIdentifiers(final ASTElExpression elExpression, EnumSet escapes) { if (elExpression == null) { return false; @@ -154,7 +155,7 @@ public final class ElEscapeDetector { * Return true if the type of all data nodes can be determined and none of them require escaping * @param expression */ - public boolean expressionContainsSafeDataNodes(ASTExpression expression) { + private static boolean expressionContainsSafeDataNodes(ASTExpression expression) { try { for (VfTypedNode node : expression.getDataNodes().keySet()) { DataType dataType = node.getDataType(); @@ -187,5 +188,4 @@ public final class ElEscapeDetector { return text; } } - } diff --git a/pmd-visualforce/src/main/resources/category/vf/security.xml b/pmd-visualforce/src/main/resources/category/vf/security.xml index 29b414402d..2f4408e929 100644 --- a/pmd-visualforce/src/main/resources/category/vf/security.xml +++ b/pmd-visualforce/src/main/resources/category/vf/security.xml @@ -28,22 +28,34 @@ Avoid calling VF action upon page load as the action becomes vulnerable to CSRF. - Use relevant encoding with EL in html tags +Checks for the correct encoding in `<style/>` tags in Visualforce pages. + +The rule is based on Salesforce Security's recommendation to prevent XSS in Visualforce as mentioned +on [Secure Coding Cross Site Scripting](https://developer.salesforce.com/docs/atlas.en-us.secure_coding_guide.meta/secure_coding_guide/secure_coding_cross_site_scripting.htm). + +In order to avoid cross site scripting, the relevant encoding must be used in HTML tags. The rule expects +`URLENCODING` or `JSINHTMLENCODING` for URL-based style values and any kind of encoding +(e.g. `HTMLENCODING`) for non-url style values. + +See also {% rule "VfUnescapeEl" %} to check escaping in other places on Visualforce pages. 3 - - + ]]>