Implement Path Traversal checks

Fixes #35
albfernandez · Nov 30, 2020 · 70f1bc8 · 70f1bc8
1 parent f44fb9f
commit 70f1bc8
Show file tree

Hide file tree

Showing 6 changed files with 91 additions and 59 deletions.
diff --git a/pom.xml b/pom.xml
@@ -1,4 +1,5 @@
-<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
 	xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
 	<modelVersion>4.0.0</modelVersion>
 	<groupId>com.github.albfernandez</groupId>
@@ -65,10 +66,10 @@
 		</dependency>
 
 		<dependency>
-		    <groupId>org.apache.commons</groupId>
-		    <artifactId>commons-text</artifactId>
-		    <version>1.2</version>
-		    <scope>test</scope>
+			<groupId>org.apache.commons</groupId>
+			<artifactId>commons-text</artifactId>
+			<version>1.2</version>
+			<scope>test</scope>
 		</dependency>
 
 		<dependency>
@@ -96,16 +97,23 @@
 			<scope>test</scope>
 		</dependency>
 		<dependency>
-    		<groupId>org.owasp.esapi</groupId>
-    		<artifactId>esapi</artifactId>
-    		<version>2.1.0.1</version>
-    		<scope>test</scope>
+			<groupId>org.owasp.esapi</groupId>
+			<artifactId>esapi</artifactId>
+			<version>2.1.0.1</version>
+			<scope>test</scope>
 		</dependency>
 		<dependency>
-    		<groupId>org.owasp.encoder</groupId>
-    		<artifactId>encoder</artifactId>
-    		<version>1.2.2</version>
-    		<scope>test</scope>
+			<groupId>org.owasp.encoder</groupId>
+			<artifactId>encoder</artifactId>
+			<version>1.2.2</version>
+			<scope>test</scope>
+		</dependency>
+
+		<dependency>
+			<groupId>commons-io</groupId>
+			<artifactId>commons-io</artifactId>
+			<version>2.8.0</version>
+			<scope>test</scope>
 		</dependency>
 
 	</dependencies>
@@ -199,26 +207,26 @@
 					<show>private</show>
 					<nohelp>true</nohelp>
 				</configuration>
-				
+
 			</plugin>
 			<plugin>
-                <groupId>org.apache.maven.plugins</groupId>
-                <artifactId>maven-gpg-plugin</artifactId>
-                <version>1.6</version>
-                <configuration>
-              		<keyname>${gpg.keyname}</keyname>
-              		<passphraseServerId>${gpg.keyname}</passphraseServerId>
-              	</configuration>
-                <executions>
-                    <execution>
-                        <id>sign-artifacts</id>
-                        <phase>verify</phase>
-                        <goals>
-                            <goal>sign</goal>
-                        </goals>
-                    </execution>
-                </executions>
-            </plugin>
+				<groupId>org.apache.maven.plugins</groupId>
+				<artifactId>maven-gpg-plugin</artifactId>
+				<version>1.6</version>
+				<configuration>
+					<keyname>${gpg.keyname}</keyname>
+					<passphraseServerId>${gpg.keyname}</passphraseServerId>
+				</configuration>
+				<executions>
+					<execution>
+						<id>sign-artifacts</id>
+						<phase>verify</phase>
+						<goals>
+							<goal>sign</goal>
+						</goals>
+					</execution>
+				</executions>
+			</plugin>
 			<plugin>
 				<groupId>org.sonatype.plugins</groupId>
 				<artifactId>nexus-staging-maven-plugin</artifactId>
@@ -235,10 +243,10 @@
 	<reporting>
 		<plugins>
 			<plugin>
-	            <groupId>org.apache.maven.plugins</groupId>
-	            <artifactId>maven-jxr-plugin</artifactId>
-	            <version>2.5</version>
-        	</plugin>
+				<groupId>org.apache.maven.plugins</groupId>
+				<artifactId>maven-jxr-plugin</artifactId>
+				<version>2.5</version>
+			</plugin>
 			<plugin>
 				<groupId>org.apache.maven.plugins</groupId>
 				<artifactId>maven-pmd-plugin</artifactId>

diff --git a/src/main/java/com/gdssecurity/pmd/rules/dfa/DfaSecurityRule.java b/src/main/java/com/gdssecurity/pmd/rules/dfa/DfaSecurityRule.java
@@ -864,11 +864,14 @@ private String getFullMethodName(Node node) {
 			if (astName != null && astName.getImage() != null) {
 				return astName.getImage();
 			}
-//			ASTAllocationExpression constructor = prefix.getFirstChildOfType(ASTAllocationExpression.class);
-//			if (constructor != null) {
-//				Class<?> type = constructor.getType();
-//				return type.getSimpleName();
-//			}
+			ASTAllocationExpression constructor = prefix.getFirstChildOfType(ASTAllocationExpression.class);
+			if (constructor != null) {
+				ASTArguments constructorArguments = constructor.getFirstChildOfType(ASTArguments.class);
+				if (constructorArguments != null && constructorArguments.getArgumentCount() > 0) {
+					Class<?> type = constructor.getType();
+					return type.getSimpleName();
+				}
+			}
 		}
 		if (prefix == null) {
 			ASTName astName = node.getFirstDescendantOfType(ASTName.class);

diff --git a/src/main/resources/rulesets/GDS/CWE/cwe-0022-path-traversal.xml b/src/main/resources/rulesets/GDS/CWE/cwe-0022-path-traversal.xml
@@ -1,22 +1,22 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<ruleset name="A10 - Unvalidated Redirects and Forwards"
+<ruleset name="CWE 22 Path Traversal"
 	language="java" xmlns="http://pmd.sf.net/ruleset/1.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
 	xsi:noNamespaceSchemaLocation="http://pmd.sf.net/ruleset_xml_schema.xsd"
 	xsi:schemaLocation="http://pmd.sf.net/ruleset/1.0.0 http://pmd.sf.net/ruleset_xml_schema.xsd">
 
 	<description>
    		This file is part of the GDS PMD Secure Coding Ruleset.
 
-   		This ruleset contains rules that are intended to detect Unvalidated Redirects and Forwards.
+   		This ruleset contains rules that are intended to detect Path Traversal.
    </description>
 
 	<rule class="com.gdssecurity.pmd.rules.dfa.DfaSecurityRule" name="PathTraversal"
-		message="Unvalidated Redirect" dfa="true" typeResolution="true"
+		message="Path Traversal" dfa="true" typeResolution="true" 
 		externalInfoUrl="https://cwe.mitre.org/data/definitions/22.html">
 		<description>
 	This rule utilizes the PMD data flow analysis (DFA) layer to help trace input from source to sink. PMD is unable to track data flow from source to sink across files. Therefore, the scope of analysis is limited to a single file and even within that a single method (i.e. PMD DFA is intraprocedural).
 
-	Specifically, it is configured with sinks that might suggest a potential URL Redirection vulnerability. Additional sinks should be added for increased coverage.	
+	Specifically, it is configured with sinks that might suggest a potential Path Traversal. Additional sinks should be added for increased coverage.	
       </description>
 
 		<priority>1</priority>
@@ -25,8 +25,10 @@
 			<property name="sinks">
 				<value>java.io.File.File</value>
 			</property>
+			<property name="sanitizers">
+				<value>org.apache.commons.io.FilenameUtils.getName|FilenameUtils.getName</value>
+			</property>
 		</properties>
-
 	</rule>
 
 </ruleset>
diff --git a/src/test/java/com/gdssecurity/pmd/CWE22PathTraversal.java b/src/test/java/com/gdssecurity/pmd/CWE22PathTraversal.java
@@ -11,9 +11,9 @@ public CWE22PathTraversal() {
 	}
 
 	@Test
-	@Ignore("Not yet implemented")
+//	@Ignore("Not yet implemented")
 	public void test1() throws Exception {
-		Assert.assertEquals(2, PMDRunner.run("src/test/java/resources/cwe22pathtraversal/PathTraversalExample.java",
+		Assert.assertEquals(4, PMDRunner.run("src/test/java/resources/cwe22pathtraversal/PathTraversalExample.java",
 				PMDRunner.RULEST_PATH_TRAVERSAL));
 	}
 }
diff --git a/src/test/java/resources/annotations/generator/AnnotationsGeneratorChainedCall.java b/src/test/java/resources/annotations/generator/AnnotationsGeneratorChainedCall.java
@@ -12,22 +12,25 @@ public class AnnotationsGeneratorChainedCall {
 
 
 
-	@HTMLGenerator
-	public String generateCode(String bad) {
-		return "ok" + StringEscapeUtils.escapeHtml4(bad);
-	}
 
-	public void generate(JspWriter out, String bad) throws IOException {
-		out.write(generateCode(bad));
-	}
+
+//	public void generate(JspWriter out, String bad) throws IOException {
+//		out.write(generateCode(bad));
+//	}
 
 	public void generate2(JspWriter out, String bad) throws IOException {
 		String goodCode = new AnnotationsGeneratorChainedCall().generateCode(bad);
 		out.write(goodCode);
 	}
-	public void generate3(JspWriter out, String bad) throws IOException {
-		String goodCode = new AnnotationsGeneratorExample().generateCode(bad);
-		out.write(goodCode);
-	}	
+//	public void generate3(JspWriter out, String bad) throws IOException {
+//		String goodCode = new AnnotationsGeneratorExample().generateCode(bad);
+//		out.write(goodCode);
+//	}	
+//	
+
+	@HTMLGenerator
+	public String generateCode(String bad) {
+		return "ok" + StringEscapeUtils.escapeHtml4(bad);
+	}
 
 }
diff --git a/src/test/java/resources/cwe22pathtraversal/PathTraversalExample.java b/src/test/java/resources/cwe22pathtraversal/PathTraversalExample.java
@@ -4,17 +4,33 @@
 
 import javax.servlet.http.HttpServletRequest;
 
+import org.apache.commons.io.FilenameUtils;
+
 public class PathTraversalExample {
 
 	public void bad(HttpServletRequest request) {
 		String fileName = request.getParameter("name");
 		new File("/base/path/" + fileName);
-//		f.getAbsolutePath();
 	}
 
 	public void bad2(String param) {
 		File f = new File(param);
 		f.getAbsolutePath();
 	}
 
+
+	public void bad3(HttpServletRequest request) {
+		String fileName = request.getParameter("name");
+		new File("/base/path", fileName);
+	}
+
+	public void bad4(String param) {
+		File f = new File("/base/path/", param);
+		f.getAbsolutePath();
+	}
+
+	public void ok(String param) {
+		File f = new File("/base/path", FilenameUtils.getName(param));
+		f.getAbsolutePath();
+	}
 }