Java: Arbitrary user-controlled read/write on user-controlled path #3794
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| import java | ||
| import semmle.code.java.dataflow.DataFlow | ||
| import semmle.code.java.dataflow.TaintTracking | ||
|
|
||
| /** The class `java.io.FileInputStream`. */ | ||
| class TypeFileInputStream extends Class { | ||
| TypeFileInputStream() { this.hasQualifiedName("java.io", "FileInputStream") } | ||
| } | ||
|
|
||
| /** Models additional taint steps like `file.toPath()`, `path.toFile()`, `new FileInputStream(..)`, `Files.readAll{Bytes|Lines}(...)`, and `new File(...)`. */ | ||
| class PathAdditionalTaintStep extends TaintTracking::AdditionalTaintStep { | ||
| override predicate step(DataFlow::Node node1, DataFlow::Node node2) { | ||
| inputStreamReadsFromFile(node1, node2) | ||
| or | ||
| isFileToPath(node1, node2) | ||
| or | ||
| isPathToFile(node1, node2) | ||
| or | ||
| readsAllFromPath(node1, node2) | ||
| or | ||
| taintedNewFile(node1, node2) | ||
| } | ||
| } | ||
|
|
||
| /** Holds if `node1` is converted to `node2` via a call to `node1.toPath()`. */ | ||
| private predicate isFileToPath(DataFlow::Node node1, DataFlow::Node node2) { | ||
| exists(MethodAccess call | | ||
| call.getReceiverType() instanceof TypeFile and | ||
| call.getMethod().hasName("toPath") and | ||
| call = node2.asExpr() and | ||
| call.getQualifier() = node1.asExpr() | ||
| ) | ||
| } | ||
|
|
||
| /** Holds if `node1` is converted to `node2` via a call to `node1.toFile()`. */ | ||
| private predicate isPathToFile(DataFlow::Node node1, DataFlow::Node node2) { | ||
| exists(MethodAccess call | | ||
| call.getReceiverType() instanceof TypePath and | ||
| call.getMethod().hasName("toFile") and | ||
| call = node2.asExpr() and | ||
| call.getQualifier() = node1.asExpr() | ||
| ) | ||
| } | ||
|
|
||
| /** Holds if `node1` is read by `node2` via a call to `Files.readAllBytes(node1)` or `Files.readAllLines(node1)`. */ | ||
| private predicate readsAllFromPath(DataFlow::Node node1, DataFlow::Node node2) { | ||
| exists(MethodAccess call | | ||
| call.getReceiverType() instanceof TypeFiles and | ||
| call.getMethod().hasName(["readAllBytes", "readAllLines"]) and | ||
| call = node2.asExpr() and | ||
| call.getArgument(0) = node1.asExpr() | ||
| ) | ||
| } | ||
|
|
||
| /** Holds if `node1` is passed to `node2` via a call to `new FileInputStream(node1)`. */ | ||
| private predicate inputStreamReadsFromFile(DataFlow::Node node1, DataFlow::Node node2) { | ||
| exists(ConstructorCall call | | ||
| call.getConstructedType() instanceof TypeFileInputStream and | ||
| call = node2.asExpr() and | ||
| call.getAnArgument() = node1.asExpr() | ||
| ) | ||
| } | ||
|
|
||
| /** Holds if `node1` is passed to `node2` via a call to `new File(node1)`. */ | ||
| private predicate taintedNewFile(DataFlow::Node node1, DataFlow::Node node2) { | ||
| exists(ConstructorCall call | | ||
| call.getConstructedType() instanceof TypeFile and | ||
| call = node2.asExpr() and | ||
| call.getAnArgument() = node1.asExpr() | ||
| ) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,221 @@ | ||
| /** | ||
| * @name Disclosure of user-controlled path expression | ||
| * @description Disclosing content from paths influenced by users can allow an attacker to read arbitrary resources. | ||
| * @kind path-problem | ||
| * @problem.severity error | ||
| * @precision medium | ||
| * @id java/tainted-file-read | ||
| * @tags security | ||
| * external/cwe/cwe-706 | ||
| */ | ||
|
|
||
| import java | ||
| import semmle.code.java.dataflow.FlowSources | ||
| import semmle.code.java.dataflow.TaintTracking2 | ||
| import semmle.code.java.security.XSS | ||
| import DataFlow2::PathGraph | ||
| import semmle.code.java.security.PathCreation | ||
|
|
||
| /** The class `org.json.JSONObject`. */ | ||
| class TypeJsonObject extends Class { | ||
| TypeJsonObject() { this.hasQualifiedName("org.json", "JSONObject") } | ||
| } | ||
|
|
||
| /** The class `org.json.JSONArray`. */ | ||
| class TypeJsonArray extends Class { | ||
| TypeJsonArray() { this.hasQualifiedName("org.json", "JSONArray") } | ||
| } | ||
|
|
||
| /** The class `ai.susi.server.ServiceResponse`. */ | ||
| class TypeServiceResponse extends Class { | ||
| TypeServiceResponse() { this.hasQualifiedName("ai.susi.server", "ServiceResponse") } | ||
| } | ||
|
|
||
| class ServiceResponseSink extends DataFlow::ExprNode { | ||
| ServiceResponseSink() { | ||
| exists(ConstructorCall call | | ||
| call.getConstructedType() instanceof TypeServiceResponse and | ||
| this.getExpr() = call.getAnArgument() | ||
| ) | ||
| or | ||
| exists(MethodAccess call | | ||
| call.getType() instanceof TypeServiceResponse and | ||
| this.getExpr() = call.getAnArgument() | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| predicate deletesFile(DataFlow::ExprNode node) { | ||
| exists(MethodAccess call | | ||
| call.getReceiverType() instanceof TypeFile and | ||
| call.getMethod().getName().matches("delete%") and | ||
| node.getExpr() = call.getQualifier() | ||
| ) | ||
| } | ||
|
|
||
| predicate deletesPath(DataFlow::ExprNode node) { | ||
| exists(MethodAccess call | | ||
| call.getReceiverType() instanceof TypeFiles and | ||
| call.getMethod().getName().matches("delete%") and | ||
| node.getExpr() = call.getArgument(0) | ||
| ) | ||
| } | ||
|
|
||
| predicate renamesFile(DataFlow::ExprNode node) { | ||
| exists(MethodAccess call | | ||
| call.getReceiverType() instanceof TypeFile and | ||
| call.getMethod().getName().matches("renameTo%") and | ||
| ( | ||
| node.getExpr() = call.getQualifier() | ||
| or | ||
| node.getExpr() = call.getArgument(0) | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| predicate renamesPath(DataFlow::ExprNode node) { | ||
| exists(MethodAccess call | | ||
| call.getReceiverType() instanceof TypeFiles and | ||
| call.getMethod().getName().matches("move%") and | ||
| ( | ||
| node.getExpr() = call.getArgument(0) | ||
| or | ||
| node.getExpr() = call.getArgument(1) | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| class SensitiveFileOperationSink extends DataFlow::ExprNode { | ||
| SensitiveFileOperationSink() { | ||
| deletesFile(this) | ||
| or | ||
| deletesPath(this) | ||
| or | ||
| renamesFile(this) | ||
| or | ||
| renamesPath(this) | ||
| } | ||
| } | ||
|
|
||
| /** Holds if `node1` is used in the creation of `node2` and not guarded. */ | ||
| predicate usedInPathCreation(DataFlow::Node node1, DataFlow::Node node2) { | ||
| exists(Expr e | e = node1.asExpr() | | ||
| e = node2.asExpr().(PathCreation).getInput() and not guarded(e) | ||
| ) | ||
| } | ||
|
|
||
| predicate putsValueIntoJsonObject(DataFlow::Node node1, DataFlow::Node node2) { | ||
| exists(MethodAccess call | | ||
| call.getReceiverType() instanceof TypeJsonObject and | ||
| call.getMethod().getName() = ["put", "putOnce", "putOpt"] and | ||
| call.getQualifier() = node2.asExpr() and | ||
| call.getArgument(1) = node1.asExpr() | ||
| ) | ||
| } | ||
|
|
||
| predicate putsValueIntoJsonArray(DataFlow::Node node1, DataFlow::Node node2) { | ||
| exists(MethodAccess call | | ||
| call.getReceiverType() instanceof TypeJsonArray and | ||
| call.getMethod().getName() = "put" and | ||
| call.getQualifier() = node2.asExpr() and | ||
| ( | ||
| call.getArgument(1) = node1.asExpr() and call.getNumArgument() = 2 | ||
| or | ||
| call.getArgument(0) = node1.asExpr() and call.getNumArgument() = 1 | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| class ContainsDotDotSanitizer extends DataFlow::BarrierGuard { | ||
|
There was a problem hiding this comment. This might cause false negatives, e.g. String s = ...;
Path ROOT = Path.of("/my-app/data);
if (!s.contains("..")) {
Files.delete(ROOT.resolve(Path.of(s));
// Or
Files.delete(ROOT.resolve(s));
)Would still be vulnerable if |
||
| ContainsDotDotSanitizer() { | ||
| this.(MethodAccess).getMethod().hasName("contains") and | ||
| this.(MethodAccess).getAnArgument().(StringLiteral).getValue() = ".." | ||
| } | ||
|
|
||
| override predicate checks(Expr e, boolean branch) { | ||
| e = this.(MethodAccess).getQualifier() and branch = false | ||
| } | ||
| } | ||
|
|
||
| class TaintedPathConfig extends TaintTracking2::Configuration { | ||
| TaintedPathConfig() { this = "TaintedPathConfig" } | ||
|
|
||
| override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } | ||
|
|
||
| override predicate isSink(DataFlow::Node sink) { sink instanceof TaintedPathSink } | ||
|
|
||
| override predicate isSanitizer(DataFlow::Node node) { | ||
| exists(Type t | t = node.getType() | t instanceof BoxedType or t instanceof PrimitiveType) | ||
| } | ||
|
|
||
| override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { | ||
| guard instanceof ContainsDotDotSanitizer | ||
| // TODO add guards from zipslip.ql | ||
| } | ||
|
|
||
| override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { | ||
| usedInPathCreation(node1, node2) | ||
| } | ||
| } | ||
|
|
||
| private class TaintedPathSink extends DataFlow::Node { | ||
| Expr path; | ||
| Expr taintedInput; | ||
|
|
||
| TaintedPathSink() { | ||
| exists(Expr e, PathCreation p | e = asExpr() | | ||
| e = p.getInput() and not guarded(e) and path = p and taintedInput = e | ||
| ) | ||
| } | ||
|
|
||
| Expr getTaintedFile() { result = path } | ||
|
|
||
| Expr getTaintedFileInput() { result = taintedInput } | ||
| } | ||
|
|
||
| class InformationLeakConfig extends TaintTracking2::Configuration { | ||
| InformationLeakConfig() { this = "InformationLeakConfig" } | ||
|
|
||
| override predicate isSource(DataFlow::Node source) { | ||
| source instanceof TaintedPathSink | ||
| //exists(TaintedPathSink s | s.getTaintedFile() = source.asExpr()) | ||
| //source instanceof TaintedPathSink | ||
| //any() //source.asExpr().getType() instanceof TypePath //any()//source instanceof RemoteFlowSource | ||
| } //source.asExpr().getFile().getBaseName().matches("GetSkillJsonService.java")}//any()}//source instanceof RemoteFlowSource } | ||
|
|
||
| override predicate isSink(DataFlow::Node sink) { | ||
| sink instanceof RemoteFlowSink | ||
| or | ||
| //sink instanceof ServiceResponseSink or | ||
| sink instanceof XssSink //or | ||
| // sink instanceof SensitiveFileOperationSink | ||
| } | ||
|
|
||
| override predicate isSanitizer(DataFlow::Node node) { | ||
| node.getType() instanceof NumericType or node.getType() instanceof BooleanType | ||
| } | ||
|
|
||
| override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { | ||
| usedInPathCreation(node1, node2) | ||
| /* | ||
| * or | ||
| * putsValueIntoJsonObject(node1, node2) | ||
| * or | ||
| * putsValueIntoJsonArray(node1, node2) | ||
| */ | ||
|
|
||
| } | ||
| } | ||
|
|
||
| from | ||
| DataFlow2::PathNode remoteSource, DataFlow2::PathNode taintedFile, | ||
| DataFlow2::PathNode taintedFile2, DataFlow2::PathNode infoLeak, | ||
| InformationLeakConfig infoLeakConf, TaintedPathConfig taintedPathConf | ||
| where | ||
| taintedPathConf.hasFlowPath(remoteSource, taintedFile) and | ||
| taintedFile.getNode() = taintedFile2.getNode() and | ||
| infoLeakConf.hasFlowPath(taintedFile2, infoLeak) | ||
| select infoLeak.getNode(), taintedFile2, infoLeak, | ||
| "Potential disclosure of arbitrary file due to $@ derived from $@.", | ||
| taintedFile2.getNode().(TaintedPathSink).getTaintedFileInput(), "user-provided value", | ||
| remoteSource.getNode(), "a remote source" | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe consider the following as well?
Files.setAttributesFiles.setOwnerFiles.setPosixFilePermissionsAnd same for respective
java.io.Filemethods.