Skip to content

Commit 0d0b69e

Browse files
aschackmullaschackmull
authored
Merge pull request github#16835 from aschackmull/java/proper-clone-model
Java: Replace the MaD Object.clone() models with a non-aliasing value step.
2 parents 0fb27fb + 37d7824 commit 0d0b69e

File tree

56 files changed

+2361
-2352
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

56 files changed

+2361
-2352
lines changed

‎java/ql/lib/ext/java.lang.model.yml

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,9 +91,6 @@ extensions:
9191
- ["java.lang", "Iterable", True, "iterator", "()", "", "Argument[this].Element", "ReturnValue.Element", "value", "manual"]
9292
- ["java.lang", "Iterable", True, "spliterator", "()", "", "Argument[this].Element", "ReturnValue.Element", "value", "manual"]
9393
- ["java.lang", "NullPointerException", False, "NullPointerException", "(String)", "", "Argument[0]", "Argument[this].SyntheticField[java.lang.Throwable.message]", "value", "manual"]
94-
- ["java.lang", "Object", True, "clone", "", "", "Argument[this].Element", "ReturnValue.Element", "value", "manual"]
95-
- ["java.lang", "Object", True, "clone", "", "", "Argument[this].MapKey", "ReturnValue.MapKey", "value", "manual"]
96-
- ["java.lang", "Object", True, "clone", "", "", "Argument[this].MapValue", "ReturnValue.MapValue", "value", "manual"]
9794
- ["java.lang", "RuntimeException", False, "RuntimeException", "(String)", "", "Argument[0]", "Argument[this].SyntheticField[java.lang.Throwable.message]", "value", "manual"]
9895
- ["java.lang", "RuntimeException", False, "RuntimeException", "(String,Throwable)", "", "Argument[0]", "Argument[this].SyntheticField[java.lang.Throwable.message]", "value", "manual"]
9996
- ["java.lang", "RuntimeException", False, "RuntimeException", "(String,Throwable)", "", "Argument[1]", "Argument[this].SyntheticField[java.lang.Throwable.cause]", "value", "manual"]

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ module JavaDataFlow implements InputSig<Location> {
2222

2323
predicate getSecondLevelScope = Private::getSecondLevelScope/1;
2424

25+
predicate validParameterAliasStep = Private::validParameterAliasStep/2;
26+
2527
predicate mayBenefitFromCallContext = Private::mayBenefitFromCallContext/1;
2628

2729
predicate viableImplInCallContext = Private::viableImplInCallContext/2;

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -400,6 +400,18 @@ class CastNode extends ExprNode {
400400
}
401401
}
402402

403+
/** Holds if `n1` is the qualifier of a call to `clone()` and `n2` is the result. */
404+
predicate cloneStep(Node n1, Node n2) {
405+
exists(MethodCall mc |
406+
mc.getMethod() instanceof CloneMethod and
407+
n1 = getInstanceArgument(mc) and
408+
n2.asExpr() = mc
409+
)
410+
}
411+
412+
bindingset[node1, node2]
413+
predicate validParameterAliasStep(Node node1, Node node2) { not cloneStep(node1, node2) }
414+
403415
private newtype TDataFlowCallable =
404416
TSrcCallable(Callable c) or
405417
TSummarizedCallable(SummarizedCallable c) or

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,8 @@ private predicate simpleLocalFlowStep0(Node node1, Node node2, string model) {
258258
model = "ValuePreservingMethod"
259259
)
260260
or
261+
cloneStep(node1, node2) and model = "CloneStep"
262+
or
261263
FlowSummaryImpl::Private::Steps::summaryLocalStep(node1.(FlowSummaryNode).getSummaryNode(),
262264
node2.(FlowSummaryNode).getSummaryNode(), true, model)
263265
}

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -316,9 +316,6 @@ private predicate qualifierToMethodStep(Expr tracked, MethodCall sink, string mo
316316
* Methods that return tainted data when called on tainted data.
317317
*/
318318
private predicate taintPreservingQualifierToMethod(Method m, string model) {
319-
model = "" and
320-
m instanceof CloneMethod
321-
or
322319
model = "%StringWriter" and
323320
m.getDeclaringType().getQualifiedName().matches("%StringWriter") and
324321
(

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,12 @@ class MethodLdapNameAddAll extends Method {
4444
}
4545
}
4646

47-
/** A method with the name `clone` declared in `javax.naming.ldap.LdapName`. */
48-
class MethodLdapNameClone extends Method {
47+
/**
48+
* DEPRECATED: No longer needed as clone steps are handled uniformly.
49+
*
50+
* A method with the name `clone` declared in `javax.naming.ldap.LdapName`.
51+
*/
52+
deprecated class MethodLdapNameClone extends Method {
4953
MethodLdapNameClone() {
5054
this.getDeclaringType() instanceof TypeLdapName and
5155
this.hasName("clone")

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ private predicate ldapNameAddAllStep(DataFlow::ExprNode n1, DataFlow::ExprNode n
6262

6363
/**
6464
* Holds if `n1` to `n2` is a dataflow step that converts between `LdapName` and `LdapName` or
65-
* `String`, i.e. `taintedLdapName.clone()`, `taintedLdapName.getAll()`,
65+
* `String`, i.e. `taintedLdapName.getAll()`,
6666
* `taintedLdapName.getRdns()` or `taintedLdapName.toString()`.
6767
*/
6868
private predicate ldapNameGetCloneStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
@@ -71,7 +71,6 @@ private predicate ldapNameGetCloneStep(DataFlow::ExprNode n1, DataFlow::ExprNode
7171
n2.asExpr() = ma and
7272
ma.getMethod() = m
7373
|
74-
m instanceof MethodLdapNameClone or
7574
m instanceof MethodLdapNameGetAll or
7675
m instanceof MethodLdapNameGetRdns or
7776
m instanceof MethodLdapNameToString

‎java/ql/test/experimental/query-tests/security/CWE-020/Log4jInjectionTest.expected

Lines changed: 2087 additions & 2087 deletions
Large diffs are not rendered by default.

‎java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ edges
33
| FilePathInjection.java:64:21:64:34 | getPara(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath | provenance | Src:MaD:1972 AdditionalValueStep Sink:MaD:42557 |
44
| FilePathInjection.java:87:21:87:34 | getPara(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath | provenance | Src:MaD:1972 AdditionalValueStep Sink:MaD:42557 |
55
| FilePathInjection.java:177:50:177:58 | file : File | FilePathInjection.java:182:30:182:33 | file | provenance | Sink:MaD:42554 |
6-
| FilePathInjection.java:205:17:205:44 | getParameter(...) : String | FilePathInjection.java:209:24:209:31 | filePath : String | provenance | Src:MaD:44687 |
6+
| FilePathInjection.java:205:17:205:44 | getParameter(...) : String | FilePathInjection.java:209:24:209:31 | filePath : String | provenance | Src:MaD:44684 |
77
| FilePathInjection.java:209:15:209:32 | new File(...) : File | FilePathInjection.java:210:23:210:26 | file | provenance | Sink:MaD:42541 |
88
| FilePathInjection.java:209:15:209:32 | new File(...) : File | FilePathInjection.java:217:19:217:22 | file : File | provenance | |
99
| FilePathInjection.java:209:24:209:31 | filePath : String | FilePathInjection.java:209:15:209:32 | new File(...) : File | provenance | MaD:42613 |

‎java/ql/test/experimental/query-tests/security/CWE-078/CommandInjectionRuntimeExecLocal.expected

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,10 @@ edges
99
| RuntimeExecTest.java:25:66:25:71 | script : String | RuntimeExecTest.java:25:42:25:72 | {...} : String[] [[]] : String | provenance | |
1010
| RuntimeExecTest.java:31:17:31:29 | commandArray2 [post update] : String[] [[]] : String | RuntimeExecTest.java:32:43:32:55 | commandArray2 | provenance | Sink:MaD:42686 |
1111
| RuntimeExecTest.java:31:36:31:41 | script : String | RuntimeExecTest.java:31:17:31:29 | commandArray2 [post update] : String[] [[]] : String | provenance | |
12-
| RuntimeExecTest.java:36:21:39:21 | concat(...) : Stream [<element>] : String | RuntimeExecTest.java:36:21:39:44 | toArray(...) : String[] [[]] : String | provenance | MaD:44369 |
12+
| RuntimeExecTest.java:36:21:39:21 | concat(...) : Stream [<element>] : String | RuntimeExecTest.java:36:21:39:44 | toArray(...) : String[] [[]] : String | provenance | MaD:44366 |
1313
| RuntimeExecTest.java:36:21:39:44 | toArray(...) : String[] [[]] : String | RuntimeExecTest.java:36:21:39:44 | toArray(...) | provenance | Sink:MaD:42686 |
14-
| RuntimeExecTest.java:38:25:38:59 | stream(...) : Stream [<element>] : String | RuntimeExecTest.java:36:21:39:21 | concat(...) : Stream [<element>] : String | provenance | MaD:44304 |
15-
| RuntimeExecTest.java:38:39:38:58 | new String[] : String[] [[]] : String | RuntimeExecTest.java:38:25:38:59 | stream(...) : Stream [<element>] : String | provenance | MaD:43738 |
14+
| RuntimeExecTest.java:38:25:38:59 | stream(...) : Stream [<element>] : String | RuntimeExecTest.java:36:21:39:21 | concat(...) : Stream [<element>] : String | provenance | MaD:44301 |
15+
| RuntimeExecTest.java:38:39:38:58 | new String[] : String[] [[]] : String | RuntimeExecTest.java:38:25:38:59 | stream(...) : Stream [<element>] : String | provenance | MaD:43735 |
1616
| RuntimeExecTest.java:38:39:38:58 | {...} : String[] [[]] : String | RuntimeExecTest.java:38:39:38:58 | new String[] : String[] [[]] : String | provenance | |
1717
| RuntimeExecTest.java:38:52:38:57 | script : String | RuntimeExecTest.java:38:39:38:58 | {...} : String[] [[]] : String | provenance | |
1818
nodes

0 commit comments

Comments
 (0)