Skip to content

Commit 6b6172f

Browse files
lcarteylcartey
committed
Java: ExternalAPIs: Further review comments
 - Extra qldoc - Remove unnecessary module
1 parent e1d4b98 commit 6b6172f

File tree

3 files changed

+125
-125
lines changed

3 files changed

+125
-125
lines changed

‎java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*/
99

1010
import java
11-
import semmle.code.java.security.ExternalAPIs::ExternalAPIs
11+
import semmle.code.java.security.ExternalAPIs
1212
import semmle.code.java.dataflow.DataFlow
1313

1414
from ExternalAPIUsedWithUntrustedData externalAPI

‎java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
import java
1212
import semmle.code.java.dataflow.FlowSources
1313
import semmle.code.java.dataflow.TaintTracking
14-
import semmle.code.java.security.ExternalAPIs::ExternalAPIs
14+
import semmle.code.java.security.ExternalAPIs
1515
import DataFlow::PathGraph
1616

1717
from UntrustedDataToExternalAPIConfig config, DataFlow::PathNode source, DataFlow::PathNode sink

‎java/ql/src/semmle/code/java/security/ExternalAPIs.qll

Lines changed: 123 additions & 123 deletions
Original file line numberDiff line numberDiff line change
@@ -7,140 +7,140 @@ import java
77
import semmle.code.java.dataflow.FlowSources
88
import semmle.code.java.dataflow.TaintTracking
99

10-
module ExternalAPIs {
11-
/**
12-
* A `Method` that is considered a "safe" external API from a security perspective.
13-
*/
14-
abstract class SafeExternalAPIMethod extends Method { }
15-
16-
/** The default set of "safe" external APIs. */
17-
class DefaultSafeExternalAPIMethod extends SafeExternalAPIMethod {
18-
DefaultSafeExternalAPIMethod() {
19-
this instanceof EqualsMethod
20-
or
21-
getName().regexpMatch("size|length|compareTo|getClass|lastIndexOf")
22-
or
23-
this.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "Validate")
24-
or
25-
getQualifiedName() = "Objects.equals"
26-
or
27-
getDeclaringType() instanceof TypeString and getName() = "equals"
28-
or
29-
getDeclaringType().hasQualifiedName("com.google.common.base", "Preconditions")
30-
or
31-
getDeclaringType().getPackage().getName().matches("org.junit%")
32-
or
33-
getDeclaringType().hasQualifiedName("com.google.common.base", "Strings") and
34-
getName() = "isNullOrEmpty"
35-
or
36-
getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "StringUtils") and
37-
getName() = "isNotEmpty"
38-
or
39-
getDeclaringType().hasQualifiedName("java.lang", "Character") and
40-
getName() = "isDigit"
41-
or
42-
getDeclaringType().hasQualifiedName("java.lang", "String") and
43-
getName().regexpMatch("equalsIgnoreCase|regionMatches")
44-
or
45-
getDeclaringType().hasQualifiedName("java.lang", "Boolean") and
46-
getName() = "parseBoolean"
47-
or
48-
getDeclaringType().hasQualifiedName("org.apache.commons.io", "IOUtils") and
49-
getName() = "closeQuietly"
10+
/**
11+
* A `Method` that is considered a "safe" external API from a security perspective.
12+
*/
13+
abstract class SafeExternalAPIMethod extends Method { }
14+
15+
/** The default set of "safe" external APIs. */
16+
private class DefaultSafeExternalAPIMethod extends SafeExternalAPIMethod {
17+
DefaultSafeExternalAPIMethod() {
18+
this instanceof EqualsMethod
19+
or
20+
getName().regexpMatch("size|length|compareTo|getClass|lastIndexOf")
21+
or
22+
this.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "Validate")
23+
or
24+
getQualifiedName() = "Objects.equals"
25+
or
26+
getDeclaringType() instanceof TypeString and getName() = "equals"
27+
or
28+
getDeclaringType().hasQualifiedName("com.google.common.base", "Preconditions")
29+
or
30+
getDeclaringType().getPackage().getName().matches("org.junit%")
31+
or
32+
getDeclaringType().hasQualifiedName("com.google.common.base", "Strings") and
33+
getName() = "isNullOrEmpty"
34+
or
35+
getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "StringUtils") and
36+
getName() = "isNotEmpty"
37+
or
38+
getDeclaringType().hasQualifiedName("java.lang", "Character") and
39+
getName() = "isDigit"
40+
or
41+
getDeclaringType().hasQualifiedName("java.lang", "String") and
42+
getName().regexpMatch("equalsIgnoreCase|regionMatches")
43+
or
44+
getDeclaringType().hasQualifiedName("java.lang", "Boolean") and
45+
getName() = "parseBoolean"
46+
or
47+
getDeclaringType().hasQualifiedName("org.apache.commons.io", "IOUtils") and
48+
getName() = "closeQuietly"
49+
or
50+
getDeclaringType().hasQualifiedName("org.springframework.util", "StringUtils") and
51+
getName().regexpMatch("hasText|isEmpty")
52+
}
53+
}
54+
55+
/** A node representing data being passed to an external API. */
56+
class ExternalAPIDataNode extends DataFlow::Node {
57+
Call call;
58+
int i;
59+
60+
ExternalAPIDataNode() {
61+
(
62+
// Argument to call to a method
63+
this.asExpr() = call.getArgument(i)
5064
or
51-
getDeclaringType().hasQualifiedName("org.springframework.util", "StringUtils") and
52-
getName().regexpMatch("hasText|isEmpty")
53-
}
65+
// Qualifier to call to a method which returns non trivial value
66+
this.asExpr() = call.getQualifier() and
67+
i = -1 and
68+
not call.getCallee().getReturnType() instanceof VoidType and
69+
not call.getCallee().getReturnType() instanceof BooleanType
70+
) and
71+
// Defined outside the source archive
72+
not call.getCallee().fromSource() and
73+
// Not a call to an method which is overridden in source
74+
not exists(Method m |
75+
m.getASourceOverriddenMethod() = call.getCallee().getSourceDeclaration() and
76+
m.fromSource()
77+
) and
78+
// Not already modeled as a taint step
79+
not exists(DataFlow::Node next | TaintTracking::localTaintStep(this, next)) and
80+
// Not a call to a known safe external API
81+
not call.getCallee() instanceof SafeExternalAPIMethod
5482
}
5583

56-
/** A node representing data being passed to an external API. */
57-
class ExternalAPIDataNode extends DataFlow::Node {
58-
Call call;
59-
int i;
60-
61-
ExternalAPIDataNode() {
62-
(
63-
// Argument to call to a method
64-
this.asExpr() = call.getArgument(i)
65-
or
66-
// Qualifier to call to a method which returns non trivial value
67-
this.asExpr() = call.getQualifier() and
68-
i = -1 and
69-
not call.getCallee().getReturnType() instanceof VoidType and
70-
not call.getCallee().getReturnType() instanceof BooleanType
71-
) and
72-
// Defined outside the source archive
73-
not call.getCallee().fromSource() and
74-
// Not a call to an method which is overridden in source
75-
not exists(Method m |
76-
m.getASourceOverriddenMethod() = call.getCallee().getSourceDeclaration() and
77-
m.fromSource()
78-
) and
79-
// Not already modeled as a taint step
80-
not exists(DataFlow::Node next | TaintTracking::localTaintStep(this, next)) and
81-
// Not a call to a known safe external API
82-
not call.getCallee() instanceof SafeExternalAPIMethod
83-
}
84-
85-
/** Gets the called API `Method`. */
86-
Method getMethod() { result = call.getCallee() }
87-
88-
/** Gets the index which is passed untrusted data (where -1 indicates the qualifier). */
89-
int getIndex() { result = i }
90-
91-
/** Gets the description of the method being called. */
92-
string getMethodDescription() {
93-
result =
94-
getMethod().getDeclaringType().getPackage() + "." + getMethod().getQualifiedName()
95-
}
84+
/** Gets the called API `Method`. */
85+
Method getMethod() { result = call.getCallee() }
86+
87+
/** Gets the index which is passed untrusted data (where -1 indicates the qualifier). */
88+
int getIndex() { result = i }
89+
90+
/** Gets the description of the method being called. */
91+
string getMethodDescription() {
92+
result = getMethod().getDeclaringType().getPackage() + "." + getMethod().getQualifiedName()
9693
}
94+
}
9795

98-
class UntrustedDataToExternalAPIConfig extends TaintTracking::Configuration {
99-
UntrustedDataToExternalAPIConfig() { this = "UntrustedDataToExternalAPIConfig" }
96+
/** A configuration for tracking flow from `RemoteFlowSource`s to `ExternalAPIDataNode`s. */
97+
class UntrustedDataToExternalAPIConfig extends TaintTracking::Configuration {
98+
UntrustedDataToExternalAPIConfig() { this = "UntrustedDataToExternalAPIConfig" }
10099

101-
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
100+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
102101

103-
override predicate isSink(DataFlow::Node sink) { sink instanceof ExternalAPIDataNode }
102+
override predicate isSink(DataFlow::Node sink) { sink instanceof ExternalAPIDataNode }
103+
}
104+
105+
/** A node representing untrusted data being passed to an external API. */
106+
class UntrustedExternalAPIDataNode extends ExternalAPIDataNode {
107+
UntrustedExternalAPIDataNode() { any(UntrustedDataToExternalAPIConfig c).hasFlow(_, this) }
108+
109+
/** Gets a source of untrusted data which is passed to this external API data node. */
110+
DataFlow::Node getAnUntrustedSource() {
111+
any(UntrustedDataToExternalAPIConfig c).hasFlow(result, this)
104112
}
113+
}
105114

106-
/** A node representing untrusted data being passed to an external API. */
107-
class UntrustedExternalAPIDataNode extends ExternalAPIDataNode {
108-
UntrustedExternalAPIDataNode() { any(UntrustedDataToExternalAPIConfig c).hasFlow(_, this) }
115+
private newtype TExternalAPI =
116+
TExternalAPIParameter(Method m, int index) {
117+
exists(UntrustedExternalAPIDataNode n |
118+
m = n.getMethod() and
119+
index = n.getIndex()
120+
)
121+
}
122+
123+
/** An external API which is used with untrusted data. */
124+
class ExternalAPIUsedWithUntrustedData extends TExternalAPI {
125+
/** Gets a possibly untrusted use of this external API. */
126+
UntrustedExternalAPIDataNode getUntrustedDataNode() {
127+
this = TExternalAPIParameter(result.getMethod(), result.getIndex())
128+
}
109129

110-
/** Gets a source of untrusted data which is passed to this external API data node. */
111-
DataFlow::Node getAnUntrustedSource() {
112-
any(UntrustedDataToExternalAPIConfig c).hasFlow(result, this)
113-
}
130+
/** Gets the number of untrusted sources used with this external API. */
131+
int getNumberOfUntrustedSources() {
132+
result = count(getUntrustedDataNode().getAnUntrustedSource())
114133
}
115134

116-
private newtype TExternalAPI =
117-
TExternalAPIParameter(Method m, int index) {
118-
exists(UntrustedExternalAPIDataNode n |
119-
m = n.getMethod() and
120-
index = n.getIndex()
121-
)
122-
}
123-
124-
/** An external API which is used with untrusted data. */
125-
class ExternalAPIUsedWithUntrustedData extends TExternalAPI {
126-
/** Gets a possibly untrusted use of this external API. */
127-
UntrustedExternalAPIDataNode getUntrustedDataNode() {
128-
this = TExternalAPIParameter(result.getMethod(), result.getIndex())
129-
}
130-
131-
/** Gets the number of untrusted sources used with this external API. */
132-
int getNumberOfUntrustedSources() {
133-
result = count(getUntrustedDataNode().getAnUntrustedSource())
134-
}
135-
136-
string toString() {
137-
exists(Method m, int index, string indexString |
138-
if index = -1 then indexString = "qualifier" else indexString = "param " + index
139-
|
140-
this = TExternalAPIParameter(m, index) and
141-
result =
142-
m.getDeclaringType().(RefType).getQualifiedName() + "." + m.getSignature() + " [" + indexString + "]"
143-
)
144-
}
135+
/** Gets a textual representation of this element. */
136+
string toString() {
137+
exists(Method m, int index, string indexString |
138+
if index = -1 then indexString = "qualifier" else indexString = "param " + index
139+
|
140+
this = TExternalAPIParameter(m, index) and
141+
result =
142+
m.getDeclaringType().(RefType).getQualifiedName() + "." + m.getSignature() + " [" +
143+
indexString + "]"
144+
)
145145
}
146146
}

0 commit comments

Comments
 (0)