Skip to content

Commit 1f83c58

Browse files
atorralbaatorralba
authored
Merge pull request #10092 from zbazztian/zbazztian/string.replace-taint
Java: Add additional taint steps for java.lang.String methods
2 parents 55e5fa2 + da79ad8 commit 1f83c58

File tree

11 files changed

+112
-26
lines changed

11 files changed

+112
-26
lines changed

‎java/ql/lib/change-notes/2022-08-19-string-taint-models.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Added taint flow models for the `java.lang.String.(charAt|getBytes)` methods.
5+
* Improved taint flow models for the `java.lang.String.(replace|replaceFirst|replaceAll)` methods. Additional results may be found where users do not properly sanitize their inputs.

‎java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll

Lines changed: 1 addition & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ private predicate qualifierToArgumentStep(Expr tracked, Expr sink) {
251251

252252
/** Access to a method that passes taint from the qualifier. */
253253
private predicate qualifierToMethodStep(Expr tracked, MethodAccess sink) {
254-
(taintPreservingQualifierToMethod(sink.getMethod()) or unsafeEscape(sink)) and
254+
taintPreservingQualifierToMethod(sink.getMethod()) and
255255
tracked = sink.getQualifier()
256256
}
257257

@@ -282,28 +282,6 @@ private predicate taintPreservingQualifierToMethod(Method m) {
282282
)
283283
}
284284

285-
private class StringReplaceMethod extends TaintPreservingCallable {
286-
StringReplaceMethod() {
287-
this.getDeclaringType() instanceof TypeString and
288-
(
289-
this.hasName("replace") or
290-
this.hasName("replaceAll") or
291-
this.hasName("replaceFirst")
292-
)
293-
}
294-
295-
override predicate returnsTaintFrom(int arg) { arg = 1 }
296-
}
297-
298-
private predicate unsafeEscape(MethodAccess ma) {
299-
// Removing `<script>` tags using a string-replace method is
300-
// unsafe if such a tag is embedded inside another one (e.g. `<scr<script>ipt>`).
301-
exists(StringReplaceMethod m | ma.getMethod() = m |
302-
ma.getArgument(0).(StringLiteral).getValue() = "(<script>)" and
303-
ma.getArgument(1).(StringLiteral).getValue() = ""
304-
)
305-
}
306-
307285
/** Access to a method that passes taint from an argument. */
308286
private predicate argToMethodStep(Expr tracked, MethodAccess sink) {
309287
exists(Method m, int i |

‎java/ql/lib/semmle/code/java/frameworks/Strings.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,17 @@ private class StringSummaryCsv extends SummaryModelCsv {
2020
"java.lang;String;false;formatted;(Object[]);;Argument[0].ArrayElement;ReturnValue;taint;manual",
2121
"java.lang;String;false;getChars;;;Argument[-1];Argument[2];taint;manual",
2222
"java.lang;String;false;getBytes;;;Argument[-1];ReturnValue;taint;manual",
23+
"java.lang;String;false;getBytes;;;Argument[-1];Argument[2];taint;manual",
2324
"java.lang;String;false;indent;;;Argument[-1];ReturnValue;taint;manual",
2425
"java.lang;String;false;intern;;;Argument[-1];ReturnValue;taint;manual",
2526
"java.lang;String;false;join;;;Argument[0..1];ReturnValue;taint;manual",
2627
"java.lang;String;false;repeat;(int);;Argument[-1];ReturnValue;taint;manual",
28+
"java.lang;String;false;replace;;;Argument[-1];ReturnValue;taint;manual",
29+
"java.lang;String;false;replace;;;Argument[1];ReturnValue;taint;manual",
30+
"java.lang;String;false;replaceAll;;;Argument[-1];ReturnValue;taint;manual",
31+
"java.lang;String;false;replaceAll;;;Argument[1];ReturnValue;taint;manual",
32+
"java.lang;String;false;replaceFirst;;;Argument[-1];ReturnValue;taint;manual",
33+
"java.lang;String;false;replaceFirst;;;Argument[1];ReturnValue;taint;manual",
2734
"java.lang;String;false;split;;;Argument[-1];ReturnValue;taint;manual",
2835
"java.lang;String;false;String;;;Argument[0];Argument[-1];taint;manual",
2936
"java.lang;String;false;strip;;;Argument[-1];ReturnValue;taint;manual",
@@ -55,6 +62,7 @@ private class StringSummaryCsv extends SummaryModelCsv {
5562
"java.lang;StringBuffer;true;StringBuffer;(CharSequence);;Argument[0];Argument[-1];taint;manual",
5663
"java.lang;StringBuffer;true;StringBuffer;(String);;Argument[0];Argument[-1];taint;manual",
5764
"java.lang;StringBuilder;true;StringBuilder;;;Argument[0];Argument[-1];taint;manual",
65+
"java.lang;CharSequence;true;charAt;;;Argument[-1];ReturnValue;taint;manual",
5866
"java.lang;CharSequence;true;subSequence;;;Argument[-1];ReturnValue;taint;manual",
5967
"java.lang;CharSequence;true;toString;;;Argument[-1];ReturnValue;taint;manual"
6068
]

‎java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,21 @@ class ResponseSplittingConfig extends TaintTracking::Configuration {
2727
override predicate isSink(DataFlow::Node sink) { sink instanceof HeaderSplittingSink }
2828

2929
override predicate isSanitizer(DataFlow::Node node) {
30-
node.getType() instanceof PrimitiveType or
30+
node.getType() instanceof PrimitiveType
31+
or
3132
node.getType() instanceof BoxedType
33+
or
34+
exists(MethodAccess ma, string methodName, CompileTimeConstantExpr target |
35+
node.asExpr() = ma and
36+
ma.getMethod().hasQualifiedName("java.lang", "String", methodName) and
37+
target = ma.getArgument(0) and
38+
(
39+
methodName = "replace" and target.getIntValue() = [10, 13] // 10 == "\n", 13 == "\r"
40+
or
41+
methodName = "replaceAll" and
42+
target.getStringValue().regexpMatch(".*([\n\r]|\\[\\^[^\\]\r\n]*\\]).*")
43+
)
44+
)
3245
}
3346
}
3447

‎java/ql/test/library-tests/dataflow/taint/B.java

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ public static void maintest() throws java.io.UnsupportedEncodingException, java.
4141
String valueOfSubstring = String.valueOf(complex.toCharArray(), 0, 1);
4242
sink(valueOfSubstring);
4343
// tainted - unsafe escape
44-
String badEscape = constructed.replaceAll("(<script>)", "");
44+
String badEscape = constructed.replaceAll("irrelevant", "irrelevant");
4545
sink(badEscape);
4646
// tainted - tokenized string
4747
String token = new StringTokenizer(badEscape).nextToken();
@@ -189,4 +189,32 @@ public StringWrapper(String s) {
189189
public static boolean safe() {
190190
return true;
191191
}
192+
193+
public static void extendedTests(){
194+
String s = taint()[0];
195+
196+
String replReceiver = s.replace("irrelevant", "irrelevant");
197+
sink(replReceiver);
198+
199+
String replChar = "a".replace('a', s.charAt(0));
200+
sink(replChar);
201+
202+
String replCharReceiver = s.replace('a', 'b');
203+
sink(replCharReceiver);
204+
205+
String charAt = "";
206+
for(int i = 0; i < 10; i++)
207+
charAt = charAt + s.charAt(i);
208+
sink(charAt);
209+
210+
byte[] bytes = new byte[10];
211+
s.getBytes(0, 1, bytes, 0);
212+
sink(bytes);
213+
214+
String replAll = s.replaceAll("irrelevant", "irrelevant");
215+
sink(replAll);
216+
217+
String replFirst = s.replaceFirst("irrelevant", "irrelevant");
218+
sink(replFirst);
219+
}
192220
}

‎java/ql/test/library-tests/dataflow/taint/test.expected

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,13 @@
4343
| B.java:15:21:15:27 | taint(...) | B.java:157:10:157:46 | toFile(...) |
4444
| B.java:15:21:15:27 | taint(...) | B.java:160:10:160:46 | getAbsoluteFile(...) |
4545
| B.java:15:21:15:27 | taint(...) | B.java:163:10:163:47 | getCanonicalFile(...) |
46+
| B.java:194:16:194:22 | taint(...) | B.java:197:10:197:21 | replReceiver |
47+
| B.java:194:16:194:22 | taint(...) | B.java:200:10:200:17 | replChar |
48+
| B.java:194:16:194:22 | taint(...) | B.java:203:10:203:25 | replCharReceiver |
49+
| B.java:194:16:194:22 | taint(...) | B.java:208:10:208:15 | charAt |
50+
| B.java:194:16:194:22 | taint(...) | B.java:212:10:212:14 | bytes |
51+
| B.java:194:16:194:22 | taint(...) | B.java:215:10:215:16 | replAll |
52+
| B.java:194:16:194:22 | taint(...) | B.java:218:10:218:18 | replFirst |
4653
| CharSeq.java:7:26:7:32 | taint(...) | CharSeq.java:8:12:8:14 | seq |
4754
| CharSeq.java:7:26:7:32 | taint(...) | CharSeq.java:11:12:11:21 | seqFromSeq |
4855
| CharSeq.java:7:26:7:32 | taint(...) | CharSeq.java:14:12:14:24 | stringFromSeq |

‎java/ql/test/query-tests/security/CWE-089/semmle/examples/taintedString.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,5 +64,7 @@
6464
| Test.java:213:21:213:24 | main | 5 | Test.java:218:14:218:17 | args |
6565
| Validation.java:6:21:6:35 | checkIdentifier | 1 | Validation.java:7:23:7:24 | id |
6666
| Validation.java:6:21:6:35 | checkIdentifier | 2 | Validation.java:8:13:8:14 | id |
67+
| Validation.java:6:21:6:35 | checkIdentifier | 2 | Validation.java:8:13:8:24 | charAt(...) |
68+
| Validation.java:6:21:6:35 | checkIdentifier | 3 | Validation.java:9:28:9:28 | c |
6769
| Validation.java:6:21:6:35 | checkIdentifier | 4 | Validation.java:10:32:10:58 | ... + ... |
6870
| Validation.java:6:21:6:35 | checkIdentifier | 4 | Validation.java:10:57:10:58 | id |

‎java/ql/test/query-tests/security/CWE-113/semmle/tests/ResponseSplitting.expected

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,20 @@
11
edges
22
| ResponseSplitting.java:22:20:22:67 | new Cookie(...) : Cookie | ResponseSplitting.java:23:23:23:28 | cookie |
33
| ResponseSplitting.java:22:39:22:66 | getParameter(...) : String | ResponseSplitting.java:22:20:22:67 | new Cookie(...) : Cookie |
4+
| ResponseSplitting.java:53:14:53:48 | getParameter(...) : String | ResponseSplitting.java:59:27:59:27 | t : String |
5+
| ResponseSplitting.java:59:27:59:27 | t : String | ResponseSplitting.java:59:27:59:57 | replaceFirst(...) |
46
nodes
57
| ResponseSplitting.java:22:20:22:67 | new Cookie(...) : Cookie | semmle.label | new Cookie(...) : Cookie |
68
| ResponseSplitting.java:22:39:22:66 | getParameter(...) : String | semmle.label | getParameter(...) : String |
79
| ResponseSplitting.java:23:23:23:28 | cookie | semmle.label | cookie |
810
| ResponseSplitting.java:28:38:28:72 | getParameter(...) | semmle.label | getParameter(...) |
911
| ResponseSplitting.java:29:38:29:72 | getParameter(...) | semmle.label | getParameter(...) |
12+
| ResponseSplitting.java:53:14:53:48 | getParameter(...) : String | semmle.label | getParameter(...) : String |
13+
| ResponseSplitting.java:59:27:59:27 | t : String | semmle.label | t : String |
14+
| ResponseSplitting.java:59:27:59:57 | replaceFirst(...) | semmle.label | replaceFirst(...) |
1015
subpaths
1116
#select
1217
| ResponseSplitting.java:23:23:23:28 | cookie | ResponseSplitting.java:22:39:22:66 | getParameter(...) : String | ResponseSplitting.java:23:23:23:28 | cookie | Response-splitting vulnerability due to this $@. | ResponseSplitting.java:22:39:22:66 | getParameter(...) | user-provided value |
1318
| ResponseSplitting.java:28:38:28:72 | getParameter(...) | ResponseSplitting.java:28:38:28:72 | getParameter(...) | ResponseSplitting.java:28:38:28:72 | getParameter(...) | Response-splitting vulnerability due to this $@. | ResponseSplitting.java:28:38:28:72 | getParameter(...) | user-provided value |
1419
| ResponseSplitting.java:29:38:29:72 | getParameter(...) | ResponseSplitting.java:29:38:29:72 | getParameter(...) | ResponseSplitting.java:29:38:29:72 | getParameter(...) | Response-splitting vulnerability due to this $@. | ResponseSplitting.java:29:38:29:72 | getParameter(...) | user-provided value |
20+
| ResponseSplitting.java:59:27:59:57 | replaceFirst(...) | ResponseSplitting.java:53:14:53:48 | getParameter(...) : String | ResponseSplitting.java:59:27:59:57 | replaceFirst(...) | Response-splitting vulnerability due to this $@. | ResponseSplitting.java:53:14:53:48 | getParameter(...) | user-provided value |

‎java/ql/test/query-tests/security/CWE-113/semmle/tests/ResponseSplitting.java

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,4 +48,32 @@ public void addCookieName(HttpServletResponse response, Cookie cookie) {
4848
Cookie cookie2 = new Cookie("name", cookie.getName());
4949
response.addCookie(cookie2);
5050
}
51+
52+
public void sanitizerTests(HttpServletRequest request, HttpServletResponse response){
53+
String t = request.getParameter("contentType");
54+
55+
// GOOD: whitelist-based sanitization
56+
response.setHeader("h", t.replaceAll("[^a-zA-Z]", ""));
57+
58+
// BAD: not replacing all problematic characters
59+
response.setHeader("h", t.replaceFirst("[^a-zA-Z]", ""));
60+
61+
// GOOD: replace all line breaks
62+
response.setHeader("h", t.replace('\n', ' ').replace('\r', ' '));
63+
64+
// FALSE NEGATIVE: replace only some line breaks
65+
response.setHeader("h", t.replace('\n', ' '));
66+
67+
// FALSE NEGATIVE: replace only some line breaks
68+
response.setHeader("h", t.replaceAll("\r", ""));
69+
70+
// GOOD: replace all linebreaks with a simple regex
71+
response.setHeader("h", t.replaceAll("\n", "").replaceAll("\r", ""));
72+
73+
// GOOD: replace all linebreaks with a complex regex
74+
response.setHeader("h", t.replaceAll("[\n\r]", ""));
75+
76+
// GOOD: replace all linebreaks with a complex regex
77+
response.setHeader("h", t.replaceAll("something|[a\nb\rc]+|somethingelse", ""));
78+
}
5179
}

‎java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.expected

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,25 @@
11
edges
2+
| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) |
3+
| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:45:28:45:39 | input : String |
24
| UrlRedirect.java:36:58:36:89 | getParameter(...) : String | UrlRedirect.java:36:25:36:89 | ... + ... |
5+
| UrlRedirect.java:45:28:45:39 | input : String | UrlRedirect.java:46:10:46:14 | input : String |
6+
| UrlRedirect.java:46:10:46:14 | input : String | UrlRedirect.java:46:10:46:40 | replaceAll(...) : String |
37
nodes
48
| UrlRedirect.java:23:25:23:54 | getParameter(...) | semmle.label | getParameter(...) |
9+
| UrlRedirect.java:32:25:32:67 | weakCleanup(...) | semmle.label | weakCleanup(...) |
10+
| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | semmle.label | getParameter(...) : String |
511
| UrlRedirect.java:36:25:36:89 | ... + ... | semmle.label | ... + ... |
612
| UrlRedirect.java:36:58:36:89 | getParameter(...) : String | semmle.label | getParameter(...) : String |
713
| UrlRedirect.java:39:34:39:63 | getParameter(...) | semmle.label | getParameter(...) |
814
| UrlRedirect.java:42:43:42:72 | getParameter(...) | semmle.label | getParameter(...) |
15+
| UrlRedirect.java:45:28:45:39 | input : String | semmle.label | input : String |
16+
| UrlRedirect.java:46:10:46:14 | input : String | semmle.label | input : String |
17+
| UrlRedirect.java:46:10:46:40 | replaceAll(...) : String | semmle.label | replaceAll(...) : String |
918
subpaths
19+
| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:45:28:45:39 | input : String | UrlRedirect.java:46:10:46:40 | replaceAll(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) |
1020
#select
1121
| UrlRedirect.java:23:25:23:54 | getParameter(...) | UrlRedirect.java:23:25:23:54 | getParameter(...) | UrlRedirect.java:23:25:23:54 | getParameter(...) | Potentially untrusted URL redirection due to $@. | UrlRedirect.java:23:25:23:54 | getParameter(...) | user-provided value |
22+
| UrlRedirect.java:32:25:32:67 | weakCleanup(...) | UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) | Potentially untrusted URL redirection due to $@. | UrlRedirect.java:32:37:32:66 | getParameter(...) | user-provided value |
1223
| UrlRedirect.java:36:25:36:89 | ... + ... | UrlRedirect.java:36:58:36:89 | getParameter(...) : String | UrlRedirect.java:36:25:36:89 | ... + ... | Potentially untrusted URL redirection due to $@. | UrlRedirect.java:36:58:36:89 | getParameter(...) | user-provided value |
1324
| UrlRedirect.java:39:34:39:63 | getParameter(...) | UrlRedirect.java:39:34:39:63 | getParameter(...) | UrlRedirect.java:39:34:39:63 | getParameter(...) | Potentially untrusted URL redirection due to $@. | UrlRedirect.java:39:34:39:63 | getParameter(...) | user-provided value |
1425
| UrlRedirect.java:42:43:42:72 | getParameter(...) | UrlRedirect.java:42:43:42:72 | getParameter(...) | UrlRedirect.java:42:43:42:72 | getParameter(...) | Potentially untrusted URL redirection due to $@. | UrlRedirect.java:42:43:42:72 | getParameter(...) | user-provided value |

‎java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response)
2727
response.sendRedirect(VALID_REDIRECT);
2828
}
2929

30-
// FALSE NEGATIVE: the user attempts to clean the string, but this will fail
30+
// BAD: the user attempts to clean the string, but this will fail
3131
// if the argument is "hthttp://tp://malicious.com"
3232
response.sendRedirect(weakCleanup(request.getParameter("target")));
3333

0 commit comments

Comments
 (0)