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 c00df1fa85f8..50b9b039315b 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll @@ -52,40 +52,6 @@ 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. */ @@ -108,12 +74,12 @@ private module TaintedPathConfig implements DataFlow::StateConfigSig { s2 = Normalized() } + predicate isBarrier(DataFlow::Node node, FlowState state) { node.(Sanitizer).isBarrier(state) } + predicate isBarrierOut(DataFlow::Node node, FlowState state) { isAdditionalFlowStep(_, state, node, _) } - predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer } - predicate observeDiffInformedIncrementalMode() { any() } } 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 d492f20336b6..f1060d0416da 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 @@ -68,7 +68,12 @@ public void ProcessRequest(HttpContext ctx) string absolutePath = ctx.Request.MapPath("~MyTempFile"); string fullPath2 = Path.Combine(absolutePath, path); if (fullPath2.StartsWith(absolutePath + Path.DirectorySeparatorChar)) { - File.ReadAllText(fullPath2); // GOOD + File.ReadAllText(fullPath2); // BAD + } + + string normalizedFullPath2 = Path.GetFullPath(fullPath2); + if (normalizedFullPath2.StartsWith(absolutePath + Path.DirectorySeparatorChar)) { + File.ReadAllText(normalizedFullPath2); // 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 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