diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index b0d2c05a4e6f7..547f428ef88bb 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -11,6 +11,9 @@ import java import semmle.code.java.controlflow.Guards import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.TempFileLib +import semmle.code.java.dataflow.DataFlow3 +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.dataflow.TaintTracking2 import DataFlow::PathGraph /** @@ -31,33 +34,41 @@ predicate isDeleteFileExpr(Expr expr) { expr = any(MethodFileDelete m).getAReference().getQualifier() } -private class TempDirHijackingToDeleteConfig extends TaintTracking::Configuration { - TempDirHijackingToDeleteConfig() { this = "TempDirHijackingToDeleteConfig" } - - override predicate isSource(DataFlow::Node source) { - source.asExpr() = +private class DirHijackingTaintSource extends DataFlow::Node { + DirHijackingTaintSource() { + this.asExpr() = any(MethodAccess ma | ma.getMethod() instanceof MethodFileCreateTempFile and ma.getNumArgument() = 2 ) or // TODO: Replace with getSystemProperty("java.io.tmpdir") - source.asExpr() = + this.asExpr() = any(MethodAccessSystemGetProperty maSgp | maSgp.hasCompileTimeConstantGetPropertyName("java.io.tmpdir") ) } +} + +private predicate isAdditionalTaintStepCommon(DataFlow::Node node1, DataFlow::Node node2) { + node2.asExpr() = + any(MethodAccess ma | + ma.getMethod() instanceof MethodFileCreateTempFile and ma.getArgument(2) = node1.asExpr() + ) +} + +private class TempDirHijackingToDeleteConfig extends TaintTracking2::Configuration { + TempDirHijackingToDeleteConfig() { this = "TempDirHijackingToDeleteConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof DirHijackingTaintSource } override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - node2.asExpr() = - any(MethodAccess ma | - ma.getMethod() instanceof MethodFileCreateTempFile and ma.getArgument(2) = node1.asExpr() - ) + isAdditionalTaintStepCommon(node1, node2) } override predicate isSink(DataFlow::Node sink) { isDeleteFileExpr(sink.asExpr()) } } -private class TempDirHijackingFromDeleteConfig extends DataFlow2::Configuration { +private class TempDirHijackingFromDeleteConfig extends DataFlow3::Configuration { TempDirHijackingFromDeleteConfig() { this = "TempDirHijackingFromDeleteConfig" } override predicate isSource(DataFlow::Node source) { isDeleteFileExpr(source.asExpr()) } @@ -127,15 +138,31 @@ private predicate safeUse(Expr e) { ) } +private class TempDirHijackingFullPath extends TaintTracking::Configuration { + TempDirHijackingFullPath() { this = "TempDirHijackingFullPath" } + + override predicate isSource(DataFlow::Node source) { source instanceof DirHijackingTaintSource } + + override predicate isSink(DataFlow::Node sink) { + isNonThrowingDirectoryCreationExpression(sink.asExpr(), _) + } + + override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + isAdditionalTaintStepCommon(node1, node2) + or + node1.asExpr() = node2.asExpr() and isDeleteFileExpr(node1.asExpr()) + } +} + from - DataFlow::PathNode source, DataFlow::PathNode deleteCheckpoint, DataFlow2::Node deleteCheckpoint2, - DataFlow2::Node sink, MethodAccess creationCall, Expr unsafe + DataFlow::PathNode pathSource, DataFlow::PathNode pathSink, DataFlow::Node deleteCheckpoint, + MethodAccess creationCall, Expr unsafe where - any(TempDirHijackingToDeleteConfig c).hasFlowPath(source, deleteCheckpoint) and - any(TempDirHijackingFromDeleteConfig c).hasFlow(deleteCheckpoint2, sink) and - deleteCheckpoint.getNode().asExpr() = deleteCheckpoint2.asExpr() and - isUnsafeUseUnconstrainedByIfCheck(sink, unsafe) and - isNonThrowingDirectoryCreationExpression(sink.asExpr(), creationCall) -select deleteCheckpoint.getNode(), source, deleteCheckpoint, - "Local temporary directory hijacking race condition $@, file $@ may have been hijacked", - creationCall, "here", unsafe, "here" + any(TempDirHijackingFullPath c).hasFlowPath(pathSource, pathSink) and + any(TempDirHijackingToDeleteConfig c).hasFlow(pathSource.getNode(), deleteCheckpoint) and + any(TempDirHijackingFromDeleteConfig c).hasFlow(deleteCheckpoint, pathSink.getNode()) and + isUnsafeUseUnconstrainedByIfCheck(pathSink.getNode(), unsafe) and + isNonThrowingDirectoryCreationExpression(pathSink.getNode().asExpr(), creationCall) +select pathSink, pathSource, pathSink, + "Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user.", + deleteCheckpoint.asExpr(), "delete here", unsafe, "here" diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected index adf2cbcc3ef2d..4e377052e4bae 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected @@ -1,54 +1,57 @@ edges -| Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp | -| Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:23:9:23:12 | temp | -| Test.java:35:21:35:60 | createTempFile(...) : File | Test.java:36:9:36:12 | temp | -| Test.java:45:21:45:60 | createTempFile(...) : File | Test.java:46:9:46:12 | temp | -| Test.java:56:21:56:60 | createTempFile(...) : File | Test.java:57:9:57:12 | temp | -| Test.java:66:21:66:60 | createTempFile(...) : File | Test.java:67:20:67:23 | temp | -| Test.java:77:21:77:60 | createTempFile(...) : File | Test.java:78:20:78:23 | temp | -| Test.java:88:21:88:60 | createTempFile(...) : File | Test.java:89:13:89:16 | temp | -| Test.java:97:21:97:60 | createTempFile(...) : File | Test.java:98:14:98:17 | temp | -| Test.java:109:21:109:60 | createTempFile(...) : File | Test.java:110:30:110:33 | temp | -| Test.java:118:21:118:60 | createTempFile(...) : File | Test.java:119:14:119:17 | temp | -| Test.java:129:62:129:107 | new File(...) : File | Test.java:130:9:130:12 | temp | +| Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | +| Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:24:9:24:12 | temp | +| Test.java:29:21:29:60 | createTempFile(...) : File | Test.java:30:9:30:12 | temp | +| Test.java:35:21:35:60 | createTempFile(...) : File | Test.java:37:13:37:16 | temp | +| Test.java:45:21:45:60 | createTempFile(...) : File | Test.java:47:29:47:32 | temp | +| Test.java:56:21:56:60 | createTempFile(...) : File | Test.java:58:15:58:18 | temp | +| Test.java:66:21:66:60 | createTempFile(...) : File | Test.java:68:20:68:23 | temp | +| Test.java:77:21:77:60 | createTempFile(...) : File | Test.java:79:20:79:23 | temp | +| Test.java:88:21:88:60 | createTempFile(...) : File | Test.java:89:30:89:33 | temp | +| Test.java:97:21:97:60 | createTempFile(...) : File | Test.java:98:32:98:35 | temp | +| Test.java:109:21:109:60 | createTempFile(...) : File | Test.java:110:48:110:51 | temp | +| Test.java:118:21:118:60 | createTempFile(...) : File | Test.java:122:14:122:17 | temp | +| Test.java:129:62:129:107 | new File(...) : File | Test.java:131:9:131:12 | temp | | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:129:62:129:107 | new File(...) : File | | Test.java:146:13:146:58 | new File(...) : File | Test.java:146:13:146:76 | getAbsoluteFile(...) : File | -| Test.java:146:13:146:76 | getAbsoluteFile(...) : File | Test.java:148:9:148:12 | temp | +| Test.java:146:13:146:76 | getAbsoluteFile(...) : File | Test.java:149:9:149:12 | temp | | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:146:13:146:58 | new File(...) : File | nodes | Test.java:11:20:11:59 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:12:13:12:16 | temp | semmle.label | temp | +| Test.java:13:13:13:16 | temp | semmle.label | temp | | Test.java:22:21:22:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:23:9:23:12 | temp | semmle.label | temp | +| Test.java:24:9:24:12 | temp | semmle.label | temp | +| Test.java:29:21:29:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:30:9:30:12 | temp | semmle.label | temp | | Test.java:35:21:35:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:36:9:36:12 | temp | semmle.label | temp | +| Test.java:37:13:37:16 | temp | semmle.label | temp | | Test.java:45:21:45:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:46:9:46:12 | temp | semmle.label | temp | +| Test.java:47:29:47:32 | temp | semmle.label | temp | | Test.java:56:21:56:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:57:9:57:12 | temp | semmle.label | temp | +| Test.java:58:15:58:18 | temp | semmle.label | temp | | Test.java:66:21:66:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:67:20:67:23 | temp | semmle.label | temp | +| Test.java:68:20:68:23 | temp | semmle.label | temp | | Test.java:77:21:77:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:78:20:78:23 | temp | semmle.label | temp | +| Test.java:79:20:79:23 | temp | semmle.label | temp | | Test.java:88:21:88:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:89:13:89:16 | temp | semmle.label | temp | +| Test.java:89:30:89:33 | temp | semmle.label | temp | | Test.java:97:21:97:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:98:14:98:17 | temp | semmle.label | temp | +| Test.java:98:32:98:35 | temp | semmle.label | temp | | Test.java:109:21:109:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:110:30:110:33 | temp | semmle.label | temp | +| Test.java:110:48:110:51 | temp | semmle.label | temp | | Test.java:118:21:118:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:119:14:119:17 | temp | semmle.label | temp | +| Test.java:122:14:122:17 | temp | semmle.label | temp | | Test.java:129:62:129:107 | new File(...) : File | semmle.label | new File(...) : File | | Test.java:129:71:129:106 | getProperty(...) : String | semmle.label | getProperty(...) : String | -| Test.java:130:9:130:12 | temp | semmle.label | temp | +| Test.java:131:9:131:12 | temp | semmle.label | temp | | Test.java:146:13:146:58 | new File(...) : File | semmle.label | new File(...) : File | | Test.java:146:13:146:76 | getAbsoluteFile(...) : File | semmle.label | getAbsoluteFile(...) : File | | Test.java:146:22:146:57 | getProperty(...) : String | semmle.label | getProperty(...) : String | -| Test.java:148:9:148:12 | temp | semmle.label | temp | +| Test.java:149:9:149:12 | temp | semmle.label | temp | subpaths #select -| Test.java:12:13:12:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:13:13:13:24 | mkdir(...) | here | Test.java:18:9:18:33 | ...=... | here | -| Test.java:12:13:12:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:13:13:13:24 | mkdir(...) | here | Test.java:18:30:18:33 | temp | here | -| Test.java:23:9:23:12 | temp | Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:23:9:23:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:24:9:24:20 | mkdir(...) | here | Test.java:25:16:25:19 | temp | here | -| Test.java:130:9:130:12 | temp | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:130:9:130:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:131:9:131:20 | mkdir(...) | here | Test.java:132:16:132:19 | temp | here | -| Test.java:148:9:148:12 | temp | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:148:9:148:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:149:9:149:20 | mkdir(...) | here | Test.java:150:16:150:19 | temp | here | +| Test.java:13:13:13:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:12:13:12:16 | temp | delete here | Test.java:18:9:18:33 | ...=... | here | +| Test.java:13:13:13:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:12:13:12:16 | temp | delete here | Test.java:18:30:18:33 | temp | here | +| Test.java:24:9:24:12 | temp | Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:24:9:24:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:23:9:23:12 | temp | delete here | Test.java:25:16:25:19 | temp | here | +| Test.java:131:9:131:12 | temp | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:131:9:131:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:130:9:130:12 | temp | delete here | Test.java:132:16:132:19 | temp | here | +| Test.java:149:9:149:12 | temp | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:149:9:149:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:148:9:148:12 | temp | delete here | Test.java:150:16:150:19 | temp | here |