Skip to content

Commit

Permalink
Relax checks related to rule description content in ITs
Browse files Browse the repository at this point in the history 
Such checks are fragile since the description content can change often
  • Loading branch information
@damien-urruty-sonarsource
damien-urruty-sonarsource committed Dec 14, 2022
1 parent d54ed1c commit 9d59ecd
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 92 deletions.
76 changes: 2 additions & 74 deletions its/tests/src/test/java/its/ConnectedModeBackendTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public void returnDescriptionSectionsForTaintRules() throws ExecutionException,
assertThat(description)
.extracting("right.tabs", as(list(ActiveRuleDescriptionTabDto.class)))
.flatExtracting(ConnectedModeBackendTest::extractTabContent)
.containsOnly(
.contains(
"Why is this an issue?",
"<p>Path injections occur when an application uses untrusted data to construct a file path and access this file without validating its path first.</p>\n" +
"<p>A user with malicious intent would inject specially crafted values, such as <code>../</code>, to change the initial intended path. The resulting\n" +
Expand All @@ -165,79 +165,7 @@ public void returnDescriptionSectionsForTaintRules() throws ExecutionException,
"<p>The injected path component tampers with the location of a file the application is supposed to read and output. The vulnerability is exploited to\n" +
"leak the content of arbitrary files from the file system, including sensitive files like SSH private keys.</p>",
"How can I fix it?",
"<p>The following code is vulnerable to path injection as it is constructing a path using untrusted data. This path is then used to delete a file\n" +
"without being validated first. Therefore, it can be leveraged by an attacker to delete arbitrary files.</p>\n" +
"<h4>Non-compliant code example</h4>\n" +
"<pre data-diff-id=\"1\" data-diff-type=\"noncompliant\">\n" +
"@Controller\n" +
"public class ExampleController\n" +
"{\n" +
" static private String targetDirectory = \"/path/to/target/directory/\";\n" +
"\n" +
" @GetMapping(value = \"/delete\")\n" +
" public void delete(@RequestParam(\"filename\") String filename) throws IOException {\n" +
"\n" +
" File file = new File(targetDirectory + filename);\n" +
" file.delete();\n" +
" }\n" +
"}\n" +
"</pre>\n" +
"<h4>Compliant solution</h4>\n" +
"<pre data-diff-id=\"1\" data-diff-type=\"compliant\">\n" +
"@Controller\n" +
"public class ExampleController\n" +
"{\n" +
" static private String targetDirectory = \"/path/to/target/directory/\";\n" +
"\n" +
" @GetMapping(value = \"/delete\")\n" +
" public void delete(@RequestParam(\"filename\") String filename) throws IOException {\n" +
"\n" +
" File file = new File(targetDirectory + filename);\n" +
" String canonicalDestinationPath = file.getCanonicalPath();\n" +
"\n" +
" if (!canonicalDestinationPath.startsWith(targetDirectory)) {\n" +
" throw new IOException(\"Entry is outside of the target directory\");\n" +
" }\n" +
"\n" +
" file.delete();\n" +
" }\n" +
"}\n" +
"</pre>\n" +
"<h3>How does this work?</h3>\n" +
"<p>The universal way to prevent path injection is to validate paths constructed from untrusted data.</p>\n" +
"<p>The validation should be done as follow:</p>\n" +
"<ol>\n" +
" <li> Resolve the canonical path of the file by using methods like <code>java.io.File.getCanonicalPath</code>. This will resolve relative path or\n" +
" path components like <code>../</code> and removes any ambiguity regarding the file’s location. </li>\n" +
" <li> Check that the canonical path is within the directory where the file should be located. </li>\n" +
" <li> Ensure the target directory path ends with a forward slash to prevent partial path traversal, for example, <strong>/base/dirmalicious</strong>\n" +
" starts with <strong>/base/dir</strong> but does not start with <strong>/base/dir/</strong>. </li>\n" +
"</ol>\n" +
"<h3>Pitfalls</h3>\n" +
"<h4>Partial Path Traversal</h4>\n" +
"<p>When validating untrusted paths by checking if they start with a trusted folder name, <strong>ensure the validation string contains a path\n" +
"separator as the last character</strong>.<br> A partial path traversal vulnerability can be unintentionally introduced into the application without a\n" +
"path separator as the last character of the validation strings.</p>\n" +
"<p>For example, the following code is vulnerable to partial path injection. Note that the string <code>targetDirectory</code> does not end with a path\n" +
"separator:</p>\n" +
"<pre>\n" +
"static private String targetDirectory = \"/Users/John\";\n" +
"\n" +
"@GetMapping(value = \"/endpoint\")\n" +
"public void endpoint(@RequestParam(\"folder\") fileName) throws IOException {\n" +
"\n" +
" String canonicalizedFileName = fileName.getCanonicalPath();\n" +
"\n" +
" if (!canonicalizedFileName .startsWith(targetDirectory)) {\n" +
" throw new IOException(\"Entry is outside of the target directory\");\n" +
" }\n" +
"}\n" +
"</pre>\n" +
"<p>This check can be bypassed because <code>\"/Users/Johnny\".startsWith(\"/Users/John\")</code> returns <code>true</code>. Thus, for validation,\n" +
"<code>\"/Users/John\"</code> should actually be <code>\"/Users/John/\"</code>.</p>\n" +
"<p><strong>Warning</strong>: Some functions, such as <code>.getCanonicalPath</code>, remove the terminating path separator in their return value.<br>\n" +
"The validation code should be tested to ensure that it cannot be impacted by this issue.</p>\n" +
"<p><a href=\"https://github.com/aws/aws-sdk-java/security/advisories/GHSA-c28r-hw5m-5gv3\">Here is a real-life example of this vulnerability.</a></p>",
// actual description not checked because it changes frequently between versions
"java_se", "Java SE",
"More Info",
"<h3>Standards</h3>\n" +
Expand Down
22 changes: 4 additions & 18 deletions its/tests/src/test/java/its/ConnectedTaintVulnerabilitiesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ public void download_taint_vulnerabilities() {
}

@Test
public void updatesStorageTaintVulnerabilityEvents() throws InterruptedException {
public void updatesStorageTaintVulnerabilityEvents() {
assumeTrue(ORCHESTRATOR.getServer().version().isGreaterThanOrEquals(9, 6));

engine.updateProject(endpointParams(ORCHESTRATOR), sqHttpClient(), PROJECT_KEY_JAVA_TAINT, null);
Expand Down Expand Up @@ -218,26 +218,12 @@ public void updatesStorageTaintVulnerabilityEvents() throws InterruptedException
.flatExtracting("flows")
.flatExtracting("locations")
.extracting("message", "filePath", "textRange.startLine", "textRange.startLineOffset", "textRange.endLine", "textRange.endLineOffset", "textRange.hash")
.containsOnly(
// flow 1
.contains(
// flow 1 (don't assert intermediate locations as they change frequently between versions)
tuple("Sink: this invocation is not safe; a malicious value can be used as argument", "src/main/java/foo/DbHelper.java", 11, 35, 11, 64, "d123d615e9ea7cc7e78c784c768f2941"),
tuple("A malicious value can be assigned to variable ‘query’", "src/main/java/foo/DbHelper.java", 8, 4, 8, 95, "4562fd67c1d6bb2a7316aa9937b9e571"),
tuple("This concatenation can propagate malicious content to the newly created string", "src/main/java/foo/DbHelper.java", 8, 19, 8, 94, "83939dd74b004980ab22c04bc7b92374"),
tuple("This instruction can propagate malicious content", "src/main/java/foo/DbHelper.java", 7, 17, 7, 29, "66b4779468e4e855e187452d513b5fb6"),
tuple("This instruction can propagate malicious content", "src/main/java/foo/Endpoint.java", 11, 11, 11, 56, "902ee03726924c32da971ce91d64a16d"),
tuple("A malicious value can be assigned to variable ‘pass’", "src/main/java/foo/Endpoint.java", 9, 4, 9, 47, "1ec8a3fab79983f35c841547397cecd3"),
tuple("A malicious value can be assigned to variable ‘user’", "src/main/java/foo/Endpoint.java", 8, 4, 8, 47, "1408257f72430dde2f97a32065230e2f"),
tuple("This invocation can propagate malicious content to its return value", "src/main/java/foo/Endpoint.java", 8, 18, 8, 46, "2ef54227b849e317e7104dc550be8146"),
tuple("Source: a user can craft an HTTP request with malicious content", "src/main/java/foo/Endpoint.java", 9, 18, 9, 46, "a2b69949119440a24e900f15c0939c30"),
// flow 2
// flow 2 (don't assert intermediate locations as they change frequently between versions)
tuple("Sink: this invocation is not safe; a malicious value can be used as argument", "src/main/java/foo/DbHelper.java", 11, 35, 11, 64, "d123d615e9ea7cc7e78c784c768f2941"),
tuple("A malicious value can be assigned to variable ‘query’", "src/main/java/foo/DbHelper.java", 8, 4, 8, 95, "4562fd67c1d6bb2a7316aa9937b9e571"),
tuple("This concatenation can propagate malicious content to the newly created string", "src/main/java/foo/DbHelper.java", 8, 19, 8, 94, "83939dd74b004980ab22c04bc7b92374"),
tuple("This instruction can propagate malicious content", "src/main/java/foo/DbHelper.java", 7, 17, 7, 29, "66b4779468e4e855e187452d513b5fb6"),
tuple("This instruction can propagate malicious content", "src/main/java/foo/Endpoint.java", 11, 11, 11, 56, "902ee03726924c32da971ce91d64a16d"),
tuple("A malicious value can be assigned to variable ‘pass’", "src/main/java/foo/Endpoint.java", 9, 4, 9, 47, "1ec8a3fab79983f35c841547397cecd3"),
tuple("This invocation can propagate malicious content to its return value", "src/main/java/foo/Endpoint.java", 9, 18, 9, 46, "a2b69949119440a24e900f15c0939c30"),
tuple("A malicious value can be assigned to variable ‘user’", "src/main/java/foo/Endpoint.java", 8, 4, 8, 47, "1408257f72430dde2f97a32065230e2f"),
tuple("Source: a user can craft an HTTP request with malicious content", "src/main/java/foo/Endpoint.java", 8, 18, 8, 46, "2ef54227b849e317e7104dc550be8146"));

// check IssueChangedEvent is received
Expand Down

0 comments on commit 9d59ecd

Please sign in to comment.