Skip to content

Commit c4d028d

Browse files
authored
Merge pull request #22835 from owen-mc/java/mad-barrier-log-injection
Java: Allow models-as-data barriers for log injection and improve log injection and path injection sanitizers
2 parents 532a278 + 94f4996 commit c4d028d

22 files changed

Lines changed: 7847 additions & 7109 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The `java/log-injection` query now recognizes additional sanitizers and sanitizer guards from the JDK, Apache Commons Lang and Text, Guava, OWASP Java Encoder, and Spring Framework.
5+
* Queries that use path-injection sanitizers now recognize additional substring guards and string replacement and removal operations from Apache Commons Lang and Guava.

‎java/ql/lib/ext/com.google.common.base.model.yml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@ extensions:
1818
- ["com.google.common.base", "Ascii", False, "truncate", "(CharSequence,int,String)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
1919
- ["com.google.common.base", "Ascii", False, "truncate", "(CharSequence,int,String)", "", "Argument[2]", "ReturnValue", "taint", "manual"]
2020
- ["com.google.common.base", "CaseFormat", True, "to", "(CaseFormat,String)", "", "Argument[1]", "ReturnValue", "taint", "manual"]
21+
- ["com.google.common.base", "CharMatcher", True, "removeFrom", "(CharSequence)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
22+
- ["com.google.common.base", "CharMatcher", True, "replaceFrom", "(CharSequence,char)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
23+
- ["com.google.common.base", "CharMatcher", True, "replaceFrom", "(CharSequence,char)", "", "Argument[1]", "ReturnValue", "taint", "manual"]
24+
- ["com.google.common.base", "CharMatcher", True, "replaceFrom", "(CharSequence,CharSequence)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
25+
- ["com.google.common.base", "CharMatcher", True, "replaceFrom", "(CharSequence,CharSequence)", "", "Argument[1]", "ReturnValue", "taint", "manual"]
2126
- ["com.google.common.base", "Converter", True, "apply", "(Object)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
2227
- ["com.google.common.base", "Converter", True, "convert", "(Object)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
2328
- ["com.google.common.base", "Converter", True, "convertAll", "(Iterable)", "", "Argument[0].Element", "ReturnValue.Element", "taint", "manual"]

‎java/ql/lib/ext/org.apache.commons.lang3.model.yml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,11 @@
11
extensions:
2+
- addsTo:
3+
pack: codeql/java-all
4+
extensible: barrierModel
5+
data:
6+
- ["org.apache.commons.lang3", "StringEscapeUtils", False, "escapeJava", "(String)", "", "ReturnValue", "log-injection", "manual"]
7+
- ["org.apache.commons.lang3", "StringUtils", False, "deleteWhitespace", "", "", "ReturnValue", "log-injection", "manual"]
8+
- ["org.apache.commons.lang3", "StringUtils", False, "normalizeSpace", "", "", "ReturnValue", "log-injection", "manual"]
29
- addsTo:
310
pack: codeql/java-all
411
extensible: sinkModel
@@ -75,6 +82,7 @@ extensions:
7582
- ["org.apache.commons.lang3", "RegExUtils", False, "replaceFirst", "", "", "Argument[2]", "ReturnValue", "taint", "manual"]
7683
- ["org.apache.commons.lang3", "RegExUtils", False, "replacePattern", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
7784
- ["org.apache.commons.lang3", "RegExUtils", False, "replacePattern", "", "", "Argument[2]", "ReturnValue", "taint", "manual"]
85+
- ["org.apache.commons.lang3", "StringEscapeUtils", False, "escapeJava", "(String)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
7886
- ["org.apache.commons.lang3", "StringEscapeUtils", False, "escapeJson", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
7987
- ["org.apache.commons.lang3", "StringUtils", False, "abbreviate", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
8088
- ["org.apache.commons.lang3", "StringUtils", False, "abbreviate", "(java.lang.String,java.lang.String,int)", "", "Argument[1]", "ReturnValue", "taint", "manual"]

‎java/ql/lib/ext/org.apache.commons.text.model.yml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,14 @@
11
extensions:
2+
- addsTo:
3+
pack: codeql/java-all
4+
extensible: barrierModel
5+
data:
6+
- ["org.apache.commons.text", "StringEscapeUtils", False, "escapeJava", "(String)", "", "ReturnValue", "log-injection", "manual"]
27
- addsTo:
38
pack: codeql/java-all
49
extensible: summaryModel
510
data:
11+
- ["org.apache.commons.text", "StringEscapeUtils", False, "escapeJava", "(String)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
612
- ["org.apache.commons.text", "StrBuilder", False, "StrBuilder", "(java.lang.String)", "", "Argument[0]", "Argument[this]", "taint", "manual"]
713
- ["org.apache.commons.text", "StrBuilder", False, "append", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
814
- ["org.apache.commons.text", "StrBuilder", False, "append", "", "", "Argument[this]", "ReturnValue", "value", "manual"]
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
extensions:
2+
- addsTo:
3+
pack: codeql/java-all
4+
extensible: barrierModel
5+
data:
6+
- ["org.owasp.encoder", "Encode", False, "forJava", "(String)", "", "ReturnValue", "log-injection", "manual"]
7+
- addsTo:
8+
pack: codeql/java-all
9+
extensible: summaryModel
10+
data:
11+
- ["org.owasp.encoder", "Encode", False, "forJava", "(String)", "", "Argument[0]", "ReturnValue", "taint", "manual"]

‎java/ql/lib/ext/org.springframework.util.model.yml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,9 @@
11
extensions:
2+
- addsTo:
3+
pack: codeql/java-all
4+
extensible: barrierModel
5+
data:
6+
- ["org.springframework.util", "StringUtils", False, "trimAllWhitespace", "", "", "ReturnValue", "log-injection", "manual"]
27
- addsTo:
38
pack: codeql/java-all
49
extensible: sinkModel

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

Lines changed: 207 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,13 @@ 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 {
1616
private import semmle.code.java.JDK
17+
private import semmle.code.java.frameworks.apache.Lang
18+
private import semmle.code.java.frameworks.guava.Guava
1719
private import semmle.code.java.frameworks.JavaxAnnotations
1820
}
1921

@@ -79,3 +81,207 @@ module RegexMatch {
7981
abstract string getName();
8082
}
8183
}
84+
85+
/**
86+
* An expression that tests whether an input string contains a substring.
87+
*
88+
* Extend this class to refine existing API models. If you want to model new APIs,
89+
* extend `StringSubstringMatch::Range` instead.
90+
*/
91+
class StringSubstringMatch extends Expr instanceof StringSubstringMatch::Range {
92+
/** Gets the input string being tested. */
93+
Expr getInputString() { result = super.getInputString() }
94+
95+
/** Gets the expression that specifies the substring. */
96+
Expr getMatchExpression() { result = super.getMatchExpression() }
97+
98+
/** Gets an expression to be sanitized. */
99+
Expr getASanitizedExpr() { result = [this.getInputString(), super.getAdditionalSanitizedExpr()] }
100+
101+
/** Holds if taking `branch` means that the input contains the substring. */
102+
predicate hasSubstringOnBranch(boolean branch) { super.hasSubstringOnBranch(branch) }
103+
104+
/** Gets a descriptive name for this substring match. */
105+
string getName() { result = super.getName() }
106+
}
107+
108+
/** Provides classes for modeling substring-matching APIs. */
109+
module StringSubstringMatch {
110+
/** A range class for APIs that test whether an input contains a substring. */
111+
abstract class Range extends Expr {
112+
/** Gets the input string being tested. */
113+
abstract Expr getInputString();
114+
115+
/** Gets the expression that specifies the substring. */
116+
abstract Expr getMatchExpression();
117+
118+
/** Gets an additional expression to be sanitized, if any. */
119+
Expr getAdditionalSanitizedExpr() { none() }
120+
121+
/** Holds if taking `branch` means that the input contains the substring. */
122+
abstract predicate hasSubstringOnBranch(boolean branch);
123+
124+
/** Gets a descriptive name for this substring match. */
125+
abstract string getName();
126+
}
127+
}
128+
129+
/**
130+
* An expression that tests whether an input string contains characters from a
131+
* character set.
132+
*
133+
* Extend this class to refine existing API models. If you want to model new APIs,
134+
* extend `StringCharacterSetMatch::Range` instead.
135+
*/
136+
class StringCharacterSetMatch extends Expr instanceof StringCharacterSetMatch::Range {
137+
/** Gets the input string being tested. */
138+
Expr getInputString() { result = super.getInputString() }
139+
140+
/** Gets the expression that specifies the character set. */
141+
Expr getMatchExpression() { result = super.getMatchExpression() }
142+
143+
/** Gets an expression to be sanitized. */
144+
Expr getASanitizedExpr() { result = [this.getInputString(), super.getAdditionalSanitizedExpr()] }
145+
146+
/**
147+
* Holds if taking `branch` means that the input contains at least one
148+
* character from the match expression.
149+
*/
150+
predicate hasMatchingCharacterOnBranch(boolean branch) {
151+
super.hasMatchingCharacterOnBranch(branch)
152+
}
153+
154+
/** Gets a descriptive name for this character-set match. */
155+
string getName() { result = super.getName() }
156+
}
157+
158+
/** Provides classes for modeling character-set matching APIs. */
159+
module StringCharacterSetMatch {
160+
/**
161+
* A range class for APIs that test whether an input contains characters from
162+
* a character set.
163+
*/
164+
abstract class Range extends Expr {
165+
/** Gets the input string being tested. */
166+
abstract Expr getInputString();
167+
168+
/** Gets the expression that specifies the character set. */
169+
abstract Expr getMatchExpression();
170+
171+
/** Gets an additional expression to be sanitized, if any. */
172+
Expr getAdditionalSanitizedExpr() { none() }
173+
174+
/**
175+
* Holds if taking `branch` means that the input contains at least one
176+
* character from the match expression.
177+
*/
178+
abstract predicate hasMatchingCharacterOnBranch(boolean branch);
179+
180+
/** Gets a descriptive name for this character-set match. */
181+
abstract string getName();
182+
}
183+
}
184+
185+
/**
186+
* An expression that replaces every occurrence of a target in a string.
187+
*
188+
* Extend this class to refine existing API models. If you want to model new APIs,
189+
* extend `StringReplacement::Range` instead.
190+
*/
191+
class StringReplacement extends Expr instanceof StringReplacement::Range {
192+
/** Gets the input string whose contents are replaced. */
193+
Expr getInputString() { result = super.getInputString() }
194+
195+
/** Gets the expression that identifies the content to replace. */
196+
Expr getMatchExpression() { result = super.getMatchExpression() }
197+
198+
/** Gets the replacement expression. */
199+
Expr getReplacement() { result = super.getReplacement() }
200+
201+
/** Holds if the match expression is interpreted as a regular expression. */
202+
predicate matchExpressionIsRegex() { super.matchExpressionIsRegex() }
203+
204+
/** Holds if each character in the match expression is matched independently. */
205+
predicate matchExpressionIsCharacterSet() { super.matchExpressionIsCharacterSet() }
206+
207+
/** Gets a descriptive name for this replacement operation. */
208+
string getName() { result = super.getName() }
209+
}
210+
211+
/** Provides classes for modeling string-replacement APIs. */
212+
module StringReplacement {
213+
/**
214+
* An expression that replaces every occurrence of a target in a string.
215+
*
216+
* Extend this class to model new APIs. If you want to refine existing API models,
217+
* extend `StringReplacement` instead.
218+
*/
219+
abstract class Range extends Expr {
220+
/** Gets the input string whose contents are replaced. */
221+
abstract Expr getInputString();
222+
223+
/** Gets the expression that identifies the content to replace. */
224+
abstract Expr getMatchExpression();
225+
226+
/** Gets the replacement expression. */
227+
abstract Expr getReplacement();
228+
229+
/** Holds if the match expression is interpreted as a regular expression. */
230+
predicate matchExpressionIsRegex() { none() }
231+
232+
/** Holds if each character in the match expression is matched independently. */
233+
predicate matchExpressionIsCharacterSet() { none() }
234+
235+
/** Gets a descriptive name for this replacement operation. */
236+
abstract string getName();
237+
}
238+
}
239+
240+
/**
241+
* An expression that removes every occurrence of a target from a string.
242+
*
243+
* Extend this class to refine existing API models. If you want to model new APIs,
244+
* extend `StringRemoval::Range` instead.
245+
*/
246+
class StringRemoval extends Expr instanceof StringRemoval::Range {
247+
/** Gets the input string whose contents are removed. */
248+
Expr getInputString() { result = super.getInputString() }
249+
250+
/** Gets the expression that identifies the content to remove. */
251+
Expr getMatchExpression() { result = super.getMatchExpression() }
252+
253+
/** Holds if the match expression is interpreted as a regular expression. */
254+
predicate matchExpressionIsRegex() { super.matchExpressionIsRegex() }
255+
256+
/** Holds if each character in the match expression is matched independently. */
257+
predicate matchExpressionIsCharacterSet() { super.matchExpressionIsCharacterSet() }
258+
259+
/** Gets a descriptive name for this removal operation. */
260+
string getName() { result = super.getName() }
261+
}
262+
263+
/** Provides classes for modeling string-removal APIs. */
264+
module StringRemoval {
265+
/**
266+
* An expression that removes every occurrence of a target from a string.
267+
*
268+
* Extend this class to model new APIs. If you want to refine existing API models,
269+
* extend `StringRemoval` instead.
270+
*/
271+
abstract class Range extends Expr {
272+
/** Gets the input string whose contents are removed. */
273+
abstract Expr getInputString();
274+
275+
/** Gets the expression that identifies the content to remove. */
276+
abstract Expr getMatchExpression();
277+
278+
/** Holds if the match expression is interpreted as a regular expression. */
279+
predicate matchExpressionIsRegex() { none() }
280+
281+
/** Holds if each character in the match expression is matched independently. */
282+
predicate matchExpressionIsCharacterSet() { none() }
283+
284+
/** Gets a descriptive name for this removal operation. */
285+
abstract string getName();
286+
}
287+
}

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,17 @@ class CompileTimeConstantExpr extends Expr {
205205
result = this.(LiveLiteral).getValue().getStringValue()
206206
}
207207

208+
/**
209+
* Gets the string value of this expression if it is a `String` constant, or
210+
* the single-character string value if it is a `char` constant.
211+
*/
212+
string getStringOrCharValue() {
213+
result = this.getStringValue()
214+
or
215+
this.getType() instanceof CharacterType and
216+
result = this.getIntValue().toUnicode()
217+
}
218+
208219
/**
209220
* Gets the boolean value of this expression, where possible.
210221
*/

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

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,19 @@ class StringContainsMethod extends Method {
4747
}
4848
}
4949

50+
/** A call to the `java.lang.String.contains` method. */
51+
class StringContainsCall extends MethodCall, StringSubstringMatch::Range {
52+
StringContainsCall() { this.getMethod() instanceof StringContainsMethod }
53+
54+
override Expr getInputString() { result = this.getQualifier() }
55+
56+
override Expr getMatchExpression() { result = this.getArgument(0) }
57+
58+
override predicate hasSubstringOnBranch(boolean branch) { branch = true }
59+
60+
override string getName() { result = "String.contains" }
61+
}
62+
5063
/** A call to the `java.lang.String.matches` method. */
5164
class StringMatchesCall extends MethodCall, RegexMatch::Range {
5265
StringMatchesCall() {
@@ -64,23 +77,41 @@ class StringMatchesCall extends MethodCall, RegexMatch::Range {
6477
}
6578

6679
/** A call to the `java.lang.String.replaceAll` method. */
67-
class StringReplaceAllCall extends MethodCall {
80+
class StringReplaceAllCall extends MethodCall, StringReplacement::Range {
6881
StringReplaceAllCall() {
6982
exists(Method m | m = this.getMethod() |
7083
m.getDeclaringType() instanceof TypeString and
7184
m.hasName("replaceAll")
7285
)
7386
}
87+
88+
override Expr getInputString() { result = this.getQualifier() }
89+
90+
override Expr getMatchExpression() { result = this.getArgument(0) }
91+
92+
override Expr getReplacement() { result = this.getArgument(1) }
93+
94+
override predicate matchExpressionIsRegex() { any() }
95+
96+
override string getName() { result = "String.replaceAll" }
7497
}
7598

7699
/** A call to the `java.lang.String.replace` method. */
77-
class StringReplaceCall extends MethodCall {
100+
class StringReplaceCall extends MethodCall, StringReplacement::Range {
78101
StringReplaceCall() {
79102
exists(Method m | m = this.getMethod() |
80103
m.getDeclaringType() instanceof TypeString and
81104
m.hasName("replace")
82105
)
83106
}
107+
108+
override Expr getInputString() { result = this.getQualifier() }
109+
110+
override Expr getMatchExpression() { result = this.getArgument(0) }
111+
112+
override Expr getReplacement() { result = this.getArgument(1) }
113+
114+
override string getName() { result = "String.replace" }
84115
}
85116

86117
/**

0 commit comments

Comments
 (0)