Skip to content

Commit c720695

Browse files
owen-mcCopilot
owen-mc
and
Copilot
committed
Model string match sanitizer guards
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent b633b8c commit c720695

6 files changed

Lines changed: 133 additions & 21 deletions

File tree

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

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ module;
99
import java
1010

1111
/**
12-
* A module importing the frameworks that implement `RegexMatch`es,
12+
* A module importing the frameworks that implement string matches,
1313
* ensuring that they are visible to the concepts library.
1414
*/
1515
private module Frameworks {
@@ -82,6 +82,62 @@ module RegexMatch {
8282
}
8383
}
8484

85+
/**
86+
* An expression that tests whether an input string contains characters from a
87+
* character set.
88+
*
89+
* Extend this class to refine existing API models. If you want to model new APIs,
90+
* extend `StringCharacterSetMatch::Range` instead.
91+
*/
92+
class StringCharacterSetMatch extends Expr instanceof StringCharacterSetMatch::Range {
93+
/** Gets the input string being tested. */
94+
Expr getInputString() { result = super.getInputString() }
95+
96+
/** Gets the expression that specifies the character set. */
97+
Expr getMatchExpression() { result = super.getMatchExpression() }
98+
99+
/** Gets an expression to be sanitized. */
100+
Expr getASanitizedExpr() { result = [this.getInputString(), super.getAdditionalSanitizedExpr()] }
101+
102+
/**
103+
* Holds if taking `branch` means that the input contains at least one
104+
* character from the match expression.
105+
*/
106+
predicate hasMatchingCharacterOnBranch(boolean branch) {
107+
super.hasMatchingCharacterOnBranch(branch)
108+
}
109+
110+
/** Gets a descriptive name for this character-set match. */
111+
string getName() { result = super.getName() }
112+
}
113+
114+
/** Provides classes for modeling character-set matching APIs. */
115+
module StringCharacterSetMatch {
116+
/**
117+
* A range class for APIs that test whether an input contains characters from
118+
* a character set.
119+
*/
120+
abstract class Range extends Expr {
121+
/** Gets the input string being tested. */
122+
abstract Expr getInputString();
123+
124+
/** Gets the expression that specifies the character set. */
125+
abstract Expr getMatchExpression();
126+
127+
/** Gets an additional expression to be sanitized, if any. */
128+
Expr getAdditionalSanitizedExpr() { none() }
129+
130+
/**
131+
* Holds if taking `branch` means that the input contains at least one
132+
* character from the match expression.
133+
*/
134+
abstract predicate hasMatchingCharacterOnBranch(boolean branch);
135+
136+
/** Gets a descriptive name for this character-set match. */
137+
abstract string getName();
138+
}
139+
}
140+
85141
/**
86142
* An expression that replaces every occurrence of a target in a string.
87143
*

‎java/ql/lib/semmle/code/java/frameworks/apache/Lang.qll‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,28 @@ class TypeApacheSystemUtils extends Class {
4848
}
4949
}
5050

51+
/** A call to `StringUtils.containsAny` or `StringUtils.containsNone`. */
52+
private class ApacheStringUtilsCharacterSetMatch extends MethodCall, StringCharacterSetMatch::Range {
53+
ApacheStringUtilsCharacterSetMatch() {
54+
this.getMethod()
55+
.getDeclaringType()
56+
.hasQualifiedName(["org.apache.commons.lang", "org.apache.commons.lang3"], "StringUtils") and
57+
this.getMethod().hasName(["containsAny", "containsNone"])
58+
}
59+
60+
override Expr getInputString() { result = this.getArgument(0) }
61+
62+
override Expr getMatchExpression() { result = this.getArgument(1) }
63+
64+
override predicate hasMatchingCharacterOnBranch(boolean branch) {
65+
this.getMethod().hasName("containsAny") and branch = true
66+
or
67+
this.getMethod().hasName("containsNone") and branch = false
68+
}
69+
70+
override string getName() { result = "StringUtils." + this.getMethod().getName() }
71+
}
72+
5173
/** A call to `StringUtils.replaceChars`. */
5274
private class ApacheStringUtilsReplaceCharsCall extends MethodCall, StringReplacement::Range {
5375
ApacheStringUtilsReplaceCharsCall() {

‎java/ql/lib/semmle/code/java/frameworks/guava/Guava.qll‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,46 @@ private class CharMatcherAnyOfCall extends MethodCall {
2424
}
2525
}
2626

27+
/** A call to `CharMatcher.noneOf`. */
28+
private class CharMatcherNoneOfCall extends MethodCall {
29+
CharMatcherNoneOfCall() {
30+
this.getMethod().hasQualifiedName("com.google.common.base", "CharMatcher", "noneOf")
31+
}
32+
}
33+
34+
/** A call that tests a string using a character-set `CharMatcher`. */
35+
private class CharMatcherCharacterSetMatch extends MethodCall, StringCharacterSetMatch::Range {
36+
CharMatcherCharacterSetMatch() {
37+
this.getMethod()
38+
.hasQualifiedName("com.google.common.base", "CharMatcher",
39+
["matchesAllOf", "matchesAnyOf", "matchesNoneOf"]) and
40+
(
41+
this.getMethod().hasName(["matchesAnyOf", "matchesNoneOf"]) and
42+
getUniqueLocalOrigin(this.getQualifier()) instanceof CharMatcherAnyOfCall
43+
or
44+
this.getMethod().hasName("matchesAllOf") and
45+
getUniqueLocalOrigin(this.getQualifier()) instanceof CharMatcherNoneOfCall
46+
)
47+
}
48+
49+
override Expr getInputString() { result = this.getArgument(0) }
50+
51+
override Expr getMatchExpression() {
52+
result = getUniqueLocalOrigin(this.getQualifier()).(MethodCall).getArgument(0)
53+
}
54+
55+
override predicate hasMatchingCharacterOnBranch(boolean branch) {
56+
this.getMethod().hasName("matchesAnyOf") and branch = true
57+
or
58+
this.getMethod().hasName("matchesNoneOf") and branch = false
59+
or
60+
this.getMethod().hasName("matchesAllOf") and
61+
branch = false
62+
}
63+
64+
override string getName() { result = "CharMatcher." + this.getMethod().getName() }
65+
}
66+
2767
/** A call to `CharMatcher.replaceFrom`. */
2868
private class CharMatcherReplaceFromCall extends MethodCall, StringReplacement::Range {
2969
CharMatcherReplaceFromCall() {

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,15 @@ private predicate logInjectionGuard(Guard g, Expr e, boolean branch) {
126126
branch = false
127127
)
128128
or
129+
exists(StringCharacterSetMatch match, CompileTimeConstantExpr characters, boolean matchingBranch |
130+
match = g and
131+
characters = match.getMatchExpression() and
132+
containsBothLineBreaks(characters) and
133+
e = match.getASanitizedExpr() and
134+
match.hasMatchingCharacterOnBranch(matchingBranch) and
135+
branch = matchingBranch.booleanNot()
136+
)
137+
or
129138
exists(RegexMatch rm, CompileTimeConstantExpr target |
130139
rm = g and
131140
target = rm.getRegex() and

‎java/ql/test/query-tests/security/CWE-117/LibrarySanitizers.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -99,23 +99,23 @@ public void testGuards() {
9999
}
100100

101101
if (!org.apache.commons.lang3.StringUtils.containsAny(source, "\r\n")) {
102-
logger.debug(source); // $ SPURIOUS: Alert
102+
logger.debug(source);
103103
}
104104

105105
if (org.apache.commons.lang3.StringUtils.containsNone(source, "\r\n")) {
106-
logger.debug(source); // $ SPURIOUS: Alert
106+
logger.debug(source);
107107
}
108108

109109
if (!CharMatcher.anyOf("\r\n").matchesAnyOf(source)) {
110-
logger.debug(source); // $ SPURIOUS: Alert
110+
logger.debug(source);
111111
}
112112

113113
if (CharMatcher.anyOf("\r\n").matchesNoneOf(source)) {
114-
logger.debug(source); // $ SPURIOUS: Alert
114+
logger.debug(source);
115115
}
116116

117117
if (CharMatcher.noneOf("\r\n").matchesAllOf(source)) {
118-
logger.debug(source); // $ SPURIOUS: Alert
118+
logger.debug(source);
119119
}
120120
}
121121
}

‎java/ql/test/query-tests/security/CWE-117/LogInjectionTest.expected‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,6 @@
2828
| LibrarySanitizers.java:82:22:82:47 | removeFrom(...) | LibrarySanitizers.java:72:25:72:32 | source(...) : String | LibrarySanitizers.java:82:22:82:47 | removeFrom(...) | This log entry depends on a $@. | LibrarySanitizers.java:72:25:72:32 | source(...) | user-provided value |
2929
| LibrarySanitizers.java:94:26:94:31 | source | LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:94:26:94:31 | source | This log entry depends on a $@. | LibrarySanitizers.java:86:25:86:32 | source(...) | user-provided value |
3030
| LibrarySanitizers.java:98:26:98:31 | source | LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:98:26:98:31 | source | This log entry depends on a $@. | LibrarySanitizers.java:86:25:86:32 | source(...) | user-provided value |
31-
| LibrarySanitizers.java:102:26:102:31 | source | LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:102:26:102:31 | source | This log entry depends on a $@. | LibrarySanitizers.java:86:25:86:32 | source(...) | user-provided value |
32-
| LibrarySanitizers.java:106:26:106:31 | source | LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:106:26:106:31 | source | This log entry depends on a $@. | LibrarySanitizers.java:86:25:86:32 | source(...) | user-provided value |
33-
| LibrarySanitizers.java:110:26:110:31 | source | LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:110:26:110:31 | source | This log entry depends on a $@. | LibrarySanitizers.java:86:25:86:32 | source(...) | user-provided value |
34-
| LibrarySanitizers.java:114:26:114:31 | source | LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:114:26:114:31 | source | This log entry depends on a $@. | LibrarySanitizers.java:86:25:86:32 | source(...) | user-provided value |
35-
| LibrarySanitizers.java:118:26:118:31 | source | LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:118:26:118:31 | source | This log entry depends on a $@. | LibrarySanitizers.java:86:25:86:32 | source(...) | user-provided value |
3631
| LogInjectionTest.java:39:26:39:31 | source | LogInjectionTest.java:35:34:35:41 | source(...) : Object | LogInjectionTest.java:39:26:39:31 | source | This log entry depends on a $@. | LogInjectionTest.java:35:34:35:41 | source(...) | user-provided value |
3732
| LogInjectionTest.java:45:26:45:31 | source | LogInjectionTest.java:35:34:35:41 | source(...) : Object | LogInjectionTest.java:45:26:45:31 | source | This log entry depends on a $@. | LogInjectionTest.java:35:34:35:41 | source(...) | user-provided value |
3833
| LogInjectionTest.java:51:26:51:31 | source | LogInjectionTest.java:35:34:35:41 | source(...) : Object | LogInjectionTest.java:51:26:51:31 | source | This log entry depends on a $@. | LogInjectionTest.java:35:34:35:41 | source(...) | user-provided value |
@@ -1962,11 +1957,6 @@ edges
19621957
| LibrarySanitizers.java:82:41:82:46 | source : String | LibrarySanitizers.java:82:22:82:47 | removeFrom(...) | provenance | MaD:538 Sink:MaD:101 |
19631958
| LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:94:26:94:31 | source | provenance | Src:MaD:536 Sink:MaD:101 |
19641959
| LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:98:26:98:31 | source | provenance | Src:MaD:536 Sink:MaD:101 |
1965-
| LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:102:26:102:31 | source | provenance | Src:MaD:536 Sink:MaD:101 |
1966-
| LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:106:26:106:31 | source | provenance | Src:MaD:536 Sink:MaD:101 |
1967-
| LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:110:26:110:31 | source | provenance | Src:MaD:536 Sink:MaD:101 |
1968-
| LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:114:26:114:31 | source | provenance | Src:MaD:536 Sink:MaD:101 |
1969-
| LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:118:26:118:31 | source | provenance | Src:MaD:536 Sink:MaD:101 |
19701960
| LogInjectionTest.java:35:25:35:41 | (...)... : String | LogInjectionTest.java:39:26:39:31 | source | provenance | Sink:MaD:101 |
19711961
| LogInjectionTest.java:35:25:35:41 | (...)... : String | LogInjectionTest.java:45:26:45:31 | source | provenance | Sink:MaD:101 |
19721962
| LogInjectionTest.java:35:25:35:41 | (...)... : String | LogInjectionTest.java:51:26:51:31 | source | provenance | Sink:MaD:101 |
@@ -4520,11 +4510,6 @@ nodes
45204510
| LibrarySanitizers.java:86:25:86:32 | source(...) : String | semmle.label | source(...) : String |
45214511
| LibrarySanitizers.java:94:26:94:31 | source | semmle.label | source |
45224512
| LibrarySanitizers.java:98:26:98:31 | source | semmle.label | source |
4523-
| LibrarySanitizers.java:102:26:102:31 | source | semmle.label | source |
4524-
| LibrarySanitizers.java:106:26:106:31 | source | semmle.label | source |
4525-
| LibrarySanitizers.java:110:26:110:31 | source | semmle.label | source |
4526-
| LibrarySanitizers.java:114:26:114:31 | source | semmle.label | source |
4527-
| LibrarySanitizers.java:118:26:118:31 | source | semmle.label | source |
45284513
| LogInjectionTest.java:35:25:35:41 | (...)... : String | semmle.label | (...)... : String |
45294514
| LogInjectionTest.java:35:34:35:41 | source(...) : Object | semmle.label | source(...) : Object |
45304515
| LogInjectionTest.java:39:26:39:31 | source | semmle.label | source |

0 commit comments

Comments
 (0)