Skip to content

Commit 76cecc1

Browse files
smehta23smehta23
committed
Fix documentation
1 parent 09ec379 commit 76cecc1

File tree

6 files changed

+97
-54
lines changed

6 files changed

+97
-54
lines changed

‎java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,51 +1,56 @@
1+
/** Provides classes to reason about partial path traversal vulnerabilities. */
2+
13
import java
24
private import semmle.code.java.dataflow.DataFlow
35
private import semmle.code.java.environment.SystemProperty
46

5-
class MethodStringStartsWith extends Method {
7+
private class MethodStringStartsWith extends Method {
68
MethodStringStartsWith() {
79
this.getDeclaringType() instanceof TypeString and
810
this.hasName("startsWith")
911
}
1012
}
1113

12-
class MethodFileGetCanonicalPath extends Method {
14+
private class MethodFileGetCanonicalPath extends Method {
1315
MethodFileGetCanonicalPath() {
1416
this.getDeclaringType() instanceof TypeFile and
1517
this.hasName("getCanonicalPath")
1618
}
1719
}
1820

19-
class MethodAccessFileGetCanonicalPath extends MethodAccess {
21+
private class MethodAccessFileGetCanonicalPath extends MethodAccess {
2022
MethodAccessFileGetCanonicalPath() { this.getMethod() instanceof MethodFileGetCanonicalPath }
2123
}
2224

23-
abstract class FileSeparatorExpr extends Expr { }
25+
private abstract class FileSeparatorExpr extends Expr { }
2426

25-
class SystemPropFileSeparatorExpr extends FileSeparatorExpr {
27+
private class SystemPropFileSeparatorExpr extends FileSeparatorExpr {
2628
SystemPropFileSeparatorExpr() { this = getSystemProperty("file.separator") }
2729
}
2830

29-
class StringLiteralFileSeparatorExpr extends FileSeparatorExpr, StringLiteral {
31+
private class StringLiteralFileSeparatorExpr extends FileSeparatorExpr, StringLiteral {
3032
StringLiteralFileSeparatorExpr() {
3133
this.getValue().matches("%/") or this.getValue().matches("%\\")
3234
}
3335
}
3436

35-
class CharacterLiteralFileSeparatorExpr extends FileSeparatorExpr, CharacterLiteral {
37+
private class CharacterLiteralFileSeparatorExpr extends FileSeparatorExpr, CharacterLiteral {
3638
CharacterLiteralFileSeparatorExpr() { this.getValue() = "/" or this.getValue() = "\\" }
3739
}
3840

39-
class FileSeparatorAppend extends AddExpr {
41+
private class FileSeparatorAppend extends AddExpr {
4042
FileSeparatorAppend() { this.getRightOperand() instanceof FileSeparatorExpr }
4143
}
4244

43-
predicate isSafe(Expr expr) {
45+
private predicate isSafe(Expr expr) {
4446
DataFlow::localExprFlow(any(Expr e |
4547
e instanceof FileSeparatorAppend or e instanceof FileSeparatorExpr
4648
), expr)
4749
}
4850

51+
/**
52+
* A method access that returns a boolean that incorrectly guards against Partial Path Traversal.
53+
*/
4954
class PartialPathTraversalMethodAccess extends MethodAccess {
5055
PartialPathTraversalMethodAccess() {
5156
this.getMethod() instanceof MethodStringStartsWith and

‎java/ql/lib/semmle/code/java/security/PartialPathTraversalQuery.qll

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,19 @@
1+
/** Provides taint tracking configurations to be used in partial path traversal queries. */
2+
13
import java
24
import semmle.code.java.security.PartialPathTraversal
35
import semmle.code.java.dataflow.DataFlow
46
import semmle.code.java.dataflow.ExternalFlow
57
import semmle.code.java.dataflow.TaintTracking
68
import semmle.code.java.dataflow.FlowSources
79

10+
/**
11+
* A taint-tracking configuration for unsafe user input
12+
* that is used to validate against path traversal, but is insufficient
13+
* and remains vulnerable to Partial Path Traversal.
14+
*/
815
class PartialPathTraversalFromRemoteConfig extends TaintTracking::Configuration {
9-
PartialPathTraversalFromRemoteConfig() { this = "PartialPathTraversalFromRemoteConfig" }
16+
PartialPathTraversalFromRemoteConfig() { this = "PartialPathTraversalFromRemoteConfig" }
1017

1118
override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
1219

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

Lines changed: 3 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -3,51 +3,10 @@
33
"qhelp.dtd">
44
<qhelp>
55
<overview>
6-
<p>A common way to check that a user-supplied path <code>SUBDIR</code> falls inside a directory <code>DIR</code>
7-
is to use <code>getCanonicalPath()</code> to remove any path-traversal elements and then check that <code>DIR</code>
8-
is a prefix. However, if <code>DIR</code> is not slash-terminated, this can unexpectedly allow accessing siblings of <code>DIR</code>.</p>
6+
<include src="PartialPathTraversalOverview.inc.qhelp">
7+
<p>See also <code>java/partial-path-traversal-from-remote</code>, which is similar to this query but only flags instances with evidence of remote exploitability</p>
98
</overview>
10-
<recommendation>
119

12-
<p>If the user should only access items within a certain directory <code>DIR</code>, ensure that <code>DIR</code> is slash-terminated
13-
before checking that <code>DIR</code> is a prefix of the user-provided path, <code>SUBDIR</code>. Note, Java's <code>getCanonicalPath()</code>
14-
returns a <b>non</b>-slash-terminated path string, so a slash must be added to <code>DIR</code> if that method is used.</p>
10+
<include src="PartialPathTraversalRemainder.inc.qhelp">
1511

16-
</recommendation>
17-
<example>
18-
19-
<p>
20-
21-
22-
In this example, the <code>if</code> statement checks if <code>parent.getCanonicalPath()</code>
23-
is a prefix of <code>dir.getCanonicalPath()</code>. However, <code>parent.getCanonicalPath()</code> is
24-
not slash-terminated. So, the user that supplies <code>dir</code> may be allowed to access siblings of <code>parent</code>
25-
and not just children of <code>parent</code>, which is a security issue.
26-
27-
</p>
28-
29-
<sample src="PartialPathTraversalBad.java" />
30-
31-
<p>
32-
33-
In this example, the <code>if</code> statement checks if <code>parent.getCanonicalPath() + File.separator </code>
34-
is a prefix of <code>dir.getCanonicalPath()</code>. Because <code>parent.getCanonicalPath().toPath()</code> is
35-
indeed slash-terminated, the user supplying <code>dir</code> can only access children of
36-
<code>parent</code>, as desired.
37-
38-
</p>
39-
40-
<sample src="PartialPathTraversalGood.java" />
41-
42-
</example>
43-
<references>
44-
45-
<li>
46-
OWASP:
47-
<a href="https://owasp.org/www-community/attacks/Path_Traversal">Partial Path Traversal</a>.
48-
CVE-2022-23457:
49-
<a href="https://github.com/ESAPI/esapi-java-legacy/blob/develop/documentation/GHSL-2022-008_The_OWASP_Enterprise_Security_API.md"> ESAPI Vulnerability Report</a>
50-
</li>
51-
52-
</references>
5312
</qhelp>

‎java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<include src="PartialPathTraversalOverview.inc.qhelp">
7+
</overview>
8+
9+
<include src="PartialPathTraversalRemainder.inc.qhelp">
10+
11+
</qhelp>

‎java/ql/src/Security/CWE/CWE-023/PartialPathTraversalOverview.inc.qhelp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<p>A common way to check that a user-supplied path <code>SUBDIR</code> falls inside a directory <code>DIR</code>
7+
is to use <code>getCanonicalPath()</code> to remove any path-traversal elements and then check that <code>DIR</code>
8+
is a prefix. However, if <code>DIR</code> is not slash-terminated, this can unexpectedly allow accessing siblings of <code>DIR</code>.</p>
9+
10+
</qhelp>

‎java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<recommendation>
7+
8+
<p>If the user should only access items within a certain directory <code>DIR</code>, ensure that <code>DIR</code> is slash-terminated
9+
before checking that <code>DIR</code> is a prefix of the user-provided path, <code>SUBDIR</code>. Note, Java's <code>getCanonicalPath()</code>
10+
returns a <b>non</b>-slash-terminated path string, so a slash must be added to <code>DIR</code> if that method is used.</p>
11+
12+
</recommendation>
13+
<example>
14+
15+
<p>
16+
17+
18+
In this example, the <code>if</code> statement checks if <code>parent.getCanonicalPath()</code>
19+
is a prefix of <code>dir.getCanonicalPath()</code>. However, <code>parent.getCanonicalPath()</code> is
20+
not slash-terminated. So, the user that supplies <code>dir</code> may be allowed to access siblings of <code>parent</code>
21+
and not just children of <code>parent</code>, which is a security issue.
22+
23+
</p>
24+
25+
<sample src="PartialPathTraversalBad.java" />
26+
27+
<p>
28+
29+
In this example, the <code>if</code> statement checks if <code>parent.getCanonicalPath() + File.separator </code>
30+
is a prefix of <code>dir.getCanonicalPath()</code>. Because <code>parent.getCanonicalPath().toPath()</code> is
31+
indeed slash-terminated, the user supplying <code>dir</code> can only access children of
32+
<code>parent</code>, as desired.
33+
34+
</p>
35+
36+
<sample src="PartialPathTraversalGood.java" />
37+
38+
</example>
39+
<references>
40+
41+
<li>
42+
OWASP:
43+
<a href="https://owasp.org/www-community/attacks/Path_Traversal">Partial Path Traversal</a>.
44+
CVE-2022-23457:
45+
<a href="https://github.com/ESAPI/esapi-java-legacy/blob/develop/documentation/GHSL-2022-008_The_OWASP_Enterprise_Security_API.md"> ESAPI Vulnerability Report</a>
46+
</li>
47+
48+
</references>
49+
50+
51+
</qhelp>

0 commit comments

Comments
 (0)