phoedos/pmd
1
1
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge branch 'pr-175'

Browse Source 
Closes #175 (rebased onto pmd/5.5.x)
This commit is contained in:
Andreas Dangel
2017-01-04 20:26:43 +01:00
parent 2c714e1fbc 04cefcbe06
commit d12920eb72
4 changed files with 119 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexXSSFromURLParamRule.java
View File

@@ -35,7 +35,9 @@ public class ApexXSSFromURLParamRule extends AbstractApexRule {
private static final String[] INTEGER_VALUEOF = new String[] { "Integer", "valueOf" }; private static final String[] INTEGER_VALUEOF = new String[] { "Integer", "valueOf" };
private static final String[] ID_VALUEOF = new String[] { "ID", "valueOf" }; private static final String[] ID_VALUEOF = new String[] { "ID", "valueOf" };
private static final String[] DOUBLE_VALUEOF = new String[] { "Double", "valueOf" }; private static final String[] DOUBLE_VALUEOF = new String[] { "Double", "valueOf" };
private static final String[] BOOLEAN_VALUEOF = new String[] { "Boolean", "valueOf" };
private static final String[] STRING_ISEMPTY = new String[] { "String", "isEmpty" }; private static final String[] STRING_ISEMPTY = new String[] { "String", "isEmpty" };
private static final String[] STRING_ISBLANK = new String[] { "String", "isBlank" };
private final Set<String> urlParameterStrings = new HashSet<>(); private final Set<String> urlParameterStrings = new HashSet<>();
@@ -102,7 +104,9 @@ public class ApexXSSFromURLParamRule extends AbstractApexRule {
|| Helper.isMethodCallChain(methodNode, URL_ESCAPING) || Helper.isMethodCallChain(methodNode, URL_ESCAPING)
|| Helper.isMethodCallChain(methodNode, INTEGER_VALUEOF) || Helper.isMethodCallChain(methodNode, INTEGER_VALUEOF)
|| Helper.isMethodCallChain(methodNode, DOUBLE_VALUEOF) || Helper.isMethodCallChain(methodNode, DOUBLE_VALUEOF)
|| Helper.isMethodCallChain(methodNode, BOOLEAN_VALUEOF)
|| Helper.isMethodCallChain(methodNode, STRING_ISEMPTY) || Helper.isMethodCallChain(methodNode, STRING_ISEMPTY)
|| Helper.isMethodCallChain(methodNode, STRING_ISBLANK)
|| Helper.isMethodCallChain(methodNode, ID_VALUEOF); || Helper.isMethodCallChain(methodNode, ID_VALUEOF);
} }
@@ -150,15 +154,6 @@ public class ApexXSSFromURLParamRule extends AbstractApexRule {
final ASTVariableExpression variable = methodNode.getFirstChildOfType(ASTVariableExpression.class); final ASTVariableExpression variable = methodNode.getFirstChildOfType(ASTVariableExpression.class);
if (variable != null) { if (variable != null) {
// safe method
if (Helper.isMethodCallChain(methodNode, INTEGER_VALUEOF)
|| Helper.isMethodCallChain(methodNode, ID_VALUEOF)
|| Helper.isMethodCallChain(methodNode, DOUBLE_VALUEOF)
|| Helper.isMethodCallChain(methodNode, STRING_ISEMPTY)) {
return;
}
if (urlParameterStrings.contains(Helper.getFQVariableName(variable))) { if (urlParameterStrings.contains(Helper.getFQVariableName(variable))) {
if (!isEscapingMethod(methodNode)) { if (!isEscapingMethod(methodNode)) {
addViolation(data, variable); addViolation(data, variable);

134
pmd-apex/src/main/resources/rulesets/apex/security.xml
View File

@@ -12,7 +12,8 @@
class="net.sourceforge.pmd.lang.apex.rule.security.ApexSharingViolationsRule" class="net.sourceforge.pmd.lang.apex.rule.security.ApexSharingViolationsRule"
externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexSharingViolations"> externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexSharingViolations">
<description> <description>
Avoid Apex classes declared with no explicit sharing mode if DML methods are used. Detect classes declared without explicit sharing mode if DML methods are used. This
forces the developer to take access restrictions into account before modifying objects.
</description> </description>
<priority>3</priority> <priority>3</priority>
<example> <example>
@@ -29,13 +30,14 @@ public class without sharing Foo {
class="net.sourceforge.pmd.lang.apex.rule.security.ApexOpenRedirectRule" class="net.sourceforge.pmd.lang.apex.rule.security.ApexOpenRedirectRule"
externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexOpenRedirect"> externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexOpenRedirect">
<description> <description>
Avoid Apex controllers using PageReference to redirect to an unknown location Checks against redirects to user-controlled locations. This prevents attackers from
redirecting users to phishing sites.
</description> </description>
<priority>3</priority> <priority>3</priority>
<example> <example>
<![CDATA[ <![CDATA[
public class without sharing Foo { public class without sharing Foo {
String unsafeLocation = ApexPage.getCurrentPage().getParameters.get('url_param'); String unsafeLocation = ApexPage.getCurrentPage().getParameters.get('url_param');
PageReference page() { PageReference page() {
return new PageReference(unsafeLocation); return new PageReference(unsafeLocation);
} }
@@ -50,15 +52,16 @@ public class without sharing Foo {
class="net.sourceforge.pmd.lang.apex.rule.security.ApexInsecureEndpointRule" class="net.sourceforge.pmd.lang.apex.rule.security.ApexInsecureEndpointRule"
externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexInsecureEndpoint"> externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexInsecureEndpoint">
<description> <description>
Apex callouts should use encrypted communication channels Checks against accessing endpoints under plain **http**. You should always use
**https** for security.
</description> </description>
<priority>3</priority> <priority>3</priority>
<example> <example>
<![CDATA[ <![CDATA[
public class without sharing Foo { public class without sharing Foo {
void foo() { void foo() {
HttpRequest req = new HttpRequest(); HttpRequest req = new HttpRequest();
req.setEndpoint('http://localhost:com'); req.setEndpoint('http://localhost:com');
} }
} }
]]> ]]>
@@ -70,14 +73,15 @@ public class without sharing Foo {
class="net.sourceforge.pmd.lang.apex.rule.security.ApexXSSFromURLParamRule" class="net.sourceforge.pmd.lang.apex.rule.security.ApexXSSFromURLParamRule"
externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexXSSFromURLParam"> externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexXSSFromURLParam">
<description> <description>
Apex classes should escape/sanitize Strings obtained from URL parameters Makes sure that all values obtained from URL parameters are properly escaped / sanitized
to avoid XSS attacks.
</description> </description>
<priority>3</priority> <priority>3</priority>
<example> <example>
<![CDATA[ <![CDATA[
public class without sharing Foo { public class without sharing Foo {
String unescapedstring = ApexPage.getCurrentPage().getParameters.get('url_param'); String unescapedstring = ApexPage.getCurrentPage().getParameters.get('url_param');
String usedLater = unescapedstring; String usedLater = unescapedstring;
} }
]]> ]]>
</example> </example>
@@ -89,13 +93,15 @@ public class without sharing Foo {
class="net.sourceforge.pmd.lang.apex.rule.security.ApexXSSFromEscapeFalseRule" class="net.sourceforge.pmd.lang.apex.rule.security.ApexXSSFromEscapeFalseRule"
externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexXSSFromEscapeFalse"> externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexXSSFromEscapeFalse">
<description> <description>
Apex classes should escape Strings in error messages Reports on calls to `addError` with disabled escaping. The message passed to `addError`
will be displayed directly to the user in the UI, making it prime ground for XSS
attacks if unescaped.
</description> </description>
<priority>3</priority> <priority>3</priority>
<example> <example>
<![CDATA[ <![CDATA[
public class without sharing Foo { public class without sharing Foo {
Trigger.new[0].addError(vulnerableHTMLGoesHere, false); Trigger.new[0].addError(vulnerableHTMLGoesHere, false);
} }
]]> ]]>
</example> </example>
@@ -106,16 +112,17 @@ public class without sharing Foo {
class="net.sourceforge.pmd.lang.apex.rule.security.ApexBadCryptoRule" class="net.sourceforge.pmd.lang.apex.rule.security.ApexBadCryptoRule"
externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexBadCrypto"> externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexBadCrypto">
<description> <description>
Apex classes should use random IV/key The rule makes sure you are using randomly generated IVs and keys for `Crypto` calls.
Hard-wiring these values greatly compromises the security of encrypted data.
</description> </description>
<priority>3</priority> <priority>3</priority>
<example> <example>
<![CDATA[ <![CDATA[
public class without sharing Foo { public class without sharing Foo {
Blob hardCodedIV = Blob.valueOf('Example of IV123'); Blob hardCodedIV = Blob.valueOf('Hardcoded IV 123');
Blob key = Crypto.generateAesKey(128); Blob hardCodedKey = Blob.valueOf('0000000000000000');
Blob data = Blob.valueOf('Data to be encrypted'); Blob data = Blob.valueOf('Data to be encrypted');
Blob encrypted = Crypto.encrypt('AES128', key, hardCodedIV, data); Blob encrypted = Crypto.encrypt('AES128', hardCodedKey, hardCodedIV, data);
} }
]]> ]]>
</example> </example>
@@ -127,19 +134,20 @@ public class without sharing Foo {
class="net.sourceforge.pmd.lang.apex.rule.security.ApexCSRFRule" class="net.sourceforge.pmd.lang.apex.rule.security.ApexCSRFRule"
externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexCSRF"> externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexCSRF">
<description> <description>
Avoid DML actions in Apex class constructor/init method without CSRF protection Check to avoid making DML operations in Apex class constructor/init method. This prevents
modification of the database just by accessing a page.
</description> </description>
<priority>3</priority> <priority>3</priority>
<example> <example>
<![CDATA[ <![CDATA[
public class Foo { public class Foo {
public init() { public init() {
insert data; insert data;
} }
public Foo() { public Foo() {
insert data; insert data;
} }
} }
]]> ]]>
</example> </example>
@@ -150,15 +158,15 @@ public class Foo {
class="net.sourceforge.pmd.lang.apex.rule.security.ApexSOQLInjectionRule" class="net.sourceforge.pmd.lang.apex.rule.security.ApexSOQLInjectionRule"
externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexSOQLInjection"> externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexSOQLInjection">
<description> <description>
Avoid merging untrusted/unescaped variables in DML operations Detects the usage of untrusted / unescaped variables in DML queries.
</description> </description>
<priority>3</priority> <priority>3</priority>
<example> <example>
<![CDATA[ <![CDATA[
public class Foo { public class Foo {
public void test1(String t1) { public void test1(String t1) {
Database.query('SELECT Id FROM Account' + t1); Database.query('SELECT Id FROM Account' + t1);
} }
} }
]]> ]]>
</example> </example>
@@ -169,21 +177,26 @@ public class Foo {
class="net.sourceforge.pmd.lang.apex.rule.security.ApexCRUDViolationRule" class="net.sourceforge.pmd.lang.apex.rule.security.ApexCRUDViolationRule"
externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexCRUDViolationRule"> externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexCRUDViolationRule">
<description> <description>
Validate CRUD permission before SOQL/SOSL/DML operation The rule validates you are checking for access permissions before a SOQL/SOSL/DML operation.
Since Apex runs in system mode not having proper permissions checks results in escalation of
privilege and may produce runtime errors. This check forces you to handle such scenarios.
</description> </description>
<priority>3</priority> <priority>3</priority>
<example> <example>
<![CDATA[ <![CDATA[
public class Foo { public class Foo {
public Contact foo(String status, String ID) { public Contact foo(String status, String ID) {
Contact c = [SELECT Status__c FROM Contact WHERE Id=:ID]; Contact c = [SELECT Status__c FROM Contact WHERE Id=:ID];
if (!Schema.sObjectType.Contact.fields.Name.isUpdateable()){
return null; // Make sure we can update the database before even trying
} if (!Schema.sObjectType.Contact.fields.Name.isUpdateable()) {
c.Status__c = status; return null;
update c; }
return c;
} c.Status__c = status;
update c;
return c;
}
} }
]]> ]]>
</example> </example>
@@ -193,16 +206,23 @@ public class Foo {
message="Calling potentially dangerous method" message="Calling potentially dangerous method"
class="net.sourceforge.pmd.lang.apex.rule.security.ApexDangerousMethodsRule" class="net.sourceforge.pmd.lang.apex.rule.security.ApexDangerousMethodsRule"
externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexDangerousMethodsRule"> externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexDangerousMethodsRule">
<description> <description><![CDATA[
Calling potentially dangerous method Checks against calling dangerous methods.
</description>
For the time being, it reports:
* Against `FinancialForce`'s `Configuration.disableTriggerCRUDSecurity()`. Disabling CRUD security
opens the door to several attacks and requires manual validation, which is unreliable.
* Calling `System.debug` passing sensitive data as parameter, which could lead to exposure
of private data.
]]></description>
<priority>3</priority> <priority>3</priority>
<example> <example>
<![CDATA[ <![CDATA[
public class Foo { public class Foo {
public Foo() { public Foo() {
Configuration.disableTriggerCRUDSecurity(); Configuration.disableTriggerCRUDSecurity();
} }
} }
]]> ]]>
</example> </example>
@@ -212,18 +232,30 @@ public class Foo {
message="Suggest named credentials for authentication" message="Suggest named credentials for authentication"
class="net.sourceforge.pmd.lang.apex.rule.security.ApexSuggestUsingNamedCredRule" class="net.sourceforge.pmd.lang.apex.rule.security.ApexSuggestUsingNamedCredRule"
externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexSuggestUsingNamedCredRule"> externalInfoUrl="${pmd.website.baseurl}/rules/apex/security.html#ApexSuggestUsingNamedCredRule">
<description> <description><![CDATA[
Consider using named credentials for authenticated callouts Detects hardcoded credentials used in requests to an endpoint.
</description>
You should refrain from hardcoding credentials:
* They are hard to mantain by being mixed in application code
* Particularly hard to update them when used from different classes
* Granting a developer access to the codebase means granting knowledge
of credentials, keeping a two-level access is not possible.
* Using different credentials for different environments is troublesome
and error-prone.
Instead, you should use *Named Credentials* and a callout endpoint.
For more information, you can check [this](https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/apex_callouts_named_credentials.htm)
]]></description>
<priority>3</priority> <priority>3</priority>
<example> <example>
<![CDATA[ <![CDATA[
public class Foo { public class Foo {
public void foo(String username, String password) { public void foo(String username, String password) {
Blob headerValue = Blob.valueOf(username + ':' + password); Blob headerValue = Blob.valueOf(username + ':' + password);
String authorizationHeader = 'BASIC ' + EncodingUtil.base64Encode(headerValue); String authorizationHeader = 'BASIC ' + EncodingUtil.base64Encode(headerValue);
req.setHeader('Authorization', authorizationHeader); req.setHeader('Authorization', authorizationHeader);
} }
} }
]]> ]]>
</example> </example>

29
pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexXSSFromURLParam.xml
View File

@@ -349,5 +349,34 @@ public class Foo {
]]></code> ]]></code>
</test-code> </test-code>
<test-code>
<description>Casting applied to a variable
</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
public void test1() {
String bas;
bas = ApexPages.currentPage().getParameters().get('foo');
ID baz = ID.valueOf(bas);
Boolean jj = Boolean.valueOf(bas);
}
}
]]></code>
</test-code>
<test-code>
<description>Casting applied to a method call
</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
public void test1() {
ID baz = ID.valueOf(ApexPages.currentPage().getParameters().get('foo'));
Boolean jj = Boolean.valueOf(ApexPages.currentPage().getParameters().get('foo'));
}
}
]]></code>
</test-code>
</test-data> </test-data>

5
src/site/markdown/overview/changelog.md
View File

@@ -180,7 +180,7 @@ For instance, it would report on:
``` ```
public class Foo { public class Foo {
public void test1(String t1) { public void test1(String t1) {
Database.query('SELECT Id FROM Account' + t1); Database.query('SELECT Id FROM Account' + t1);
} }
} }
``` ```
@@ -260,7 +260,8 @@ to avoid XSS attacks.
* [#160](https://github.com/pmd/pmd/pull/160): \[apex] Flagging of dangerous method call * [#160](https://github.com/pmd/pmd/pull/160): \[apex] Flagging of dangerous method call
* [#163](https://github.com/pmd/pmd/pull/163): \[apex] Flagging of System.debug * [#163](https://github.com/pmd/pmd/pull/163): \[apex] Flagging of System.debug
* [#165](https://github.com/pmd/pmd/pull/165): \[apex] Improving open redirect rule to avoid test classes/methods * [#165](https://github.com/pmd/pmd/pull/165): \[apex] Improving open redirect rule to avoid test classes/methods
* [#168](https://github.com/pmd/pmd/pull/167): \[apex] GC and thread safety changes * [#167](https://github.com/pmd/pmd/pull/167): \[apex] GC and thread safety changes
* [#169](https://github.com/pmd/pmd/pull/169): \[apex] Improving detection for DML with inline new object * [#169](https://github.com/pmd/pmd/pull/169): \[apex] Improving detection for DML with inline new object
* [#172](https://github.com/pmd/pmd/pull/172): \[apex] Bug fix, detects both Apex fields and class members * [#172](https://github.com/pmd/pmd/pull/172): \[apex] Bug fix, detects both Apex fields and class members
* [#175](https://github.com/pmd/pmd/pull/175): \[apex] ApexXSSFromURLParam: Adding missing casting methods