Skip to content

Commit 09ec379

Browse files
smehta23smehta23
committed
Partial Path Traversal split into 2 queries
1 parent b7e5227 commit 09ec379

File tree

7 files changed

+125
-69
lines changed

7 files changed

+125
-69
lines changed

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

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
import java
2+
private import semmle.code.java.dataflow.DataFlow
3+
private import semmle.code.java.environment.SystemProperty
4+
5+
class MethodStringStartsWith extends Method {
6+
MethodStringStartsWith() {
7+
this.getDeclaringType() instanceof TypeString and
8+
this.hasName("startsWith")
9+
}
10+
}
11+
12+
class MethodFileGetCanonicalPath extends Method {
13+
MethodFileGetCanonicalPath() {
14+
this.getDeclaringType() instanceof TypeFile and
15+
this.hasName("getCanonicalPath")
16+
}
17+
}
18+
19+
class MethodAccessFileGetCanonicalPath extends MethodAccess {
20+
MethodAccessFileGetCanonicalPath() { this.getMethod() instanceof MethodFileGetCanonicalPath }
21+
}
22+
23+
abstract class FileSeparatorExpr extends Expr { }
24+
25+
class SystemPropFileSeparatorExpr extends FileSeparatorExpr {
26+
SystemPropFileSeparatorExpr() { this = getSystemProperty("file.separator") }
27+
}
28+
29+
class StringLiteralFileSeparatorExpr extends FileSeparatorExpr, StringLiteral {
30+
StringLiteralFileSeparatorExpr() {
31+
this.getValue().matches("%/") or this.getValue().matches("%\\")
32+
}
33+
}
34+
35+
class CharacterLiteralFileSeparatorExpr extends FileSeparatorExpr, CharacterLiteral {
36+
CharacterLiteralFileSeparatorExpr() { this.getValue() = "/" or this.getValue() = "\\" }
37+
}
38+
39+
class FileSeparatorAppend extends AddExpr {
40+
FileSeparatorAppend() { this.getRightOperand() instanceof FileSeparatorExpr }
41+
}
42+
43+
predicate isSafe(Expr expr) {
44+
DataFlow::localExprFlow(any(Expr e |
45+
e instanceof FileSeparatorAppend or e instanceof FileSeparatorExpr
46+
), expr)
47+
}
48+
49+
class PartialPathTraversalMethodAccess extends MethodAccess {
50+
PartialPathTraversalMethodAccess() {
51+
this.getMethod() instanceof MethodStringStartsWith and
52+
DataFlow::localExprFlow(any(MethodAccessFileGetCanonicalPath gcpma), this.getQualifier()) and
53+
not isSafe(this.getArgument(0))
54+
}
55+
}

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
import java
2+
import semmle.code.java.security.PartialPathTraversal
3+
import semmle.code.java.dataflow.DataFlow
4+
import semmle.code.java.dataflow.ExternalFlow
5+
import semmle.code.java.dataflow.TaintTracking
6+
import semmle.code.java.dataflow.FlowSources
7+
8+
class PartialPathTraversalFromRemoteConfig extends TaintTracking::Configuration {
9+
PartialPathTraversalFromRemoteConfig() { this = "PartialPathTraversalFromRemoteConfig" }
10+
11+
override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
12+
13+
override predicate isSink(DataFlow::Node node) {
14+
any(PartialPathTraversalMethodAccess ma).getQualifier() = node.asExpr()
15+
}
16+
}

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

Lines changed: 2 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -10,57 +10,7 @@
1010
* external/cwe/cwe-023
1111
*/
1212

13-
import java
14-
private import semmle.code.java.dataflow.DataFlow
15-
private import semmle.code.java.environment.SystemProperty
13+
import semmle.code.java.security.PartialPathTraversal
1614

17-
class MethodStringStartsWith extends Method {
18-
MethodStringStartsWith() {
19-
this.getDeclaringType() instanceof TypeString and
20-
this.hasName("startsWith")
21-
}
22-
}
23-
24-
class MethodFileGetCanonicalPath extends Method {
25-
MethodFileGetCanonicalPath() {
26-
this.getDeclaringType() instanceof TypeFile and
27-
this.hasName("getCanonicalPath")
28-
}
29-
}
30-
31-
class MethodAccessFileGetCanonicalPath extends MethodAccess {
32-
MethodAccessFileGetCanonicalPath() { this.getMethod() instanceof MethodFileGetCanonicalPath }
33-
}
34-
35-
abstract class FileSeparatorExpr extends Expr { }
36-
37-
class SystemPropFileSeparatorExpr extends FileSeparatorExpr {
38-
SystemPropFileSeparatorExpr() { this = getSystemProperty("file.separator") }
39-
}
40-
41-
class StringLiteralFileSeparatorExpr extends FileSeparatorExpr, StringLiteral {
42-
StringLiteralFileSeparatorExpr() {
43-
this.getValue().matches("%/") or this.getValue().matches("%\\")
44-
}
45-
}
46-
47-
class CharacterLiteralFileSeparatorExpr extends FileSeparatorExpr, CharacterLiteral {
48-
CharacterLiteralFileSeparatorExpr() { this.getValue() = "/" or this.getValue() = "\\" }
49-
}
50-
51-
class FileSeparatorAppend extends AddExpr {
52-
FileSeparatorAppend() { this.getRightOperand() instanceof FileSeparatorExpr }
53-
}
54-
55-
predicate isSafe(Expr expr) {
56-
DataFlow::localExprFlow(any(Expr e |
57-
e instanceof FileSeparatorAppend or e instanceof FileSeparatorExpr
58-
), expr)
59-
}
60-
61-
from MethodAccess ma
62-
where
63-
ma.getMethod() instanceof MethodStringStartsWith and
64-
DataFlow::localExprFlow(any(MethodAccessFileGetCanonicalPath gcpma), ma.getQualifier()) and
65-
not isSafe(ma.getArgument(0))
15+
from PartialPathTraversalMethodAccess ma
6616
select ma, "Partial Path Traversal Vulnerability due to insufficient guard against path traversal"

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
/**
2+
* @name Partial Path Traversal Vulnerability From Remote
3+
* @description A prefix used to check that a canonicalised path falls within another must be slash-terminated.
4+
* @kind path-problem
5+
* @problem.severity error
6+
* @security-severity 9.3
7+
* @precision high
8+
* @id java/partial-path-traversal-from-remote
9+
* @tags security
10+
* external/cwe/cwe-023
11+
*/
12+
13+
import semmle.code.java.security.PartialPathTraversalQuery
14+
15+
from DataFlow::PathNode source, DataFlow::PathNode sink
16+
where any(PartialPathTraversalFromRemoteConfig config).hasFlowPath(source, sink)
17+
select sink.getNode(), source, sink,
18+
"Partial Path Traversal Vulnerability due to insufficient guard against path traversal from user-supplied data"

‎java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.expected

Whitespace-only changes.

‎java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.ql

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
import java
2+
import TestUtilities.InlineFlowTest
3+
import semmle.code.java.security.PartialPathTraversalQuery
4+
5+
class TestRemoteSource extends RemoteFlowSource {
6+
TestRemoteSource() { this.asParameter().hasName(["dir", "path"]) }
7+
8+
override string getSourceType() { result = "TestSource" }
9+
}
10+
11+
class Test extends InlineFlowTest {
12+
override DataFlow::Configuration getValueFlowConfig() { none() }
13+
14+
override TaintTracking::Configuration getTaintFlowConfig() {
15+
result instanceof PartialPathTraversalFromRemoteConfig
16+
}
17+
}

‎java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,14 @@
77

88
public class PartialPathTraversalTest {
99
public void esapiExample(File dir, File parent) throws IOException {
10-
if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath())) {
10+
if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath())) { // $hasTaintFlow
1111
throw new IOException("Invalid directory: " + dir.getCanonicalPath());
1212
}
1313
}
1414

1515
@SuppressWarnings("ResultOfMethodCallIgnored")
1616
void foo1(File dir, File parent) throws IOException {
17-
(dir.getCanonicalPath()).startsWith((parent.getCanonicalPath()));
17+
(dir.getCanonicalPath()).startsWith((parent.getCanonicalPath())); // $hasTaintFlow
1818
}
1919

2020
void foo2(File dir, File parent) throws IOException {
@@ -26,42 +26,42 @@ void foo2(File dir, File parent) throws IOException {
2626

2727
void foo3(File dir, File parent) throws IOException {
2828
String parentPath = parent.getCanonicalPath();
29-
if (!dir.getCanonicalPath().startsWith(parentPath)) {
29+
if (!dir.getCanonicalPath().startsWith(parentPath)) { // $hasTaintFlow
3030
throw new IOException("Invalid directory: " + dir.getCanonicalPath());
3131
}
3232
}
3333

3434
void foo4(File dir) throws IOException {
35-
if (!dir.getCanonicalPath().startsWith("/usr" + "/dir")) {
35+
if (!dir.getCanonicalPath().startsWith("/usr" + "/dir")) { // $hasTaintFlow
3636
throw new IOException("Invalid directory: " + dir.getCanonicalPath());
3737
}
3838
}
3939

4040
void foo5(File dir, File parent) throws IOException {
4141
String canonicalPath = dir.getCanonicalPath();
42-
if (!canonicalPath.startsWith(parent.getCanonicalPath())) {
42+
if (!canonicalPath.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow
4343
throw new IOException("Invalid directory: " + dir.getCanonicalPath());
4444
}
4545
}
4646

4747
void foo6(File dir, File parent) throws IOException {
4848
String canonicalPath = dir.getCanonicalPath();
49-
if (!canonicalPath.startsWith(parent.getCanonicalPath())) {
49+
if (!canonicalPath.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow
5050
throw new IOException("Invalid directory: " + dir.getCanonicalPath());
5151
}
5252
String canonicalPath2 = dir.getCanonicalPath();
53-
if (!canonicalPath2.startsWith(parent.getCanonicalPath())) {
53+
if (!canonicalPath2.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow
5454
throw new IOException("Invalid directory: " + dir.getCanonicalPath());
5555
}
5656
}
5757

5858
void foo7(File dir, File parent) throws IOException {
5959
String canonicalPath = dir.getCanonicalPath();
6060
String canonicalPath2 = dir.getCanonicalPath();
61-
if (!canonicalPath.startsWith(parent.getCanonicalPath())) {
61+
if (!canonicalPath.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow
6262
throw new IOException("Invalid directory: " + dir.getCanonicalPath());
6363
}
64-
if (!canonicalPath2.startsWith(parent.getCanonicalPath())) {
64+
if (!canonicalPath2.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow
6565
throw new IOException("Invalid directory: " + dir.getCanonicalPath());
6666
}
6767
}
@@ -72,7 +72,7 @@ File getChild() {
7272

7373
void foo8(File parent) throws IOException {
7474
String canonicalPath = getChild().getCanonicalPath();
75-
if (!canonicalPath.startsWith(parent.getCanonicalPath())) {
75+
if (!canonicalPath.startsWith(parent.getCanonicalPath())) {
7676
throw new IOException("Invalid directory: " + getChild().getCanonicalPath());
7777
}
7878
}
@@ -91,18 +91,18 @@ void foo10(File dir, File parent) throws IOException {
9191

9292
void foo11(File dir, File parent) throws IOException {
9393
String parentCanonical = parent.getCanonicalPath();
94-
if (!dir.getCanonicalPath().startsWith(parentCanonical)) {
94+
if (!dir.getCanonicalPath().startsWith(parentCanonical)) { // $hasTaintFlow
9595
throw new IOException("Invalid directory: " + dir.getCanonicalPath());
9696
}
9797
}
9898

9999
void foo12(File dir, File parent) throws IOException {
100100
String parentCanonical = parent.getCanonicalPath();
101101
String parentCanonical2 = parent.getCanonicalPath();
102-
if (!dir.getCanonicalPath().startsWith(parentCanonical)) {
102+
if (!dir.getCanonicalPath().startsWith(parentCanonical)) { // $hasTaintFlow
103103
throw new IOException("Invalid directory: " + dir.getCanonicalPath());
104104
}
105-
if (!dir.getCanonicalPath().startsWith(parentCanonical2)) {
105+
if (!dir.getCanonicalPath().startsWith(parentCanonical2)) { // $hasTaintFlow
106106
throw new IOException("Invalid directory: " + dir.getCanonicalPath());
107107
}
108108
}
@@ -116,7 +116,7 @@ void foo13(File dir, File parent) throws IOException {
116116

117117
void foo14(File dir, File parent) throws IOException {
118118
String parentCanonical = parent.getCanonicalPath() + separatorChar;
119-
if (!dir.getCanonicalPath().startsWith(parentCanonical)) {
119+
if (!dir.getCanonicalPath().startsWith(parentCanonical)) {
120120
throw new IOException("Invalid directory: " + dir.getCanonicalPath());
121121
}
122122
}
@@ -170,7 +170,7 @@ void foo18(File dir, File parent, boolean branch) throws IOException {
170170

171171
void foo19(File dir, File parent) throws IOException {
172172
String parentCanonical = parent.getCanonicalPath() + "/potato";
173-
if (!dir.getCanonicalPath().startsWith(parentCanonical)) {
173+
if (!dir.getCanonicalPath().startsWith(parentCanonical)) { // $hasTaintFlow
174174
throw new IOException("Invalid directory: " + dir.getCanonicalPath());
175175
}
176176
}
@@ -188,7 +188,7 @@ InputStream foo20(String... path) {
188188
String filePath = sb.toString();
189189
File encodedFile = new File(filePath);
190190
try {
191-
if (!encodedFile.getCanonicalPath().startsWith(cacheDir.getCanonicalPath())) {
191+
if (!encodedFile.getCanonicalPath().startsWith(cacheDir.getCanonicalPath())) { // $hasTaintFlow
192192
return null;
193193
}
194194
return Files.newInputStream(encodedFile.toPath());
@@ -206,7 +206,7 @@ void foo21(File dir, File parent) throws IOException {
206206

207207
void foo22(File dir, File dir2, File parent, boolean conditional) throws IOException {
208208
String canonicalPath = conditional ? dir.getCanonicalPath() : dir2.getCanonicalPath();
209-
if (!canonicalPath.startsWith(parent.getCanonicalPath())) {
209+
if (!canonicalPath.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow
210210
throw new IOException("Invalid directory: " + dir.getCanonicalPath());
211211
}
212212
}

0 commit comments

Comments
 (0)