Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java: File constructor path sanitizer #18504

Merged
merged 10 commits into from
Feb 18, 2025
Merged

Java: File constructor path sanitizer #18504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
59d4547
Java: add FileConstructorSanitizer and tests
Jan 16, 2025
60cc16c
Java: change note
Jan 16, 2025
4a4585a
Java: move comment
Feb 5, 2025
e8724ab
Java: sanitize constructor call instead and update test cases
Feb 5, 2025
bd47dcc
Java: check first arg for taint
Feb 5, 2025
d21c8d7
Java: restrict sink to first arg of two-arg constructor call
Feb 6, 2025
c0ebeb9
Java: use AdditionalTaintStep
Feb 14, 2025
2bb6a39
Java: update tests
Feb 14, 2025
61a184c
Java: update more tests
Feb 14, 2025
9bb5fe8
Java: address review comments
Feb 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions java/ql/lib/change-notes/2025-01-16-file-constructor-sanitizer.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Added a path injection sanitizer for the `child` argument of a `java.io.File` constructor if that argument does not contain path traversal sequences.
5 changes: 4 additions & 1 deletion java/ql/lib/ext/java.io.model.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,10 @@ extensions:
- ["java.io", "DataInput", True, "readUTF", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
- ["java.io", "DataInputStream", False, "DataInputStream", "", "", "Argument[0]", "Argument[this]", "taint", "manual"]
- ["java.io", "File", False, "File", "", "", "Argument[0]", "Argument[this]", "taint", "manual"]
- ["java.io", "File", False, "File", "", "", "Argument[1]", "Argument[this]", "taint", "manual"]
# We model this taint step in QL as `FileConstructorChildArgumentStep` in the `PathSanitizer` library
# since we need to sanitize the use of this argument but not later uses of the same SSA variable,
# which is not currently possible in Java with a standard sanitizer due to use-use flow.
# - ["java.io", "File", False, "File", "", "", "Argument[1]", "Argument[this]", "taint", "manual"]
- ["java.io", "File", True, "getAbsoluteFile", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
- ["java.io", "File", True, "getAbsolutePath", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
- ["java.io", "File", True, "getCanonicalFile", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
Expand Down
1 change: 1 addition & 0 deletions java/ql/lib/semmle/code/java/dataflow/FlowSteps.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ private module Frameworks {
private import semmle.code.java.frameworks.ratpack.RatpackExec
private import semmle.code.java.frameworks.stapler.Stapler
private import semmle.code.java.security.ListOfConstantsSanitizer
private import semmle.code.java.security.PathSanitizer
}

/**
Expand Down
47 changes: 39 additions & 8 deletions java/ql/lib/semmle/code/java/security/PathSanitizer.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ private import semmle.code.java.dataflow.FlowSources
private import semmle.code.java.dataflow.SSA
private import semmle.code.java.frameworks.kotlin.IO
private import semmle.code.java.frameworks.kotlin.Text
private import semmle.code.java.dataflow.Nullness

/** A sanitizer that protects against path injection vulnerabilities. */
abstract class PathInjectionSanitizer extends DataFlow::Node { }
Expand Down Expand Up @@ -137,14 +138,7 @@ private class AllowedPrefixSanitizer extends PathInjectionSanitizer {
* been checked for a trusted prefix.
*/
private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) {
// Local taint-flow is used here to handle cases where the validated expression comes from the
// expression reaching the sink, but it's not the same one, e.g.:
// Path path = source();
// String strPath = path.toString();
// if (!strPath.contains("..") && strPath.startsWith("/safe/dir"))
// sink(path);
branch = g.(PathTraversalGuard).getBranch() and
localTaintFlowToPathGuard(e, g) and
pathTraversalGuard(g, e, branch) and
exists(Guard previousGuard |
previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock(), true)
or
Expand Down Expand Up @@ -352,3 +346,40 @@ private class FileGetNameSanitizer extends PathInjectionSanitizer {
)
}
}

/** Holds if `g` is a guard that checks for `..` components. */
private predicate pathTraversalGuard(Guard g, Expr e, boolean branch) {
// Local taint-flow is used here to handle cases where the validated expression comes from the
// expression reaching the sink, but it's not the same one, e.g.:
// Path path = source();
// String strPath = path.toString();
// if (!strPath.contains("..") && strPath.startsWith("/safe/dir"))
// sink(path);
branch = g.(PathTraversalGuard).getBranch() and
localTaintFlowToPathGuard(e, g)
}

/**
* A taint step from a child argument of a `java.io.File` constructor to the
* constructor call.
*
* This step only applies if the argument is not guarded by a check for `..`
* components (`PathTraversalGuard`), or it does not have any internal `..`
* components removed from it (`PathNormalizeSanitizer`), or if the parent
* argument of the constructor may be null.
*/
private class FileConstructorChildArgumentStep extends AdditionalTaintStep {
override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
exists(ConstructorCall constrCall |
constrCall.getConstructedType() instanceof TypeFile and
n1.asExpr() = constrCall.getArgument(1) and
n2.asExpr() = constrCall
|
not n1 = DataFlow::BarrierGuard<pathTraversalGuard/3>::getABarrierNode() and
not n1 = ValidationMethod<pathTraversalGuard/3>::getAValidatedNode() and
not TaintTracking::localExprTaint(any(PathNormalizeSanitizer p), n1.asExpr())
or
DataFlow::localExprFlow(nullExpr(), constrCall.getArgument(0))
)
}
}
29 changes: 12 additions & 17 deletions java/ql/test/experimental/query-tests/security/CWE-200/InsecureWebResourceResponse.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ edges
| InsecureWebResourceResponse.java:65:41:65:43 | url : String | InsecureWebResourceResponse.java:65:31:65:44 | parse(...) : Uri | provenance | MaD:2 |
| InsecureWebResourceResponse.java:66:51:66:84 | new FileInputStream(...) : FileInputStream | InsecureWebResourceResponse.java:68:71:68:81 | inputStream | provenance | |
| InsecureWebResourceResponse.java:66:71:66:73 | uri : Uri | InsecureWebResourceResponse.java:66:71:66:83 | getPath(...) : String | provenance | MaD:4 |
| InsecureWebResourceResponse.java:66:71:66:83 | getPath(...) : String | InsecureWebResourceResponse.java:66:51:66:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:7 |
| InsecureWebResourceResponse.java:66:71:66:83 | getPath(...) : String | InsecureWebResourceResponse.java:66:51:66:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:6 |
| InsecureWebResourceResponse.java:75:20:75:22 | url : String | InsecureWebResourceResponse.java:63:77:63:86 | url : String | provenance | AdditionalTaintStep |
| InsecureWebResourceResponse.java:75:20:75:22 | url : String | InsecureWebResourceResponse.java:84:77:84:86 | url : String | provenance | AdditionalTaintStep |
| InsecureWebResourceResponse.java:75:20:75:22 | url : String | InsecureWebResourceResponse.java:110:77:110:86 | url : String | provenance | AdditionalTaintStep |
Expand All @@ -32,11 +32,10 @@ edges
| InsecureWebResourceResponse.java:84:77:84:86 | url : String | InsecureWebResourceResponse.java:86:41:86:43 | url : String | provenance | |
| InsecureWebResourceResponse.java:86:31:86:44 | parse(...) : Uri | InsecureWebResourceResponse.java:88:66:88:68 | uri : Uri | provenance | |
| InsecureWebResourceResponse.java:86:41:86:43 | url : String | InsecureWebResourceResponse.java:86:31:86:44 | parse(...) : Uri | provenance | MaD:2 |
| InsecureWebResourceResponse.java:88:42:88:90 | new File(...) : File | InsecureWebResourceResponse.java:89:75:89:83 | cacheFile : File | provenance | |
| InsecureWebResourceResponse.java:88:66:88:68 | uri : Uri | InsecureWebResourceResponse.java:88:66:88:89 | getLastPathSegment(...) : String | provenance | MaD:3 |
| InsecureWebResourceResponse.java:88:66:88:89 | getLastPathSegment(...) : String | InsecureWebResourceResponse.java:88:42:88:90 | new File(...) : File | provenance | MaD:6 |
| InsecureWebResourceResponse.java:88:66:88:89 | getLastPathSegment(...) : String | InsecureWebResourceResponse.java:89:75:89:83 | cacheFile : File | provenance | AdditionalTaintStep |
| InsecureWebResourceResponse.java:89:55:89:84 | new FileInputStream(...) : FileInputStream | InsecureWebResourceResponse.java:91:75:91:85 | inputStream | provenance | |
| InsecureWebResourceResponse.java:89:75:89:83 | cacheFile : File | InsecureWebResourceResponse.java:89:55:89:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:7 |
| InsecureWebResourceResponse.java:89:75:89:83 | cacheFile : File | InsecureWebResourceResponse.java:89:55:89:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:6 |
| InsecureWebResourceResponse.java:101:20:101:22 | url : String | InsecureWebResourceResponse.java:63:77:63:86 | url : String | provenance | AdditionalTaintStep |
| InsecureWebResourceResponse.java:101:20:101:22 | url : String | InsecureWebResourceResponse.java:84:77:84:86 | url : String | provenance | AdditionalTaintStep |
| InsecureWebResourceResponse.java:101:20:101:22 | url : String | InsecureWebResourceResponse.java:110:77:110:86 | url : String | provenance | AdditionalTaintStep |
Expand All @@ -47,11 +46,11 @@ edges
| InsecureWebResourceResponse.java:112:31:112:44 | parse(...) : Uri | InsecureWebResourceResponse.java:113:35:113:37 | uri : Uri | provenance | |
| InsecureWebResourceResponse.java:112:41:112:43 | url : String | InsecureWebResourceResponse.java:112:31:112:44 | parse(...) : Uri | provenance | MaD:2 |
| InsecureWebResourceResponse.java:113:35:113:37 | uri : Uri | InsecureWebResourceResponse.java:113:35:113:47 | getPath(...) : String | provenance | MaD:4 |
| InsecureWebResourceResponse.java:113:35:113:47 | getPath(...) : String | InsecureWebResourceResponse.java:113:35:113:60 | substring(...) : String | provenance | MaD:8 |
| InsecureWebResourceResponse.java:113:35:113:47 | getPath(...) : String | InsecureWebResourceResponse.java:113:35:113:60 | substring(...) : String | provenance | MaD:7 |
| InsecureWebResourceResponse.java:113:35:113:60 | substring(...) : String | InsecureWebResourceResponse.java:115:75:115:78 | path : String | provenance | |
| InsecureWebResourceResponse.java:115:55:115:108 | new FileInputStream(...) : FileInputStream | InsecureWebResourceResponse.java:117:75:117:85 | inputStream | provenance | |
| InsecureWebResourceResponse.java:115:75:115:78 | path : String | InsecureWebResourceResponse.java:115:75:115:107 | substring(...) : String | provenance | MaD:8 |
| InsecureWebResourceResponse.java:115:75:115:107 | substring(...) : String | InsecureWebResourceResponse.java:115:55:115:108 | new FileInputStream(...) : FileInputStream | provenance | MaD:7 |
| InsecureWebResourceResponse.java:115:75:115:78 | path : String | InsecureWebResourceResponse.java:115:75:115:107 | substring(...) : String | provenance | MaD:7 |
| InsecureWebResourceResponse.java:115:75:115:107 | substring(...) : String | InsecureWebResourceResponse.java:115:55:115:108 | new FileInputStream(...) : FileInputStream | provenance | MaD:6 |
| InsecureWebResourceResponse.java:127:20:127:22 | url : String | InsecureWebResourceResponse.java:63:77:63:86 | url : String | provenance | AdditionalTaintStep |
| InsecureWebResourceResponse.java:127:20:127:22 | url : String | InsecureWebResourceResponse.java:84:77:84:86 | url : String | provenance | AdditionalTaintStep |
| InsecureWebResourceResponse.java:127:20:127:22 | url : String | InsecureWebResourceResponse.java:110:77:110:86 | url : String | provenance | AdditionalTaintStep |
Expand Down Expand Up @@ -79,11 +78,10 @@ edges
| InsecureWebResourceResponse.java:192:77:192:102 | request : WebResourceRequest | InsecureWebResourceResponse.java:194:31:194:37 | request : WebResourceRequest | provenance | |
| InsecureWebResourceResponse.java:194:31:194:37 | request : WebResourceRequest | InsecureWebResourceResponse.java:194:31:194:46 | getUrl(...) : Uri | provenance | MaD:5 |
| InsecureWebResourceResponse.java:194:31:194:46 | getUrl(...) : Uri | InsecureWebResourceResponse.java:196:66:196:68 | uri : Uri | provenance | |
| InsecureWebResourceResponse.java:196:42:196:90 | new File(...) : File | InsecureWebResourceResponse.java:197:75:197:83 | cacheFile : File | provenance | |
| InsecureWebResourceResponse.java:196:66:196:68 | uri : Uri | InsecureWebResourceResponse.java:196:66:196:89 | getLastPathSegment(...) : String | provenance | MaD:3 |
| InsecureWebResourceResponse.java:196:66:196:89 | getLastPathSegment(...) : String | InsecureWebResourceResponse.java:196:42:196:90 | new File(...) : File | provenance | MaD:6 |
| InsecureWebResourceResponse.java:196:66:196:89 | getLastPathSegment(...) : String | InsecureWebResourceResponse.java:197:75:197:83 | cacheFile : File | provenance | AdditionalTaintStep |
| InsecureWebResourceResponse.java:197:55:197:84 | new FileInputStream(...) : FileInputStream | InsecureWebResourceResponse.java:199:75:199:85 | inputStream | provenance | |
| InsecureWebResourceResponse.java:197:75:197:83 | cacheFile : File | InsecureWebResourceResponse.java:197:55:197:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:7 |
| InsecureWebResourceResponse.java:197:75:197:83 | cacheFile : File | InsecureWebResourceResponse.java:197:55:197:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:6 |
| InsecureWebResourceResponse.java:209:20:209:22 | url : String | InsecureWebResourceResponse.java:63:77:63:86 | url : String | provenance | AdditionalTaintStep |
| InsecureWebResourceResponse.java:209:20:209:22 | url : String | InsecureWebResourceResponse.java:84:77:84:86 | url : String | provenance | AdditionalTaintStep |
| InsecureWebResourceResponse.java:209:20:209:22 | url : String | InsecureWebResourceResponse.java:110:77:110:86 | url : String | provenance | AdditionalTaintStep |
Expand All @@ -100,7 +98,7 @@ edges
| InsecureWebResourceResponse.java:234:33:234:35 | url : String | InsecureWebResourceResponse.java:234:23:234:36 | parse(...) : Uri | provenance | MaD:2 |
| InsecureWebResourceResponse.java:235:43:235:76 | new FileInputStream(...) : FileInputStream | InsecureWebResourceResponse.java:237:63:237:73 | inputStream | provenance | |
| InsecureWebResourceResponse.java:235:63:235:65 | uri : Uri | InsecureWebResourceResponse.java:235:63:235:75 | getPath(...) : String | provenance | MaD:4 |
| InsecureWebResourceResponse.java:235:63:235:75 | getPath(...) : String | InsecureWebResourceResponse.java:235:43:235:76 | new FileInputStream(...) : FileInputStream | provenance | MaD:7 |
| InsecureWebResourceResponse.java:235:63:235:75 | getPath(...) : String | InsecureWebResourceResponse.java:235:43:235:76 | new FileInputStream(...) : FileInputStream | provenance | MaD:6 |
| InsecureWebViewActivity.java:27:27:27:37 | getIntent(...) : Intent | InsecureWebViewActivity.java:27:27:27:64 | getStringExtra(...) : String | provenance | MaD:1 |
| InsecureWebViewActivity.java:27:27:27:64 | getStringExtra(...) : String | InsecureWebViewActivity.java:28:20:28:27 | inputUrl : String | provenance | |
| InsecureWebViewActivity.java:28:20:28:27 | inputUrl : String | InsecureWebViewActivity.java:42:28:42:37 | url : String | provenance | |
Expand All @@ -111,16 +109,15 @@ edges
| InsecureWebViewActivity.java:55:41:55:43 | url : String | InsecureWebViewActivity.java:55:31:55:44 | parse(...) : Uri | provenance | MaD:2 |
| InsecureWebViewActivity.java:56:51:56:84 | new FileInputStream(...) : FileInputStream | InsecureWebViewActivity.java:58:71:58:81 | inputStream | provenance | |
| InsecureWebViewActivity.java:56:71:56:73 | uri : Uri | InsecureWebViewActivity.java:56:71:56:83 | getPath(...) : String | provenance | MaD:4 |
| InsecureWebViewActivity.java:56:71:56:83 | getPath(...) : String | InsecureWebViewActivity.java:56:51:56:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:7 |
| InsecureWebViewActivity.java:56:71:56:83 | getPath(...) : String | InsecureWebViewActivity.java:56:51:56:84 | new FileInputStream(...) : FileInputStream | provenance | MaD:6 |
models
| 1 | Summary: android.content; Intent; true; getStringExtra; (String); ; Argument[this].SyntheticField[android.content.Intent.extras].MapValue; ReturnValue; value; manual |
| 2 | Summary: android.net; Uri; false; parse; ; ; Argument[0]; ReturnValue; taint; manual |
| 3 | Summary: android.net; Uri; true; getLastPathSegment; ; ; Argument[this]; ReturnValue; taint; manual |
| 4 | Summary: android.net; Uri; true; getPath; ; ; Argument[this]; ReturnValue; taint; manual |
| 5 | Summary: android.webkit; WebResourceRequest; false; getUrl; ; ; Argument[this]; ReturnValue; taint; manual |
| 6 | Summary: java.io; File; false; File; ; ; Argument[1]; Argument[this]; taint; manual |
| 7 | Summary: java.io; FileInputStream; true; FileInputStream; ; ; Argument[0]; Argument[this]; taint; manual |
| 8 | Summary: java.lang; String; false; substring; ; ; Argument[this]; ReturnValue; taint; manual |
| 6 | Summary: java.io; FileInputStream; true; FileInputStream; ; ; Argument[0]; Argument[this]; taint; manual |
| 7 | Summary: java.lang; String; false; substring; ; ; Argument[this]; ReturnValue; taint; manual |
nodes
| InsecureWebResourceResponse.java:28:27:28:37 | getIntent(...) : Intent | semmle.label | getIntent(...) : Intent |
| InsecureWebResourceResponse.java:28:27:28:64 | getStringExtra(...) : String | semmle.label | getStringExtra(...) : String |
Expand All @@ -145,7 +142,6 @@ nodes
| InsecureWebResourceResponse.java:84:77:84:86 | url : String | semmle.label | url : String |
| InsecureWebResourceResponse.java:86:31:86:44 | parse(...) : Uri | semmle.label | parse(...) : Uri |
| InsecureWebResourceResponse.java:86:41:86:43 | url : String | semmle.label | url : String |
| InsecureWebResourceResponse.java:88:42:88:90 | new File(...) : File | semmle.label | new File(...) : File |
| InsecureWebResourceResponse.java:88:66:88:68 | uri : Uri | semmle.label | uri : Uri |
| InsecureWebResourceResponse.java:88:66:88:89 | getLastPathSegment(...) : String | semmle.label | getLastPathSegment(...) : String |
| InsecureWebResourceResponse.java:89:55:89:84 | new FileInputStream(...) : FileInputStream | semmle.label | new FileInputStream(...) : FileInputStream |
Expand Down Expand Up @@ -174,7 +170,6 @@ nodes
| InsecureWebResourceResponse.java:192:77:192:102 | request : WebResourceRequest | semmle.label | request : WebResourceRequest |
| InsecureWebResourceResponse.java:194:31:194:37 | request : WebResourceRequest | semmle.label | request : WebResourceRequest |
| InsecureWebResourceResponse.java:194:31:194:46 | getUrl(...) : Uri | semmle.label | getUrl(...) : Uri |
| InsecureWebResourceResponse.java:196:42:196:90 | new File(...) : File | semmle.label | new File(...) : File |
| InsecureWebResourceResponse.java:196:66:196:68 | uri : Uri | semmle.label | uri : Uri |
| InsecureWebResourceResponse.java:196:66:196:89 | getLastPathSegment(...) : String | semmle.label | getLastPathSegment(...) : String |
| InsecureWebResourceResponse.java:197:55:197:84 | new FileInputStream(...) : FileInputStream | semmle.label | new FileInputStream(...) : FileInputStream |
Expand Down
Loading
Loading