Modify S2083&S6096(Education): Add Partial Path Traversal to pitfalls (#1243)

rules/S2083/common/fix/how-does-this-work.adoc

@@ -3,4 +3,5 @@ The universal way to prevent path injection is to validate paths constructed fro
 The validation should be done as follow:
 
 1. Resolve the canonical path of the file by using methods like `{canonicalization_function}`. This will resolve relative path or path components like `../` and removes any ambiguity regarding the file's location. 
-2. Check that the canonical path is within the directory where the file should be located.
+2. Check that the canonical path is within the directory where the file should be located.
+3. Ensure the target directory path ends with a forward slash to prevent partial path traversal, for example, */base/dirmalicious* starts with */base/dir* but does not start with */base/dir/*.

rules/S2083/common/pitfalls/partial-path-traversal.adoc

@@ -0,0 +1,8 @@
+==== Partial Path Traversal
+
+When validating untrusted paths by checking if they start with a trusted folder name,
+**ensure the validation string contains a path separator as the last character**. +
+A partial path traversal vulnerability can be unintentionally introduced into
+the application without a path separator as the last character of the
+validation strings.
+

rules/S2083/csharp/how-to-fix-it/dotnet.adoc

@@ -26,7 +26,7 @@ h| Compliant solution
 ----
 public class ExampleController : Controller
 {
-    private static string TargetDirectory;
+    private static string TargetDirectory = "/path/to/target/directory/";
 
     public void Example(string filename)
     {
@@ -45,3 +45,37 @@ public class ExampleController : Controller
 === How does this work?
 
 include::../../common/fix/how-does-this-work.adoc[]
+
+=== Pitfalls
+
+include::../../common/pitfalls/partial-path-traversal.adoc[]
+
+For example, the following code is vulnerable to partial path injection. Note
+that the string `TargetDirectory` does not end with a path separator:
+
+
+[source, csharp]
+----
+private static string TargetDirectory = "/Users/John";
+
+public void Example(string filename)
+{
+    string canonicalDestinationPath = Path.GetFullPath(filename);
+
+    if (canonicalDestinationPath.StartsWith(TargetDirectory, StringComparison.Ordinal))
+    {
+        System.IO.File.Delete(canonicalDestinationPath);
+    }
+}
+----
+
+This check can be bypassed because `"/Users/Johnny/file".startsWith("/Users/John")`
+returns `true`. Thus, for validation, `"/Users/John"` should actually be
+`"/Users/John/"`.
+
+**Warning**: Some functions remove the terminating path separator in their
+return value. +
+The validation code should be tested to ensure that it cannot be impacted by this
+issue.
+
+https://github.com/aws/aws-sdk-java/security/advisories/GHSA-c28r-hw5m-5gv3[Here is a real-life example of this vulnerability.]

rules/S2083/java/how-to-fix-it/java-se.adoc

@@ -50,3 +50,37 @@ public class ExampleController
 === How does this work?
 
 include::../../common/fix/how-does-this-work.adoc[]
+
+=== Pitfalls
+
+include::../../common/pitfalls/partial-path-traversal.adoc[]
+
+For example, the following code is vulnerable to partial path injection. Note
+that the string `targetDirectory` does not end with a path separator:
+
+
+[source, java]
+----
+static private String targetDirectory = "/Users/John";
+
+@GetMapping(value = "/endpoint")
+public void endpoint(@RequestParam("folder") fileName) throws IOException {
+
+    String canonicalizedFileName = fileName.getCanonicalPath();
+
+    if (!canonicalizedFileName .startsWith(targetDirectory)) {
+        throw new IOException("Entry is outside of the target directory");
+    }
+}
+----
+
+This check can be bypassed because `"/Users/Johnny".startsWith("/Users/John")`
+returns `true`. Thus, for validation, `"/Users/John"` should actually be
+`"/Users/John/"`.
+
+**Warning**: Some functions, such as `.getCanonicalPath`, remove the
+terminating path separator in their return value. +
+The validation code should be tested to ensure that it cannot be impacted by this
+issue.
+
+https://github.com/aws/aws-sdk-java/security/advisories/GHSA-c28r-hw5m-5gv3[Here is a real-life example of this vulnerability.]

rules/S6096/common/pitfalls/partial-path-traversal.adoc

@@ -0,0 +1,9 @@
+==== Partial Path Traversal
+
+When validating untrusted paths by checking if they start with a trusted folder name,
+**ensure the validation strings all contain a path separator as the last
+character**. +
+A partial path traversal vulnerability can be unintentionally introduced into
+the application without a path separator as the last character of the
+validation strings.
+

rules/S6096/csharp/how-to-fix-it/dotnet.adoc

@@ -13,12 +13,12 @@ h| Non-compliant code example
 ----
 public class ExampleController : Controller
 {
-    private static string targetDirectory = "/example/directory/";
+    private static string TargetDirectory = "/example/directory/";
 
     public void ExtractEntry(IEnumerator<ZipArchiveEntry> entriesEnumerator)
     {
         ZipArchiveEntry entry = entriesEnumerator.Current;
-        string destinationPath = Path.Combine(targetDirectory, entry.FullName);
+        string destinationPath = Path.Combine(TargetDirectory, entry.FullName);
 
         entry.ExtractToFile(destinationPath); // Noncompliant
     }
@@ -30,15 +30,15 @@ h| Compliant solution
 ----
 public class ExampleController : Controller
 {
-    private static string targetDirectory = "/example/directory/";
+    private static string TargetDirectory = "/example/directory/";
     
     public void ExtractEntry(IEnumerator<ZipArchiveEntry> entriesEnumerator)
     {
         ZipArchiveEntry entry = entriesEnumerator.Current;
-        string destinationPath = Path.Combine(targetDirectory, entry.FullName);
+        string destinationPath = Path.Combine(TargetDirectory, entry.FullName);
         string canonicalDestinationPath = Path.GetFullPath(destinationPath);
 
-        if (canonicalDestinationPath.StartsWith(targetDirectory, StringComparison.Ordinal))
+        if (canonicalDestinationPath.StartsWith(TargetDirectory, StringComparison.Ordinal))
         {
             entry.ExtractToFile(canonicalDestinationPath);
         }
@@ -50,3 +50,39 @@ public class ExampleController : Controller
 === How does this work?
 
 include::../../common/fix/how-does-this-work.adoc[]
+
+=== Pitfalls
+
+include::../../common/pitfalls/partial-path-traversal.adoc[]
+
+For example, the following code is vulnerable to partial path injection. Note
+that the string `TargetDirectory` does not end with a path separator:
+
+
+[source, csharp]
+----
+static private String TargetDirectory = "/Users/John";
+
+public void ExtractEntry(IEnumerator<ZipArchiveEntry> entriesEnumerator)
+{
+    ZipArchiveEntry entry = entriesEnumerator.Current;
+    string canonicalDestinationPath = Path.GetFullPath(TargetDirectory);
+
+    if (canonicalDestinationPath.StartsWith(TargetDirectory, StringComparison.Ordinal))
+    {
+        entry.ExtractToFile(canonicalDestinationPath);
+    }
+}
+----
+
+This check can be bypassed because `"/Users/Johnny".startsWith("/Users/John")`
+returns `true`. Thus, for validation, `"/Users/John"` should actually be
+`"/Users/John/"`.
+
+**Warning**: Some functions remove the terminating path separator in their
+return value. +
+The validation code should be tested to ensure that it cannot be impacted by this
+issue.
+
+https://github.com/aws/aws-sdk-java/security/advisories/GHSA-c28r-hw5m-5gv3[Here is a real-life example of this vulnerability.]
+

rules/S6096/java/how-to-fix-it/java-se.adoc

@@ -9,7 +9,7 @@ include::../../common/fix/code-rationale.adoc[]
 |===
 h| Non-compliant code example
 |
-[source,csharp]
+[source,java]
 ----
 public class Example {
 
@@ -29,7 +29,7 @@ public class Example {
 ----
 h| Compliant solution
 |
-[source,csharp]
+[source,java]
 ----
 public class Example {
 
@@ -56,3 +56,42 @@ public class Example {
 === How does this work?
 
 include::../../common/fix/how-does-this-work.adoc[]
+
+=== Pitfalls
+
+include::../../common/pitfalls/partial-path-traversal.adoc[]
+
+For example, the following code is vulnerable to partial path injection. Note
+that the string `targetDirectory` does not end with a path separator:
+
+
+[source, java]
+----
+static private String targetDirectory = "/Users/John";
+
+public void ExtractEntry(ZipFile zipFile) throws IOException {
+
+    Enumeration<? extends ZipEntry> entries = zipFile.entries();
+    ZipEntry entry = entries.nextElement();
+    InputStream inputStream = zipFile.getInputStream(entry);
+
+    File file = new File(entry.getName());
+
+    String canonicalDestinationPath = file.getCanonicalPath();
+
+    if (canonicalDestinationPath.startsWith(targetDirectory)) {
+        Files.copy(inputStream, file.toPath(), StandardCopyOption.REPLACE_EXISTING, LinkOption.NOFOLLOW_LINKS);
+    }
+}
+----
+
+This check can be bypassed because `"/Users/Johnny".startsWith("/Users/John")`
+returns `true`. Thus, for validation, `"/Users/John"` should actually be
+`"/Users/John/"`.
+
+**Warning**: Some functions, such as `.getCanonicalPath`, remove the
+terminating path separator in their return value. +
+The validation code should be tested to ensure that it cannot be impacted by this
+issue.
+
+https://github.com/aws/aws-sdk-java/security/advisories/GHSA-c28r-hw5m-5gv3[Here is a real-life example of this vulnerability.]