Consider calls to setReadable(false, false) then setReadable(true, true) to be safe

This commit is contained in:
Jonathan Leitschuh
2022-02-08 17:57:10 -05:00
parent a6596ea7ce
commit 7f46640176
3 changed files with 68 additions and 12 deletions

View File

@@ -197,14 +197,17 @@ class InsecureMethodPseudoConfiguration extends DataFlow::Configuration {
from DataFlow::PathNode source, DataFlow::PathNode sink, string message
where
any(TempDirSystemGetPropertyToCreateConfig conf).hasFlowPath(source, sink) and
message =
"Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users."
or
any(InsecureMethodPseudoConfiguration conf).hasFlowPath(source, sink) and
// Note this message has no "$@" placeholder, so the "system temp directory" template parameter below is not used.
message =
"Local information disclosure vulnerability due to use of " +
source.getNode().asExpr().(MethodAccessInsecureFileCreation).getFileSystemEntityType() +
" readable by other local users."
(
any(TempDirSystemGetPropertyToCreateConfig conf).hasFlowPath(source, sink) and
message =
"Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users."
or
any(InsecureMethodPseudoConfiguration conf).hasFlowPath(source, sink) and
// Note this message has no "$@" placeholder, so the "system temp directory" template parameter below is not used.
message =
"Local information disclosure vulnerability due to use of " +
source.getNode().asExpr().(MethodAccessInsecureFileCreation).getFileSystemEntityType() +
" readable by other local users."
) and
not isPermissionsProtectedTempDirUse(sink.getNode())
select source.getNode(), source, sink, message, source.getNode(), "system temp directory"

View File

@@ -59,8 +59,8 @@ private predicate isFileConstructorArgument(Expr expSource, Expr exprDest) {
*/
private class TaintFollowingFileMethod extends Method {
TaintFollowingFileMethod() {
getDeclaringType() instanceof TypeFile and
hasName(["getAbsoluteFile", "getCanonicalFile"])
this.getDeclaringType() instanceof TypeFile and
this.hasName(["getAbsoluteFile", "getCanonicalFile"])
}
}
@@ -80,3 +80,50 @@ predicate isAdditionalFileTaintStep(DataFlow::Node node1, DataFlow::Node node2)
isFileConstructorArgument(node1.asExpr(), node2.asExpr()) or
isTaintPropagatingFileTransformation(node1.asExpr(), node2.asExpr())
}
/**
* A method call to `java.io.File::setReadable`.
*/
private class FileSetRedableMethodAccess extends MethodAccess {
FileSetRedableMethodAccess() {
exists(Method m | this.getMethod() = m |
m.getDeclaringType() instanceof TypeFile and
m.hasName("setReadable")
)
}
predicate isCallWithArguments(boolean arg1, boolean arg2) {
this.isCallWithArgument(0, arg1) and this.isCallToSecondArgumentWithValue(arg2)
}
private predicate isCallToSecondArgumentWithValue(boolean value) {
this.getMethod().getNumberOfParameters() = 1 and value = true
or
isCallWithArgument(1, value)
}
private predicate isCallWithArgument(int index, boolean arg) {
DataFlow::localExprFlow(any(CompileTimeConstantExpr e | e.getBooleanValue() = arg),
this.getArgument(index))
}
}
/**
* Hold's if temporary directory's use is protected if there is an explicit call to
* `setReadable(false, false)`, then `setRedabale(true, true)`.
*/
predicate isPermissionsProtectedTempDirUse(DataFlow::Node sink) {
exists(FileSetRedableMethodAccess setReadable1, FileSetRedableMethodAccess setReadable2 |
setReadable1.isCallWithArguments(false, false) and
setReadable2.isCallWithArguments(true, true)
|
exists(DataFlow::Node setReadableNode1, DataFlow::Node setReadableNode2 |
setReadableNode1.asExpr() = setReadable1.getQualifier() and
setReadableNode2.asExpr() = setReadable2.getQualifier()
|
DataFlow::localFlow(sink, setReadableNode1) and // Flow from sink to setReadable(false, false)
DataFlow::localFlow(sink, setReadableNode2) and // Flow from sink to setReadable(true, true)
DataFlow::localFlow(setReadableNode1, setReadableNode2) // Flow from setReadable(false, false) to setReadable(true, true)
)
)
}