Skip to content

Commit cf2bfb5

Browse files
JLLeitschuhJLLeitschuh
committed
Update OS Check from Review Feedback
1 parent efcdd23 commit cf2bfb5

File tree

9 files changed

+171
-40
lines changed

9 files changed

+171
-40
lines changed

‎java/ql/lib/semmle/code/java/JDK.qll‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,27 @@ class StringLengthMethod extends Method {
3737
StringLengthMethod() { this.hasName("length") and this.getDeclaringType() instanceof TypeString }
3838
}
3939

40+
/**
41+
* The methods on the class `java.lang.String` that are used to perform partial matches with a specified substring or char.
42+
*/
43+
class StringPartialMatchMethod extends Method {
44+
StringPartialMatchMethod() {
45+
this.hasName([
46+
"contains", "startsWith", "endsWith", "matches", "indexOf", "lastIndexOf", "regionMatches"
47+
]) and
48+
this.getDeclaringType() instanceof TypeString
49+
}
50+
51+
/**
52+
* The index of the parameter that is being matched against.
53+
*/
54+
int getMatchParameterIndex() {
55+
if not this.hasName("regionMatches")
56+
then result = 0
57+
else this.getParameterType(result) instanceof TypeString
58+
}
59+
}
60+
4061
/** The class `java.lang.StringBuffer`. */
4162
class TypeStringBuffer extends Class {
4263
TypeStringBuffer() { this.hasQualifiedName("java.lang", "StringBuffer") }

‎java/ql/lib/semmle/code/java/StringCheck.qll‎

Lines changed: 0 additions & 14 deletions
This file was deleted.

‎java/ql/lib/semmle/code/java/os/OSCheck.qll‎

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import java
66
import semmle.code.java.controlflow.Guards
77
private import semmle.code.java.frameworks.apache.Lang
88
private import semmle.code.java.dataflow.DataFlow
9-
private import semmle.code.java.StringCheck
109

1110
/**
1211
* A guard that checks if the current platform is Windows.
@@ -21,34 +20,36 @@ abstract class IsUnixGuard extends Guard { }
2120
/**
2221
* Holds when the MethodAccess is a call to check the current OS using either the upper case `osUpperCase` or lower case `osLowerCase` string constants.
2322
*/
24-
bindingset[osUpperCase, osLowerCase]
25-
private predicate isOsFromSystemProp(MethodAccess ma, string osUpperCase, string osLowerCase) {
26-
exists(MethodAccessSystemGetProperty sgpMa |
23+
bindingset[osString]
24+
private predicate isOsFromSystemProp(MethodAccess ma, string osString) {
25+
exists(MethodAccessSystemGetProperty sgpMa, Expr sgpFlowsToExpr |
2726
sgpMa.hasCompileTimeConstantGetPropertyName("os.name")
2827
|
29-
DataFlow::localExprFlow(sgpMa, ma.getQualifier()) and // Call from System.getProperty to some partial match method
30-
ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().matches(osUpperCase)
31-
or
32-
exists(MethodAccess toLowerCaseMa |
33-
toLowerCaseMa.getMethod() =
34-
any(Method m | m.getDeclaringType() instanceof TypeString and m.hasName("toLowerCase"))
35-
|
36-
DataFlow::localExprFlow(sgpMa, toLowerCaseMa.getQualifier()) and // Call from System.getProperty to toLowerCase
37-
DataFlow::localExprFlow(toLowerCaseMa, ma.getQualifier()) and // Call from toLowerCase to some partial match method
38-
ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().matches(osLowerCase)
28+
DataFlow::localExprFlow(sgpMa, sgpFlowsToExpr) and
29+
ma.getAnArgument().(CompileTimeConstantExpr).getStringValue().toLowerCase().matches(osString) and // Call from System.getProperty to some partial match method
30+
(
31+
sgpFlowsToExpr = ma.getQualifier()
32+
or
33+
exists(MethodAccess caseChangeMa |
34+
caseChangeMa.getMethod() =
35+
any(Method m |
36+
m.getDeclaringType() instanceof TypeString and m.hasName(["toLowerCase", "toUpperCase"])
37+
)
38+
|
39+
sgpFlowsToExpr = caseChangeMa.getQualifier() and // Call from System.getProperty to case-switching method
40+
DataFlow::localExprFlow(caseChangeMa, ma.getQualifier()) // Call from case-switching method to some partial match method
41+
)
3942
)
4043
) and
41-
isStringPartialMatch(ma)
44+
ma.getMethod() instanceof StringPartialMatchMethod
4245
}
4346

4447
private class IsWindowsFromSystemProp extends IsWindowsGuard instanceof MethodAccess {
45-
IsWindowsFromSystemProp() { isOsFromSystemProp(this, "Window%", "window%") }
48+
IsWindowsFromSystemProp() { isOsFromSystemProp(this, "window%") }
4649
}
4750

4851
private class IsUnixFromSystemProp extends IsUnixGuard instanceof MethodAccess {
49-
IsUnixFromSystemProp() {
50-
isOsFromSystemProp(this, ["Mac%", "Linux%", "LINUX%"], ["mac%", "linux%"])
51-
}
52+
IsUnixFromSystemProp() { isOsFromSystemProp(this, ["mac%", "linux%"]) }
5253
}
5354

5455
bindingset[fieldNamePattern]
@@ -70,7 +71,7 @@ private class IsUnixFromApacheCommons extends IsUnixGuard instanceof FieldAccess
7071
/**
7172
* A guard that checks if the `java.nio.file.FileSystem` supports posix file permissions.
7273
* This is often used to infer if the OS is unix-based.
73-
* Looks for calls to `contains("posix")` on the `supportedFileAttributeViews` method returned by `FileSystem`.
74+
* Looks for calls to `contains("posix")` on the `supportedFileAttributeViews()` method returned by `FileSystem`.
7475
*/
7576
private class IsUnixFromPosixFromFileSystem extends IsUnixGuard instanceof MethodAccess {
7677
IsUnixFromPosixFromFileSystem() {

‎java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -104,17 +104,17 @@ private class FileCreateTempFileSink extends FileCreationSink {
104104
}
105105

106106
/**
107-
* A guard that checks what OS the program is running on.
107+
* A guard that checks whether or not the the program is running on the Windows OS.
108108
*/
109-
abstract private class OsBarrierGuard extends DataFlow::BarrierGuard { }
109+
abstract private class WindowsOsBarrierGuard extends DataFlow::BarrierGuard { }
110110

111-
private class IsUnixBarrierGuard extends OsBarrierGuard instanceof IsUnixGuard {
111+
private class IsNotUnixBarrierGuard extends WindowsOsBarrierGuard instanceof IsUnixGuard {
112112
override predicate checks(Expr e, boolean branch) {
113113
this.controls(e.getBasicBlock(), branch.booleanNot())
114114
}
115115
}
116116

117-
private class IsWindowsBarrierGuard extends OsBarrierGuard instanceof IsWindowsGuard {
117+
private class IsWindowsBarrierGuard extends WindowsOsBarrierGuard instanceof IsWindowsGuard {
118118
override predicate checks(Expr e, boolean branch) { this.controls(e.getBasicBlock(), branch) }
119119
}
120120

@@ -147,7 +147,7 @@ private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Conf
147147
}
148148

149149
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
150-
guard instanceof OsBarrierGuard
150+
guard instanceof WindowsOsBarrierGuard
151151
}
152152
}
153153

‎java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import semmle.code.java.dataflow.FlowSources
55
import semmle.code.java.frameworks.Servlets
66
import semmle.code.java.frameworks.spring.SpringWeb
77
import semmle.code.java.security.RequestForgery
8-
private import semmle.code.java.StringCheck
98
private import semmle.code.java.dataflow.StringPrefixes
109

1110
/** A sink for unsafe URL forward vulnerabilities. */
@@ -168,6 +167,13 @@ private class URLEncodingBarrierGuard extends UnsafeUrlForwardBarrierGuard insta
168167
}
169168
}
170169

170+
/**
171+
* Holds if `ma` is a call to a method that checks a partial string match.
172+
*/
173+
private predicate isStringPartialMatch(MethodAccess ma) {
174+
ma.getMethod() instanceof StringPartialMatchMethod
175+
}
176+
171177
/**
172178
* Holds if `ma` is a call to a method of `java.nio.file.Path` that checks a partial path match.
173179
*/

‎java/ql/test/library-tests/JDK/PrintAst.expected‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,75 @@ jdk/A.java:
6060
# 28| 0: [ArrayTypeAccess] ...[]
6161
# 28| 0: [TypeAccess] String
6262
# 28| 5: [BlockStmt] { ... }
63+
jdk/StringMatch.java:
64+
# 0| [CompilationUnit] StringMatch
65+
# 1| 1: [Class] StringMatch
66+
# 2| 3: [FieldDeclaration] String STR;
67+
# 2| -1: [TypeAccess] String
68+
# 2| 0: [StringLiteral] "the quick brown fox jumps over the lazy dog"
69+
# 4| 4: [Method] a
70+
# 4| 3: [TypeAccess] void
71+
# 4| 5: [BlockStmt] { ... }
72+
# 5| 0: [ExprStmt] <Expr>;
73+
# 5| 0: [MethodAccess] matches(...)
74+
# 5| -1: [VarAccess] STR
75+
# 5| 0: [StringLiteral] "[a-z]+"
76+
# 8| 5: [Method] b
77+
# 8| 3: [TypeAccess] void
78+
# 8| 5: [BlockStmt] { ... }
79+
# 9| 0: [ExprStmt] <Expr>;
80+
# 9| 0: [MethodAccess] contains(...)
81+
# 9| -1: [VarAccess] STR
82+
# 9| 0: [StringLiteral] "the"
83+
# 12| 6: [Method] c
84+
# 12| 3: [TypeAccess] void
85+
# 12| 5: [BlockStmt] { ... }
86+
# 13| 0: [ExprStmt] <Expr>;
87+
# 13| 0: [MethodAccess] startsWith(...)
88+
# 13| -1: [VarAccess] STR
89+
# 13| 0: [StringLiteral] "the"
90+
# 16| 7: [Method] d
91+
# 16| 3: [TypeAccess] void
92+
# 16| 5: [BlockStmt] { ... }
93+
# 17| 0: [ExprStmt] <Expr>;
94+
# 17| 0: [MethodAccess] endsWith(...)
95+
# 17| -1: [VarAccess] STR
96+
# 17| 0: [StringLiteral] "dog"
97+
# 20| 8: [Method] e
98+
# 20| 3: [TypeAccess] void
99+
# 20| 5: [BlockStmt] { ... }
100+
# 21| 0: [ExprStmt] <Expr>;
101+
# 21| 0: [MethodAccess] indexOf(...)
102+
# 21| -1: [VarAccess] STR
103+
# 21| 0: [StringLiteral] "lazy"
104+
# 24| 9: [Method] f
105+
# 24| 3: [TypeAccess] void
106+
# 24| 5: [BlockStmt] { ... }
107+
# 25| 0: [ExprStmt] <Expr>;
108+
# 25| 0: [MethodAccess] lastIndexOf(...)
109+
# 25| -1: [VarAccess] STR
110+
# 25| 0: [StringLiteral] "lazy"
111+
# 28| 10: [Method] g
112+
# 28| 3: [TypeAccess] void
113+
# 28| 5: [BlockStmt] { ... }
114+
# 29| 0: [ExprStmt] <Expr>;
115+
# 29| 0: [MethodAccess] regionMatches(...)
116+
# 29| -1: [VarAccess] STR
117+
# 29| 0: [IntegerLiteral] 0
118+
# 29| 1: [StringLiteral] "fox"
119+
# 29| 2: [IntegerLiteral] 0
120+
# 29| 3: [IntegerLiteral] 4
121+
# 32| 11: [Method] h
122+
# 32| 3: [TypeAccess] void
123+
# 32| 5: [BlockStmt] { ... }
124+
# 33| 0: [ExprStmt] <Expr>;
125+
# 33| 0: [MethodAccess] regionMatches(...)
126+
# 33| -1: [VarAccess] STR
127+
# 33| 0: [BooleanLiteral] true
128+
# 33| 1: [IntegerLiteral] 0
129+
# 33| 2: [StringLiteral] "FOX"
130+
# 33| 3: [IntegerLiteral] 0
131+
# 33| 4: [IntegerLiteral] 4
63132
jdk/SystemGetPropertyCall.java:
64133
# 0| [CompilationUnit] SystemGetPropertyCall
65134
# 3| 1: [Class] SystemGetPropertyCall

‎java/ql/test/library-tests/JDK/StringMatch.expected‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
| jdk/StringMatch.java:5:9:5:29 | matches(...) | jdk/StringMatch.java:5:21:5:28 | "[a-z]+" |
2+
| jdk/StringMatch.java:9:9:9:27 | contains(...) | jdk/StringMatch.java:9:22:9:26 | "the" |
3+
| jdk/StringMatch.java:13:9:13:29 | startsWith(...) | jdk/StringMatch.java:13:24:13:28 | "the" |
4+
| jdk/StringMatch.java:17:9:17:27 | endsWith(...) | jdk/StringMatch.java:17:22:17:26 | "dog" |
5+
| jdk/StringMatch.java:21:9:21:27 | indexOf(...) | jdk/StringMatch.java:21:21:21:26 | "lazy" |
6+
| jdk/StringMatch.java:25:9:25:31 | lastIndexOf(...) | jdk/StringMatch.java:25:25:25:30 | "lazy" |
7+
| jdk/StringMatch.java:29:9:29:41 | regionMatches(...) | jdk/StringMatch.java:29:30:29:34 | "fox" |
8+
| jdk/StringMatch.java:33:9:33:47 | regionMatches(...) | jdk/StringMatch.java:33:36:33:40 | "FOX" |

‎java/ql/test/library-tests/JDK/StringMatch.ql‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
import java
2+
3+
from MethodAccess ma, StringPartialMatchMethod m
4+
where ma.getMethod() = m
5+
select ma, ma.getArgument(m.getMatchParameterIndex())

‎java/ql/test/library-tests/JDK/jdk/StringMatch.java‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
public class StringMatch {
2+
private static String STR = "the quick brown fox jumps over the lazy dog";
3+
4+
void a() {
5+
STR.matches("[a-z]+");
6+
}
7+
8+
void b() {
9+
STR.contains("the");
10+
}
11+
12+
void c() {
13+
STR.startsWith("the");
14+
}
15+
16+
void d() {
17+
STR.endsWith("dog");
18+
}
19+
20+
void e() {
21+
STR.indexOf("lazy");
22+
}
23+
24+
void f() {
25+
STR.lastIndexOf("lazy");
26+
}
27+
28+
void g() {
29+
STR.regionMatches(0, "fox", 0, 4);
30+
}
31+
32+
void h() {
33+
STR.regionMatches(true, 0, "FOX", 0, 4);
34+
}
35+
}

0 commit comments

Comments
 (0)