From b40a43701cc99d3f9c48bd5c30f620d9129a079b Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 27 May 2025 18:30:54 +0100 Subject: [PATCH 1/6] C#: Small optimization. Avoid a small CP between sinks and states. --- .../code/csharp/security/dataflow/TaintedPathQuery.qll | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll index 704421c06275..b981269e839e 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll @@ -64,10 +64,9 @@ private module TaintedPathConfig implements DataFlow::StateConfigSig { source instanceof Source and state = NotNormalized() } - predicate isSink(DataFlow::Node sink, FlowState state) { - sink instanceof Sink and - exists(state) - } + predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + + predicate isSink(DataFlow::Node sink, FlowState state) { none() } predicate isAdditionalFlowStep(DataFlow::Node n1, FlowState s1, DataFlow::Node n2, FlowState s2) { any(PathNormalizationStep step).isAdditionalFlowStep(n1, n2) and From 03e671aff1fc849457f665a2a0d8afee9b8e6f72 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 27 May 2025 18:34:23 +0100 Subject: [PATCH 2/6] C#: Add a false negative. --- .../Security Features/CWE-022/TaintedPath/TaintedPath.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs index b1f4e56b4600..083496ca53c5 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs @@ -61,6 +61,9 @@ public void ProcessRequest(HttpContext ctx) { File.ReadAllText(fullPath); // GOOD } + + // This test ensures that we can flow through `Path.GetFullPath` and still get a result. + ctx.Response.Write(File.ReadAllText(path)); // BAD [MISSING] } public bool IsReusable From a2d4c2006899a2b535a0f21f39040202d8baee26 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 27 May 2025 18:36:53 +0100 Subject: [PATCH 3/6] C#: Fix FN by blocking flow out of the function call instead of out of the argument (which is incorrect when there is use-use flow). --- .../code/csharp/security/dataflow/TaintedPathQuery.qll | 2 +- .../Security Features/CWE-022/TaintedPath/TaintedPath.cs | 2 +- .../CWE-022/TaintedPath/TaintedPath.expected | 5 +++++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll index b981269e839e..b943d67f8ee7 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll @@ -77,7 +77,7 @@ private module TaintedPathConfig implements DataFlow::StateConfigSig { predicate isBarrier(DataFlow::Node node, FlowState state) { node.(Sanitizer).isBarrier(state) } predicate isBarrierOut(DataFlow::Node node, FlowState state) { - isAdditionalFlowStep(node, state, _, _) + isAdditionalFlowStep(_, state, node, _) } } diff --git a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs index 083496ca53c5..4206a970394f 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs @@ -63,7 +63,7 @@ public void ProcessRequest(HttpContext ctx) } // This test ensures that we can flow through `Path.GetFullPath` and still get a result. - ctx.Response.Write(File.ReadAllText(path)); // BAD [MISSING] + ctx.Response.Write(File.ReadAllText(path)); // BAD } public bool IsReusable diff --git a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected index edb948d412c2..a258bdf4a4e7 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected @@ -6,6 +6,7 @@ | TaintedPath.cs:36:25:36:31 | access to local variable badPath | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:36:25:36:31 | access to local variable badPath | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value | | TaintedPath.cs:38:49:38:55 | access to local variable badPath | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:38:49:38:55 | access to local variable badPath | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value | | TaintedPath.cs:51:26:51:29 | access to local variable path | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:51:26:51:29 | access to local variable path | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value | +| TaintedPath.cs:66:45:66:48 | access to local variable path | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:66:45:66:48 | access to local variable path | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value | edges | TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:12:50:12:53 | access to local variable path | provenance | | | TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:17:51:17:54 | access to local variable path | provenance | | @@ -13,11 +14,13 @@ edges | TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:31:30:31:33 | access to local variable path | provenance | | | TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:35:16:35:22 | access to local variable badPath : String | provenance | | | TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:51:26:51:29 | access to local variable path | provenance | | +| TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:59:44:59:47 | access to local variable path : String | provenance | | | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:10:16:10:19 | access to local variable path : String | provenance | | | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:10:23:10:53 | access to indexer : String | provenance | MaD:1 | | TaintedPath.cs:10:23:10:53 | access to indexer : String | TaintedPath.cs:10:16:10:19 | access to local variable path : String | provenance | | | TaintedPath.cs:35:16:35:22 | access to local variable badPath : String | TaintedPath.cs:36:25:36:31 | access to local variable badPath | provenance | | | TaintedPath.cs:35:16:35:22 | access to local variable badPath : String | TaintedPath.cs:38:49:38:55 | access to local variable badPath | provenance | | +| TaintedPath.cs:59:44:59:47 | access to local variable path : String | TaintedPath.cs:66:45:66:48 | access to local variable path | provenance | | models | 1 | Summary: System.Collections.Specialized; NameValueCollection; false; get_Item; (System.String); ; Argument[this]; ReturnValue; taint; df-generated | nodes @@ -32,4 +35,6 @@ nodes | TaintedPath.cs:36:25:36:31 | access to local variable badPath | semmle.label | access to local variable badPath | | TaintedPath.cs:38:49:38:55 | access to local variable badPath | semmle.label | access to local variable badPath | | TaintedPath.cs:51:26:51:29 | access to local variable path | semmle.label | access to local variable path | +| TaintedPath.cs:59:44:59:47 | access to local variable path : String | semmle.label | access to local variable path : String | +| TaintedPath.cs:66:45:66:48 | access to local variable path | semmle.label | access to local variable path | subpaths From db7119c29ff0485f54b53837474351583a739ecb Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 27 May 2025 18:38:50 +0100 Subject: [PATCH 4/6] C#: Add a false positive. --- .../CWE-022/TaintedPath/TaintedPath.cs | 6 ++++++ .../CWE-022/TaintedPath/TaintedPath.expected | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs index 4206a970394f..9f1d682a49e0 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs @@ -64,6 +64,12 @@ public void ProcessRequest(HttpContext ctx) // This test ensures that we can flow through `Path.GetFullPath` and still get a result. ctx.Response.Write(File.ReadAllText(path)); // BAD + + string absolutePath = ctx.Request.MapPath("~MyTempFile"); + string fullPath2 = Path.Combine(absolutePath, path); + if (fullPath2.StartsWith(absolutePath + Path.DirectorySeparatorChar)) { + File.ReadAllText(fullPath2); // GOOD [FALSE POSITIVE] + } } public bool IsReusable diff --git a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected index a258bdf4a4e7..b4849fe9dc4b 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected @@ -7,6 +7,7 @@ | TaintedPath.cs:38:49:38:55 | access to local variable badPath | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:38:49:38:55 | access to local variable badPath | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value | | TaintedPath.cs:51:26:51:29 | access to local variable path | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:51:26:51:29 | access to local variable path | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value | | TaintedPath.cs:66:45:66:48 | access to local variable path | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:66:45:66:48 | access to local variable path | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value | +| TaintedPath.cs:71:30:71:38 | access to local variable fullPath2 | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:71:30:71:38 | access to local variable fullPath2 | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value | edges | TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:12:50:12:53 | access to local variable path | provenance | | | TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:17:51:17:54 | access to local variable path | provenance | | @@ -21,8 +22,13 @@ edges | TaintedPath.cs:35:16:35:22 | access to local variable badPath : String | TaintedPath.cs:36:25:36:31 | access to local variable badPath | provenance | | | TaintedPath.cs:35:16:35:22 | access to local variable badPath : String | TaintedPath.cs:38:49:38:55 | access to local variable badPath | provenance | | | TaintedPath.cs:59:44:59:47 | access to local variable path : String | TaintedPath.cs:66:45:66:48 | access to local variable path | provenance | | +| TaintedPath.cs:59:44:59:47 | access to local variable path : String | TaintedPath.cs:69:55:69:58 | access to local variable path : String | provenance | | +| TaintedPath.cs:69:16:69:24 | access to local variable fullPath2 : String | TaintedPath.cs:71:30:71:38 | access to local variable fullPath2 | provenance | | +| TaintedPath.cs:69:28:69:59 | call to method Combine : String | TaintedPath.cs:69:16:69:24 | access to local variable fullPath2 : String | provenance | | +| TaintedPath.cs:69:55:69:58 | access to local variable path : String | TaintedPath.cs:69:28:69:59 | call to method Combine : String | provenance | MaD:2 | models | 1 | Summary: System.Collections.Specialized; NameValueCollection; false; get_Item; (System.String); ; Argument[this]; ReturnValue; taint; df-generated | +| 2 | Summary: System.IO; Path; false; Combine; (System.String,System.String); ; Argument[1]; ReturnValue; taint; manual | nodes | TaintedPath.cs:10:16:10:19 | access to local variable path : String | semmle.label | access to local variable path : String | | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection | @@ -37,4 +43,8 @@ nodes | TaintedPath.cs:51:26:51:29 | access to local variable path | semmle.label | access to local variable path | | TaintedPath.cs:59:44:59:47 | access to local variable path : String | semmle.label | access to local variable path : String | | TaintedPath.cs:66:45:66:48 | access to local variable path | semmle.label | access to local variable path | +| TaintedPath.cs:69:16:69:24 | access to local variable fullPath2 : String | semmle.label | access to local variable fullPath2 : String | +| TaintedPath.cs:69:28:69:59 | call to method Combine : String | semmle.label | call to method Combine : String | +| TaintedPath.cs:69:55:69:58 | access to local variable path : String | semmle.label | access to local variable path : String | +| TaintedPath.cs:71:30:71:38 | access to local variable fullPath2 | semmle.label | access to local variable fullPath2 | subpaths From 4dfa88626ab3a75d2f4216020809f3ad44c6b4d9 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 27 May 2025 18:43:21 +0100 Subject: [PATCH 5/6] C#: Recognize more path-normalization steps. --- .../security/dataflow/TaintedPathQuery.qll | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll index b943d67f8ee7..241a147135bc 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll @@ -52,6 +52,40 @@ private class GetFullPathStep extends PathNormalizationStep { } } +/** Holds if `e` may evaluate to an absolute path. */ +bindingset[e] +pragma[inline_late] +private predicate isAbsolute(Expr e) { + exists(Expr absolute | DataFlow::localExprFlow(absolute, e) | + exists(Call call | absolute = call | + call.getARuntimeTarget() + .hasFullyQualifiedName(["System.Web.HttpServerUtilityBase", "System.Web.HttpRequest"], + "MapPath") + or + call.getARuntimeTarget().hasFullyQualifiedName("System.IO.Path", "GetFullPath") + or + call.getARuntimeTarget().hasFullyQualifiedName("System.IO.Directory", "GetCurrentDirectory") + ) + or + exists(PropertyRead read | absolute = read | + read.getTarget().hasFullyQualifiedName("System", "Environment", "CurrentDirectory") + ) + ) +} + +private class PathCombineStep extends PathNormalizationStep { + override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) { + exists(Call call | + // The result of `Path.Combine(x, y)` is an absolute path when `x` is an + // absolute path. + call.getARuntimeTarget().hasFullyQualifiedName("System.IO.Path", "Combine") and + isAbsolute(call.getArgument(0)) and + n1.asExpr() = call.getArgument(1) and + n2.asExpr() = call + ) + } +} + /** * A taint-tracking configuration for uncontrolled data in path expression vulnerabilities. */ From 2186fef8bf0575b0037b2f30d99c8a8a280c8626 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 27 May 2025 18:43:45 +0100 Subject: [PATCH 6/6] C#: Accept test changes. --- .../CWE-022/TaintedPath/TaintedPath.cs | 4 ++-- .../CWE-022/TaintedPath/TaintedPath.expected | 10 ---------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs index 9f1d682a49e0..d492f20336b6 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs @@ -61,14 +61,14 @@ public void ProcessRequest(HttpContext ctx) { File.ReadAllText(fullPath); // GOOD } - + // This test ensures that we can flow through `Path.GetFullPath` and still get a result. ctx.Response.Write(File.ReadAllText(path)); // BAD string absolutePath = ctx.Request.MapPath("~MyTempFile"); string fullPath2 = Path.Combine(absolutePath, path); if (fullPath2.StartsWith(absolutePath + Path.DirectorySeparatorChar)) { - File.ReadAllText(fullPath2); // GOOD [FALSE POSITIVE] + File.ReadAllText(fullPath2); // GOOD } } diff --git a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected index b4849fe9dc4b..a258bdf4a4e7 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected @@ -7,7 +7,6 @@ | TaintedPath.cs:38:49:38:55 | access to local variable badPath | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:38:49:38:55 | access to local variable badPath | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value | | TaintedPath.cs:51:26:51:29 | access to local variable path | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:51:26:51:29 | access to local variable path | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value | | TaintedPath.cs:66:45:66:48 | access to local variable path | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:66:45:66:48 | access to local variable path | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value | -| TaintedPath.cs:71:30:71:38 | access to local variable fullPath2 | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | TaintedPath.cs:71:30:71:38 | access to local variable fullPath2 | This path depends on a $@. | TaintedPath.cs:10:23:10:45 | access to property QueryString | user-provided value | edges | TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:12:50:12:53 | access to local variable path | provenance | | | TaintedPath.cs:10:16:10:19 | access to local variable path : String | TaintedPath.cs:17:51:17:54 | access to local variable path | provenance | | @@ -22,13 +21,8 @@ edges | TaintedPath.cs:35:16:35:22 | access to local variable badPath : String | TaintedPath.cs:36:25:36:31 | access to local variable badPath | provenance | | | TaintedPath.cs:35:16:35:22 | access to local variable badPath : String | TaintedPath.cs:38:49:38:55 | access to local variable badPath | provenance | | | TaintedPath.cs:59:44:59:47 | access to local variable path : String | TaintedPath.cs:66:45:66:48 | access to local variable path | provenance | | -| TaintedPath.cs:59:44:59:47 | access to local variable path : String | TaintedPath.cs:69:55:69:58 | access to local variable path : String | provenance | | -| TaintedPath.cs:69:16:69:24 | access to local variable fullPath2 : String | TaintedPath.cs:71:30:71:38 | access to local variable fullPath2 | provenance | | -| TaintedPath.cs:69:28:69:59 | call to method Combine : String | TaintedPath.cs:69:16:69:24 | access to local variable fullPath2 : String | provenance | | -| TaintedPath.cs:69:55:69:58 | access to local variable path : String | TaintedPath.cs:69:28:69:59 | call to method Combine : String | provenance | MaD:2 | models | 1 | Summary: System.Collections.Specialized; NameValueCollection; false; get_Item; (System.String); ; Argument[this]; ReturnValue; taint; df-generated | -| 2 | Summary: System.IO; Path; false; Combine; (System.String,System.String); ; Argument[1]; ReturnValue; taint; manual | nodes | TaintedPath.cs:10:16:10:19 | access to local variable path : String | semmle.label | access to local variable path : String | | TaintedPath.cs:10:23:10:45 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection | @@ -43,8 +37,4 @@ nodes | TaintedPath.cs:51:26:51:29 | access to local variable path | semmle.label | access to local variable path | | TaintedPath.cs:59:44:59:47 | access to local variable path : String | semmle.label | access to local variable path : String | | TaintedPath.cs:66:45:66:48 | access to local variable path | semmle.label | access to local variable path | -| TaintedPath.cs:69:16:69:24 | access to local variable fullPath2 : String | semmle.label | access to local variable fullPath2 : String | -| TaintedPath.cs:69:28:69:59 | call to method Combine : String | semmle.label | call to method Combine : String | -| TaintedPath.cs:69:55:69:58 | access to local variable path : String | semmle.label | access to local variable path : String | -| TaintedPath.cs:71:30:71:38 | access to local variable fullPath2 | semmle.label | access to local variable fullPath2 | subpaths