Skip to content

Commit 54fbda0

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

6 files changed

Lines changed: 96 additions & 16 deletions

File tree

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

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,50 @@ module RegexMatch {
8282
}
8383
}
8484

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+
85129
/**
86130
* An expression that tests whether an input string contains characters from a
87131
* character set.

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

Lines changed: 13 additions & 0 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() {

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

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,13 +48,43 @@ class TypeApacheSystemUtils extends Class {
4848
}
4949
}
5050

51+
private predicate isStringLike(Type type) {
52+
type.(RefType).hasQualifiedName("java.lang", ["String", "CharSequence"])
53+
}
54+
55+
private predicate isCharacterSetType(Type type) {
56+
isStringLike(type)
57+
or
58+
type.(Array).getElementType().(PrimitiveType).hasName("char")
59+
}
60+
61+
/** A call to `StringUtils.contains` or `StringUtils.containsIgnoreCase`. */
62+
private class ApacheStringUtilsSubstringMatch extends MethodCall, StringSubstringMatch::Range {
63+
ApacheStringUtilsSubstringMatch() {
64+
this.getMethod()
65+
.getDeclaringType()
66+
.hasQualifiedName(["org.apache.commons.lang", "org.apache.commons.lang3"], "StringUtils") and
67+
this.getMethod().hasName(["contains", "containsIgnoreCase"]) and
68+
isStringLike(this.getMethod().getParameterType(1))
69+
}
70+
71+
override Expr getInputString() { result = this.getArgument(0) }
72+
73+
override Expr getMatchExpression() { result = this.getArgument(1) }
74+
75+
override predicate hasSubstringOnBranch(boolean branch) { branch = true }
76+
77+
override string getName() { result = "StringUtils." + this.getMethod().getName() }
78+
}
79+
5180
/** A call to `StringUtils.containsAny` or `StringUtils.containsNone`. */
5281
private class ApacheStringUtilsCharacterSetMatch extends MethodCall, StringCharacterSetMatch::Range {
5382
ApacheStringUtilsCharacterSetMatch() {
5483
this.getMethod()
5584
.getDeclaringType()
5685
.hasQualifiedName(["org.apache.commons.lang", "org.apache.commons.lang3"], "StringUtils") and
57-
this.getMethod().hasName(["containsAny", "containsNone"])
86+
this.getMethod().hasName(["containsAny", "containsNone"]) and
87+
isCharacterSetType(this.getMethod().getParameterType(1))
5888
}
5989

6090
override Expr getInputString() { result = this.getArgument(0) }

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -119,11 +119,11 @@ private predicate logInjectionSanitizer(Expr e) {
119119
* by checking if there are line breaks in `e`.
120120
*/
121121
private predicate logInjectionGuard(Guard g, Expr e, boolean branch) {
122-
exists(MethodCall mc | mc = g |
123-
mc.getMethod() instanceof StringContainsMethod and
124-
mc.getArgument(0).(CompileTimeConstantExpr).getStringValue() = ["\n", "\r"] and
125-
e = mc.getQualifier() and
126-
branch = false
122+
exists(StringSubstringMatch match, boolean matchingBranch | match = g |
123+
match.getMatchExpression().(CompileTimeConstantExpr).getStringValue() = ["\n", "\r"] and
124+
e = match.getASanitizedExpr() and
125+
match.hasSubstringOnBranch(matchingBranch) and
126+
branch = matchingBranch.booleanNot()
127127
)
128128
or
129129
exists(StringCharacterSetMatch match, CompileTimeConstantExpr characters, boolean matchingBranch |

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -83,19 +83,19 @@ public void testAmbiguousGuavaMatcher(boolean condition) {
8383
}
8484

8585
public void testGuards() {
86-
String source = source(); // $ Source
86+
String source = source();
8787
Logger logger = null;
8888

8989
if (!source.contains("\n")) {
9090
logger.debug(source);
9191
}
9292

9393
if (!org.apache.commons.lang3.StringUtils.contains(source, "\n")) {
94-
logger.debug(source); // $ SPURIOUS: Alert
94+
logger.debug(source);
9595
}
9696

9797
if (!org.apache.commons.lang3.StringUtils.containsIgnoreCase(source, "\n")) {
98-
logger.debug(source); // $ SPURIOUS: Alert
98+
logger.debug(source);
9999
}
100100

101101
if (!org.apache.commons.lang3.StringUtils.containsAny(source, "\r\n")) {

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

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,6 @@
2626
| LibrarySanitizers.java:49:22:49:57 | replaceAll(...) | LibrarySanitizers.java:12:25:12:32 | source(...) : String | LibrarySanitizers.java:49:22:49:57 | replaceAll(...) | This log entry depends on a $@. | LibrarySanitizers.java:12:25:12:32 | source(...) | user-provided value |
2727
| LibrarySanitizers.java:50:22:50:58 | replaceAll(...) | LibrarySanitizers.java:12:25:12:32 | source(...) : String | LibrarySanitizers.java:50:22:50:58 | replaceAll(...) | This log entry depends on a $@. | LibrarySanitizers.java:12:25:12:32 | source(...) | user-provided value |
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 |
29-
| 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 |
30-
| 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 |
3129
| 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 |
3230
| 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 |
3331
| 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 |
@@ -1955,8 +1953,6 @@ edges
19551953
| LibrarySanitizers.java:50:22:50:27 | source : String | LibrarySanitizers.java:50:22:50:58 | replaceAll(...) | provenance | MaD:541 Sink:MaD:101 |
19561954
| LibrarySanitizers.java:72:25:72:32 | source(...) : String | LibrarySanitizers.java:82:41:82:46 | source : String | provenance | Src:MaD:536 |
19571955
| LibrarySanitizers.java:82:41:82:46 | source : String | LibrarySanitizers.java:82:22:82:47 | removeFrom(...) | provenance | MaD:538 Sink:MaD:101 |
1958-
| LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:94:26:94:31 | source | provenance | Src:MaD:536 Sink:MaD:101 |
1959-
| LibrarySanitizers.java:86:25:86:32 | source(...) : String | LibrarySanitizers.java:98:26:98:31 | source | provenance | Src:MaD:536 Sink:MaD:101 |
19601956
| LogInjectionTest.java:35:25:35:41 | (...)... : String | LogInjectionTest.java:39:26:39:31 | source | provenance | Sink:MaD:101 |
19611957
| LogInjectionTest.java:35:25:35:41 | (...)... : String | LogInjectionTest.java:45:26:45:31 | source | provenance | Sink:MaD:101 |
19621958
| LogInjectionTest.java:35:25:35:41 | (...)... : String | LogInjectionTest.java:51:26:51:31 | source | provenance | Sink:MaD:101 |
@@ -4507,9 +4503,6 @@ nodes
45074503
| LibrarySanitizers.java:72:25:72:32 | source(...) : String | semmle.label | source(...) : String |
45084504
| LibrarySanitizers.java:82:22:82:47 | removeFrom(...) | semmle.label | removeFrom(...) |
45094505
| LibrarySanitizers.java:82:41:82:46 | source : String | semmle.label | source : String |
4510-
| LibrarySanitizers.java:86:25:86:32 | source(...) : String | semmle.label | source(...) : String |
4511-
| LibrarySanitizers.java:94:26:94:31 | source | semmle.label | source |
4512-
| LibrarySanitizers.java:98:26:98:31 | source | semmle.label | source |
45134506
| LogInjectionTest.java:35:25:35:41 | (...)... : String | semmle.label | (...)... : String |
45144507
| LogInjectionTest.java:35:34:35:41 | source(...) : Object | semmle.label | source(...) : Object |
45154508
| LogInjectionTest.java:39:26:39:31 | source | semmle.label | source |

0 commit comments

Comments
 (0)