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

Fixups for #3005

Browse Source 
- ElEscapeDetector is utility class now
- Improved description and example of new rule
This commit is contained in:
Andreas Dangel
2021-01-21 15:01:58 +01:00
parent 15dd8783c7
commit dc4cdf696e
4 changed files with 66 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -1,4 +1,4 @@
/** /*
* BSD-style license; for more info see http://pmd.sourceforge.net/license.html * 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<ElEscapeDetector.Escaping> ANY_ENCODE = EnumSet.of(ElEscapeDetector.Escaping.ANY); private static final EnumSet<ElEscapeDetector.Escaping> ANY_ENCODE = EnumSet.of(ElEscapeDetector.Escaping.ANY);
private static final Pattern URL_METHOD_PATTERN = Pattern.compile("url\\s*\\([^)]*$", Pattern.CASE_INSENSITIVE); private static final Pattern URL_METHOD_PATTERN = Pattern.compile("url\\s*\\([^)]*$", Pattern.CASE_INSENSITIVE);
private final ElEscapeDetector escapeDetector = new ElEscapeDetector();
public VfHtmlStyleTagXssRule() { public VfHtmlStyleTagXssRule() {
addRuleChainVisit(ASTElExpression.class); addRuleChainVisit(ASTElExpression.class);
} }
@ -34,12 +32,13 @@ public class VfHtmlStyleTagXssRule extends AbstractVfRule {
* placed inside an ASTContent, which in turn is placed inside * placed inside an ASTContent, which in turn is placed inside
* an ASTElement, where the element is not an inbuilt vf tag. * an ASTElement, where the element is not an inbuilt vf tag.
* *
* <pre>{@code
* <ASTElement> * <ASTElement>
* <ASTContent> * <ASTContent>
* <ASTElExpression></ASTElExpression> * <ASTElExpression></ASTElExpression>
* </ASTContent> * </ASTContent>
* </ASTElement> * </ASTElement>
* * }</pre>
*/ */
@Override @Override
public Object visit(ASTElExpression node, Object data) { public Object visit(ASTElExpression node, Object data) {
@ -81,7 +80,7 @@ public class VfHtmlStyleTagXssRule extends AbstractVfRule {
ASTElement elementNode, ASTElement elementNode,
Object data) { Object data) {
final String previousText = getPreviousText(contentNode, node); final String previousText = getPreviousText(contentNode, node);
final boolean isWithinSafeResource = escapeDetector.startsWithSafeResource(node); final boolean isWithinSafeResource = ElEscapeDetector.startsWithSafeResource(node);
// if El is inside a <style></style> tag // if El is inside a <style></style> tag
// and is not surrounded by a safe resource, check for violations // 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) { private void verifyEncodingWithinUrl(ASTElExpression elExpressionNode, Object data) {
// only allow URLENCODING or JSINHTMLENCODING // only allow URLENCODING or JSINHTMLENCODING
if (escapeDetector.doesElContainAnyUnescapedIdentifiers( if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(
elExpressionNode, elExpressionNode,
URLENCODE_JSINHTMLENCODE)) { URLENCODE_JSINHTMLENCODE)) {
addViolationWithMessage( addViolationWithMessage(
@ -116,7 +115,7 @@ public class VfHtmlStyleTagXssRule extends AbstractVfRule {
} }
private void verifyEncodingWithoutUrl(ASTElExpression elExpressionNode, Object data) { private void verifyEncodingWithoutUrl(ASTElExpression elExpressionNode, Object data) {
if (escapeDetector.doesElContainAnyUnescapedIdentifiers( if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(
elExpressionNode, elExpressionNode,
ANY_ENCODE)) { ANY_ENCODE)) {
addViolationWithMessage( 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: * For example, in this snippet:
* <style> *
* <pre>
* &lt;style>
* div { * div {
* background: url('{!HTMLENCODE(XSSHere)}'); * background: url('{!HTMLENCODE(XSSHere)}');
* } * }
* </style> * &lt;/style>
* </pre>
* *
* getPreviousText(...) would return "\n div {\n background: url(" * {@code getPreviousText(...)} would return <code>"\n div {\n background: url("</code>.
* *
*/ */
private String getPreviousText(ASTContent content, ASTElExpression elExpressionNode) { private String getPreviousText(ASTContent content, ASTElExpression elExpressionNode) {

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

@ -49,8 +49,6 @@ public class VfUnescapeElRule extends AbstractVfRule {
private static final EnumSet<ElEscapeDetector.Escaping> JSENCODE_JSINHTMLENCODE = EnumSet.of(ElEscapeDetector.Escaping.JSENCODE, ElEscapeDetector.Escaping.JSINHTMLENCODE); private static final EnumSet<ElEscapeDetector.Escaping> JSENCODE_JSINHTMLENCODE = EnumSet.of(ElEscapeDetector.Escaping.JSENCODE, ElEscapeDetector.Escaping.JSINHTMLENCODE);
private static final EnumSet<ElEscapeDetector.Escaping> ANY_ENCODE = EnumSet.of(ElEscapeDetector.Escaping.ANY); private static final EnumSet<ElEscapeDetector.Escaping> ANY_ENCODE = EnumSet.of(ElEscapeDetector.Escaping.ANY);
private final ElEscapeDetector escapeDetector = new ElEscapeDetector();
@Override @Override
public Object visit(ASTHtmlScript node, Object data) { public Object visit(ASTHtmlScript node, Object data) {
checkIfCorrectlyEscaped(node, data); checkIfCorrectlyEscaped(node, data);
@ -88,15 +86,18 @@ public class VfUnescapeElRule extends AbstractVfRule {
} }
if (quoted) { if (quoted) {
// check escaping too // check escaping too
if (!(jsonParse || escapeDetector.startsWithSafeResource(elExpression) || escapeDetector.containsSafeFields(elExpression))) { if (!(jsonParse
if (escapeDetector.doesElContainAnyUnescapedIdentifiers(elExpression, || ElEscapeDetector.startsWithSafeResource(elExpression)
|| ElEscapeDetector.containsSafeFields(elExpression))) {
if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(elExpression,
JSENCODE_JSINHTMLENCODE)) { JSENCODE_JSINHTMLENCODE)) {
addViolation(data, elExpression); addViolation(data, elExpression);
} }
} }
} else { } else {
if (!(escapeDetector.startsWithSafeResource(elExpression) || escapeDetector.containsSafeFields(elExpression))) { if (!(ElEscapeDetector.startsWithSafeResource(elExpression)
final boolean hasUnscaped = escapeDetector.doesElContainAnyUnescapedIdentifiers(elExpression, || ElEscapeDetector.containsSafeFields(elExpression))) {
final boolean hasUnscaped = ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(elExpression,
JSENCODE_JSINHTMLENCODE); JSENCODE_JSINHTMLENCODE);
if (!(jsonParse && !hasUnscaped)) { if (!(jsonParse && !hasUnscaped)) {
addViolation(data, elExpression); addViolation(data, elExpression);
@ -181,11 +182,11 @@ public class VfUnescapeElRule extends AbstractVfRule {
break; break;
} }
if (escapeDetector.startsWithSafeResource(el)) { if (ElEscapeDetector.startsWithSafeResource(el)) {
break; break;
} }
if (escapeDetector.doesElContainAnyUnescapedIdentifiers(el, ElEscapeDetector.Escaping.URLENCODE)) { if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(el, ElEscapeDetector.Escaping.URLENCODE)) {
isEL = true; isEL = true;
toReport.add(el); toReport.add(el);
} }
@ -216,12 +217,11 @@ public class VfUnescapeElRule extends AbstractVfRule {
if (ON_EVENT.matcher(name).matches()) { if (ON_EVENT.matcher(name).matches()) {
final List<ASTElExpression> elsInVal = attr.findDescendantsOfType(ASTElExpression.class); final List<ASTElExpression> elsInVal = attr.findDescendantsOfType(ASTElExpression.class);
for (ASTElExpression el : elsInVal) { for (ASTElExpression el : elsInVal) {
if (escapeDetector.startsWithSafeResource(el)) { if (ElEscapeDetector.startsWithSafeResource(el)) {
continue; continue;
} }
if (escapeDetector.doesElContainAnyUnescapedIdentifiers(el, if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(el, ANY_ENCODE)) {
ANY_ENCODE)) {
isEL = true; isEL = true;
toReport.add(el); toReport.add(el);
} }
@ -280,11 +280,12 @@ public class VfUnescapeElRule extends AbstractVfRule {
final List<ASTElExpression> elsInVal = attr.findDescendantsOfType(ASTElExpression.class); final List<ASTElExpression> elsInVal = attr.findDescendantsOfType(ASTElExpression.class);
for (ASTElExpression el : elsInVal) { for (ASTElExpression el : elsInVal) {
if (escapeDetector.startsWithSafeResource(el)) { if (ElEscapeDetector.startsWithSafeResource(el)) {
continue; continue;
} }
if (escapeDetector.doesElContainAnyUnescapedIdentifiers(el, ElEscapeDetector.Escaping.HTMLENCODE)) { if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(el,
ElEscapeDetector.Escaping.HTMLENCODE)) {
isEL = true; isEL = true;
toReport.add(el); toReport.add(el);
} }
@ -347,11 +348,12 @@ public class VfUnescapeElRule extends AbstractVfRule {
for (ASTAttribute attrib : innerAttributes) { for (ASTAttribute attrib : innerAttributes) {
final List<ASTElExpression> elsInVal = attrib.findDescendantsOfType(ASTElExpression.class); final List<ASTElExpression> elsInVal = attrib.findDescendantsOfType(ASTElExpression.class);
for (final ASTElExpression el : elsInVal) { for (final ASTElExpression el : elsInVal) {
if (escapeDetector.startsWithSafeResource(el)) { if (ElEscapeDetector.startsWithSafeResource(el)) {
continue; continue;
} }
if (escapeDetector.doesElContainAnyUnescapedIdentifiers(el, ElEscapeDetector.Escaping.HTMLENCODE)) { if (ElEscapeDetector.doesElContainAnyUnescapedIdentifiers(el,
ElEscapeDetector.Escaping.HTMLENCODE)) {
toReturn.add(el); toReturn.add(el);
} }
@ -363,5 +365,4 @@ public class VfUnescapeElRule extends AbstractVfRule {
return toReturn; return toReturn;
} }
} }

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

@ -1,4 +1,4 @@
/** /*
* BSD-style license; for more info see http://pmd.sourceforge.net/license.html * 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.ASTExpression;
import net.sourceforge.pmd.lang.vf.ast.ASTIdentifier; import net.sourceforge.pmd.lang.vf.ast.ASTIdentifier;
import net.sourceforge.pmd.lang.vf.ast.ASTNegationExpression; 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.VfNode;
import net.sourceforge.pmd.lang.vf.ast.VfTypedNode; import net.sourceforge.pmd.lang.vf.ast.VfTypedNode;
@ -30,14 +29,18 @@ import net.sourceforge.pmd.lang.vf.ast.VfTypedNode;
public final class ElEscapeDetector { public final class ElEscapeDetector {
private static final Set<String> SAFE_EXPRESSIONS = new HashSet<>(Arrays.asList("id", "size", "caseNumber")); private static final Set<String> 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", private static final Set<String> NON_EMPTY_ARG_SAFE_RESOURCE = new HashSet<>(Arrays.asList("urlfor", "casesafeid",
"len", "getrecordids", "linkto", "sqrt", "round", "mod", "log", "ln", "exp", "abs", "floor", "ceiling", "begins", "contains", "len", "getrecordids", "linkto", "sqrt", "round", "mod", "log", "ln", "exp", "abs",
"nullvalue", "isnumber", "isnull", "isnew", "isblank", "isclone", "year", "month", "day", "datetimevalue", "floor", "ceiling", "nullvalue", "isnumber", "isnull", "isnew", "isblank", "isclone", "year", "month",
"datevalue", "date", "now", "today")); "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<String> EMPTY_ARG_SAFE_RESOURCE = new HashSet<>(Arrays.asList("$action", "$page", "$site",
"$resource", "$label", "$objecttype", "$component", "$remoteaction", "$messagechannel")); "$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()) { for (VfNode child : expression.children()) {
if (child instanceof ASTIdentifier && SAFE_EXPRESSIONS.contains(child.getImage().toLowerCase(Locale.ROOT))) { if (child instanceof ASTIdentifier && SAFE_EXPRESSIONS.contains(child.getImage().toLowerCase(Locale.ROOT))) {
@ -61,14 +64,13 @@ public final class ElEscapeDetector {
return false; return false;
} }
public boolean containsSafeFields(final AbstractVFNode expression) { public static boolean containsSafeFields(final VfNode expression) {
final ASTExpression ex = expression.getFirstChildOfType(ASTExpression.class); final ASTExpression ex = expression.getFirstChildOfType(ASTExpression.class);
return ex != null && innerContainsSafeFields(ex); 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); final ASTExpression expression = el.getFirstChildOfType(ASTExpression.class);
if (expression != null) { if (expression != null) {
final ASTNegationExpression negation = expression.getFirstChildOfType(ASTNegationExpression.class); final ASTNegationExpression negation = expression.getFirstChildOfType(ASTNegationExpression.class);
@ -94,12 +96,11 @@ public final class ElEscapeDetector {
return false; 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)); return doesElContainAnyUnescapedIdentifiers(elExpression, EnumSet.of(escape));
} }
public boolean doesElContainAnyUnescapedIdentifiers(final ASTElExpression elExpression, public static boolean doesElContainAnyUnescapedIdentifiers(final ASTElExpression elExpression,
EnumSet<Escaping> escapes) { EnumSet<Escaping> escapes) {
if (elExpression == null) { if (elExpression == null) {
return false; 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 * Return true if the type of all data nodes can be determined and none of them require escaping
* @param expression * @param expression
*/ */
public boolean expressionContainsSafeDataNodes(ASTExpression expression) { private static boolean expressionContainsSafeDataNodes(ASTExpression expression) {
try { try {
for (VfTypedNode node : expression.getDataNodes().keySet()) { for (VfTypedNode node : expression.getDataNodes().keySet()) {
DataType dataType = node.getDataType(); DataType dataType = node.getDataType();
@ -187,5 +188,4 @@ public final class ElEscapeDetector {
return text; return text;
} }
} }
} }

28
pmd-visualforce/src/main/resources/category/vf/security.xml
View File

@ -28,22 +28,34 @@ Avoid calling VF action upon page load as the action becomes vulnerable to CSRF.
<rule name="VfHtmlStyleTagXss" <rule name="VfHtmlStyleTagXss"
language="vf" language="vf"
since="6.29.0" since="6.31.0"
message="Use correct encoding for expressions within Style tag" message="Use correct encoding for expressions within Style tag"
class="net.sourceforge.pmd.lang.vf.rule.security.VfHtmlStyleTagXssRule" class="net.sourceforge.pmd.lang.vf.rule.security.VfHtmlStyleTagXssRule"
externalInfoUrl="${pmd.website.baseurl}/pmd_rules_vf_security.html#vfhtmlstyletagxss"> externalInfoUrl="${pmd.website.baseurl}/pmd_rules_vf_security.html#vfhtmlstyletagxss">
<description> <description>
Use relevant encoding with EL in html tags Checks for the correct encoding in `&lt;style/&gt;` 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.
</description> </description>
<priority>3</priority> <priority>3</priority>
<example> <example>
<![CDATA[ <![CDATA[
<apex:page> <apex:page>
<style> <style>
div { div {
background: url('{!XSSHere}'); background: url('{!XSSHere}'); // Potential XSS
} }
</style> div {
background: url('{!URLENCODE(XSSHere)}'); // correct encoding
}
</style>
</apex:page> </apex:page>
]]> ]]>
</example> </example>