Add documentation of the Partial Path Traversal vuln

Author: smehta23

java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp

@@ -3,43 +3,56 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-<p>Accessing paths controlled by users can allow an attacker to access unexpected resources. This 
-can result in sensitive information being revealed or deleted, or an attacker being able to influence
-behavior by modifying unexpected files.</p>
+<p> User inputted file paths can often pose security risks if a program does not correctly handle them. In particular, if a user 
+is meant to access files under a certain directory but does not enters a path under that directory, they can gain access to 
+(and potentially modify/delete) unexpected, possibly sensitive resources. </p>
 
-<p>Paths that are naively constructed from data controlled by a user may contain unexpected special characters,
-such as "..". Such a path may potentially point to any directory on the file system.</p>
+<p> Suppose a program is to only accept paths that point to files/folders within directory <code>DIR</code>. 
+To ensure that a user inputted path, say <code>SUBDIR</code>, is a subdirectory of <code>DIR</code>, the 
+program verifies that <code>DIR</code> is a prefix of <code>SUBDIR</code>.
+However, this check is not satisfactory: unless <code>DIR</code> is not slash-terminated, 
+<code>SUBDIR</code> may be allowed to also access siblings of <code>DIR</code> and not 
+just children of <code>DIR</code>, which is a security issue. </p>
 
 </overview>
 <recommendation>
 
-<p>Validate user input before using it to construct a file path. Ideally, follow these rules:</p>
-
-<ul>
-<li>Do not allow more than a single "." character.</li>
-<li>Do not allow directory separators such as "/" or "\" (depending on the file system).</li>
-<li>Do not rely on simply replacing problematic sequences such as "../". For example, after applying this filter to
-".../...//" the resulting string would still be "../".</li>
-<li>Ideally use a whitelist of known good patterns.</li>
-</ul>
+<p>If the user should only access items within a certain directory <code>DIR</code>, first ensure that <code>DIR</code> is slash-terminated,
+and then proceed (as normal) to verify that <code>DIR</code> is a prefix of the user-provided path, <code>SUBDIR</code>. Note, Java's <code>getCanonicalPath()</code>
+returns a <b>non</b>-slash-terminated path string, so a <code>"/"</code> must be added to <code>DIR</code> if that method is used. </p>
 
 </recommendation>
 <example>
 
-<p>In this example, a file name is read from a <code>java.net.Socket</code> and then used to access a file in the
-user's home directory and send it back over the socket. However, a malicious user could enter a file name which contains special 
-characters. For example, the string "../../etc/passwd" will result in the code reading the file located at 
-"/home/[user]/../../etc/passwd", which is the system's password file. This file would then be sent back to the user, 
-giving them access to all the system's passwords.</p>
+<p> 
+
+
+In this example, the <code>if</code> statement checks if <code>parent.getCanonicalPath()</code> 
+is a prefix of <code>dir.getCanonicalPath()</dir>. However, <code>parent.getCanonicalPath()</code> is 
+not slash-terminated. So, the user that supplies <code>dir</code> may be allowed to access siblings of <code>parent</code> 
+and not just children of <code>parent</code>, which is a security issue.
+
+</p>
+
+<sample src="PartialPathTraversalBad.java" />
+
+<p>
+
+In this example, the <code>if</code> statement checks if <code>parent.getCanonicalPath() + File.separator </code> 
+is a prefix of <code>dir.getCanonicalPath()</code>. Because <code>parent.getCanonicalPath() + File.separator</code> is 
+indeed slash-terminated, the user supplying <code>dir</code> can only access children of 
+<code>parent</code>, as desired.
+
+</p>
 
-<sample src="PartialPathTraversal.java" />
+<sample src="PartialPathTraversalGood.java" />
 
 </example>
 <references>
 
 <li>
 OWASP:
-<a href="https://owasp.org/www-community/attacks/Path_Traversal">Path Traversal</a>.
+<a href="https://owasp.org/www-community/attacks/Path_Traversal">Partial Path Traversal</a>.
 </li>
 
 </references>

java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java

@@ -0,0 +1,7 @@
+public class PartialPathTraversalBad {
+    public void esapiExample(File dir, File parent) throws IOException {
+        if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath())) {
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+}

java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java

@@ -0,0 +1,9 @@
+import java.io.File;
+
+public class PartialPathTraversalBad {
+    public void esapiExample(File dir, File parent) throws IOException {
+        if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath() + File.separator)) {
+            throw new IOException("Invalid directory: " + dir.getCanonicalPath());
+        }
+    }
+}