From 16919a7fc14a843e2017b9198c1ee4c72c804d51 Mon Sep 17 00:00:00 2001 From: Loris S <91723853+loris-s-sonarsource@users.noreply.github.com> Date: Wed, 14 Sep 2022 19:49:53 +0200 Subject: [PATCH] Modify S2083&S6096(Education): Add Partial Path Traversal to pitfalls (#1243) --- .../S2083/common/fix/how-does-this-work.adoc | 3 +- .../pitfalls/partial-path-traversal.adoc | 8 ++++ rules/S2083/csharp/how-to-fix-it/dotnet.adoc | 36 ++++++++++++++- rules/S2083/java/how-to-fix-it/java-se.adoc | 34 ++++++++++++++ .../pitfalls/partial-path-traversal.adoc | 9 ++++ rules/S6096/csharp/how-to-fix-it/dotnet.adoc | 46 +++++++++++++++++-- rules/S6096/java/how-to-fix-it/java-se.adoc | 43 ++++++++++++++++- 7 files changed, 170 insertions(+), 9 deletions(-) create mode 100644 rules/S2083/common/pitfalls/partial-path-traversal.adoc create mode 100644 rules/S6096/common/pitfalls/partial-path-traversal.adoc diff --git a/rules/S2083/common/fix/how-does-this-work.adoc b/rules/S2083/common/fix/how-does-this-work.adoc index a91eca96d6..8f0580a397 100644 --- a/rules/S2083/common/fix/how-does-this-work.adoc +++ b/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. \ No newline at end of file +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/*. diff --git a/rules/S2083/common/pitfalls/partial-path-traversal.adoc b/rules/S2083/common/pitfalls/partial-path-traversal.adoc new file mode 100644 index 0000000000..5b7e44dc5f --- /dev/null +++ b/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. + diff --git a/rules/S2083/csharp/how-to-fix-it/dotnet.adoc b/rules/S2083/csharp/how-to-fix-it/dotnet.adoc index 6380230b01..b4f59e75d0 100644 --- a/rules/S2083/csharp/how-to-fix-it/dotnet.adoc +++ b/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.] diff --git a/rules/S2083/java/how-to-fix-it/java-se.adoc b/rules/S2083/java/how-to-fix-it/java-se.adoc index 80a401ee60..b6a24403ca 100644 --- a/rules/S2083/java/how-to-fix-it/java-se.adoc +++ b/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.] diff --git a/rules/S6096/common/pitfalls/partial-path-traversal.adoc b/rules/S6096/common/pitfalls/partial-path-traversal.adoc new file mode 100644 index 0000000000..2154e4b8f1 --- /dev/null +++ b/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. + diff --git a/rules/S6096/csharp/how-to-fix-it/dotnet.adoc b/rules/S6096/csharp/how-to-fix-it/dotnet.adoc index 3779d7055c..12803cde0a 100644 --- a/rules/S6096/csharp/how-to-fix-it/dotnet.adoc +++ b/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 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 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 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.] + diff --git a/rules/S6096/java/how-to-fix-it/java-se.adoc b/rules/S6096/java/how-to-fix-it/java-se.adoc index 1b6cf804da..66b44670d5 100644 --- a/rules/S6096/java/how-to-fix-it/java-se.adoc +++ b/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 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.]