diff --git a/.github/workflows/sync-main.yml b/.github/workflows/sync-main.yml new file mode 100644 index 000000000000..6a5735e8f6ac --- /dev/null +++ b/.github/workflows/sync-main.yml @@ -0,0 +1,37 @@ +name: Sync Main +on: + schedule: + - cron: '55 * * * *' + workflow_dispatch: +jobs: + sync-main: + name: Sync-main + runs-on: ubuntu-latest + if: github.repository == 'microsoft/codeql' + permissions: + contents: write + steps: + - name: Checkout + uses: actions/checkout@v3 + with: + fetch-depth: 0 + token: ${{ secrets.WORKFLOW_TOKEN }} + - name: Git config + shell: bash + run: | + git config user.name Dilan Bhalla + git config user.email dilanbhalla@microsoft.com + - name: Fetch + shell: bash + run: | + set -x + git fetch + git remote add upstream https://github.com/github/codeql.git + git fetch upstream --tags --force + - name: Sync Main + shell: bash + run: | + git merge codeql-cli/latest + git push origin main + git push origin --tags --force + diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 000000000000..e138ec5d6a77 --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,41 @@ + + +## Security + +Microsoft takes the security of our software products and services seriously, which includes all source code repositories managed through our GitHub organizations, which include [Microsoft](https://github.com/microsoft), [Azure](https://github.com/Azure), [DotNet](https://github.com/dotnet), [AspNet](https://github.com/aspnet), [Xamarin](https://github.com/xamarin), and [our GitHub organizations](https://opensource.microsoft.com/). + +If you believe you have found a security vulnerability in any Microsoft-owned repository that meets [Microsoft's definition of a security vulnerability](https://aka.ms/opensource/security/definition), please report it to us as described below. + +## Reporting Security Issues + +**Please do not report security vulnerabilities through public GitHub issues.** + +Instead, please report them to the Microsoft Security Response Center (MSRC) at [https://msrc.microsoft.com/create-report](https://aka.ms/opensource/security/create-report). + +If you prefer to submit without logging in, send email to [secure@microsoft.com](mailto:secure@microsoft.com). If possible, encrypt your message with our PGP key; please download it from the [Microsoft Security Response Center PGP Key page](https://aka.ms/opensource/security/pgpkey). + +You should receive a response within 24 hours. If for some reason you do not, please follow up via email to ensure we received your original message. Additional information can be found at [microsoft.com/msrc](https://aka.ms/opensource/security/msrc). + +Please include the requested information listed below (as much as you can provide) to help us better understand the nature and scope of the possible issue: + + * Type of issue (e.g. buffer overflow, SQL injection, cross-site scripting, etc.) + * Full paths of source file(s) related to the manifestation of the issue + * The location of the affected source code (tag/branch/commit or direct URL) + * Any special configuration required to reproduce the issue + * Step-by-step instructions to reproduce the issue + * Proof-of-concept or exploit code (if possible) + * Impact of the issue, including how an attacker might exploit the issue + +This information will help us triage your report more quickly. + +If you are reporting for a bug bounty, more complete reports can contribute to a higher bounty award. Please visit our [Microsoft Bug Bounty Program](https://aka.ms/opensource/security/bounty) page for more details about our active programs. + +## Preferred Languages + +We prefer all communications to be in English. + +## Policy + +Microsoft follows the principle of [Coordinated Vulnerability Disclosure](https://aka.ms/opensource/security/cvd). + + diff --git a/cpp/ql/lib/change-notes/2023-10-12-additional-call-targets.md b/cpp/ql/lib/change-notes/2023-10-12-additional-call-targets.md new file mode 100644 index 000000000000..f87fba1f1720 --- /dev/null +++ b/cpp/ql/lib/change-notes/2023-10-12-additional-call-targets.md @@ -0,0 +1,4 @@ +--- +category: feature +--- +* Added a new class `AdditionalCallTarget` for specifying additional call targets. diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow.qll index 43bf134ea794..13909ae953b2 100644 --- a/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow.qll +++ b/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow.qll @@ -25,7 +25,7 @@ import cpp * Provides classes for performing local (intra-procedural) and * global (inter-procedural) data flow analyses. */ -deprecated module DataFlow { +module DataFlow { private import semmle.code.cpp.dataflow.internal.DataFlowImplSpecific private import codeql.dataflow.DataFlow import DataFlowMake diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow2.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow2.qll index 19ffa16b76c6..c87fc0c8e399 100644 --- a/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow2.qll +++ b/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow2.qll @@ -17,6 +17,6 @@ import cpp * Provides classes for performing local (intra-procedural) and * global (inter-procedural) data flow analyses. */ -deprecated module DataFlow2 { +module DataFlow2 { import semmle.code.cpp.dataflow.internal.DataFlowImpl2 } diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow3.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow3.qll index 554b2e155b4c..01d13afe9bb6 100644 --- a/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow3.qll +++ b/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow3.qll @@ -17,6 +17,6 @@ import cpp * Provides classes for performing local (intra-procedural) and * global (inter-procedural) data flow analyses. */ -deprecated module DataFlow3 { +module DataFlow3 { import semmle.code.cpp.dataflow.internal.DataFlowImpl3 } diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow4.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow4.qll index fdd4e8ab7aef..6d7598db9d10 100644 --- a/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow4.qll +++ b/cpp/ql/lib/semmle/code/cpp/dataflow/DataFlow4.qll @@ -17,6 +17,6 @@ import cpp * Provides classes for performing local (intra-procedural) and * global (inter-procedural) data flow analyses. */ -deprecated module DataFlow4 { +module DataFlow4 { import semmle.code.cpp.dataflow.internal.DataFlowImpl4 } diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/TaintTracking.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/TaintTracking.qll index 8a8db1bdcce4..dc4066b16d4e 100644 --- a/cpp/ql/lib/semmle/code/cpp/dataflow/TaintTracking.qll +++ b/cpp/ql/lib/semmle/code/cpp/dataflow/TaintTracking.qll @@ -24,7 +24,7 @@ import semmle.code.cpp.dataflow.DataFlow2 * Provides classes for performing local (intra-procedural) and * global (inter-procedural) taint-tracking analyses. */ -deprecated module TaintTracking { +module TaintTracking { import semmle.code.cpp.dataflow.internal.tainttracking1.TaintTrackingParameter::Public private import semmle.code.cpp.dataflow.internal.DataFlowImplSpecific private import semmle.code.cpp.dataflow.internal.TaintTrackingImplSpecific diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/TaintTracking2.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/TaintTracking2.qll index dce00316cbbf..93ac23be468b 100644 --- a/cpp/ql/lib/semmle/code/cpp/dataflow/TaintTracking2.qll +++ b/cpp/ql/lib/semmle/code/cpp/dataflow/TaintTracking2.qll @@ -17,6 +17,6 @@ * Provides classes for performing local (intra-procedural) and * global (inter-procedural) taint-tracking analyses. */ -deprecated module TaintTracking2 { +module TaintTracking2 { import semmle.code.cpp.dataflow.internal.tainttracking2.TaintTrackingImpl } diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowDispatch.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowDispatch.qll index 9774ad7168b8..86c64edc847c 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowDispatch.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowDispatch.qll @@ -7,9 +7,12 @@ private import DataFlowImplCommon as DataFlowImplCommon /** * Gets a function that might be called by `call`. + * + * This predicate does not take additional call targets + * from `AdditionalCallTarget` into account. */ cached -DataFlowCallable viableCallable(DataFlowCall call) { +DataFlowCallable defaultViableCallable(DataFlowCall call) { DataFlowImplCommon::forceCachingInSameStage() and result = call.getStaticCallTarget() or @@ -29,6 +32,17 @@ DataFlowCallable viableCallable(DataFlowCall call) { result = call.(VirtualDispatch::DataSensitiveCall).resolve() } +/** + * Gets a function that might be called by `call`. + */ +cached +DataFlowCallable viableCallable(DataFlowCall call) { + result = defaultViableCallable(call) + or + // Additional call targets + result = any(AdditionalCallTarget additional).viableTarget(call.getUnconvertedResultExpression()) +} + /** * Provides virtual dispatch support compatible with the original * implementation of `semmle.code.cpp.security.TaintTracking`. diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll index fd628df907e1..992e995094e3 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll @@ -14,6 +14,7 @@ private import DataFlowPrivate private import ModelUtil private import SsaInternals as Ssa private import DataFlowImplCommon as DataFlowImplCommon +private import codeql.util.Unit /** * The IR dataflow graph consists of the following nodes: @@ -2237,3 +2238,43 @@ module InstructionBarrierGuard(x)` as if the user had written `f(x)`. + exists(FunctionTemplateInstantiation inst | + inst.getTemplate().hasName("call_template_argument") and + call.getTarget() = inst and + result = inst.getTemplateArgument(0).(FunctionAccess).getTarget() + ) + } +} + module IRConfig implements ConfigSig { predicate isSource(Node src) { src.asExpr() instanceof NewExpr diff --git a/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected b/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected index 9c30be7684a8..f6284e9713af 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected @@ -770,6 +770,9 @@ edges | simple.cpp:92:7:92:7 | a indirection [post update] [i] | simple.cpp:94:10:94:11 | a2 indirection [i] | | simple.cpp:92:11:92:20 | call to user_input | simple.cpp:92:5:92:22 | ... = ... | | simple.cpp:94:10:94:11 | a2 indirection [i] | simple.cpp:94:13:94:13 | i | +| simple.cpp:103:24:103:24 | x | simple.cpp:104:14:104:14 | x | +| simple.cpp:108:17:108:26 | call to user_input | simple.cpp:109:43:109:43 | x | +| simple.cpp:109:43:109:43 | x | simple.cpp:103:24:103:24 | x | | struct_init.c:14:24:14:25 | ab indirection [a] | struct_init.c:15:8:15:9 | ab indirection [a] | | struct_init.c:15:8:15:9 | ab indirection [a] | struct_init.c:15:12:15:12 | a | | struct_init.c:20:13:20:14 | definition of ab indirection [a] | struct_init.c:22:8:22:9 | ab indirection [a] | @@ -1576,6 +1579,10 @@ nodes | simple.cpp:92:11:92:20 | call to user_input | semmle.label | call to user_input | | simple.cpp:94:10:94:11 | a2 indirection [i] | semmle.label | a2 indirection [i] | | simple.cpp:94:13:94:13 | i | semmle.label | i | +| simple.cpp:103:24:103:24 | x | semmle.label | x | +| simple.cpp:104:14:104:14 | x | semmle.label | x | +| simple.cpp:108:17:108:26 | call to user_input | semmle.label | call to user_input | +| simple.cpp:109:43:109:43 | x | semmle.label | x | | struct_init.c:14:24:14:25 | ab indirection [a] | semmle.label | ab indirection [a] | | struct_init.c:15:8:15:9 | ab indirection [a] | semmle.label | ab indirection [a] | | struct_init.c:15:12:15:12 | a | semmle.label | a | @@ -1782,6 +1789,7 @@ subpaths | simple.cpp:67:13:67:13 | i | simple.cpp:65:11:65:20 | call to user_input | simple.cpp:67:13:67:13 | i | i flows from $@ | simple.cpp:65:11:65:20 | call to user_input | call to user_input | | simple.cpp:84:14:84:20 | call to getf2f1 | simple.cpp:83:17:83:26 | call to user_input | simple.cpp:84:14:84:20 | call to getf2f1 | call to getf2f1 flows from $@ | simple.cpp:83:17:83:26 | call to user_input | call to user_input | | simple.cpp:94:13:94:13 | i | simple.cpp:92:11:92:20 | call to user_input | simple.cpp:94:13:94:13 | i | i flows from $@ | simple.cpp:92:11:92:20 | call to user_input | call to user_input | +| simple.cpp:104:14:104:14 | x | simple.cpp:108:17:108:26 | call to user_input | simple.cpp:104:14:104:14 | x | x flows from $@ | simple.cpp:108:17:108:26 | call to user_input | call to user_input | | struct_init.c:15:12:15:12 | a | struct_init.c:20:20:20:29 | call to user_input | struct_init.c:15:12:15:12 | a | a flows from $@ | struct_init.c:20:20:20:29 | call to user_input | call to user_input | | struct_init.c:15:12:15:12 | a | struct_init.c:27:7:27:16 | call to user_input | struct_init.c:15:12:15:12 | a | a flows from $@ | struct_init.c:27:7:27:16 | call to user_input | call to user_input | | struct_init.c:15:12:15:12 | a | struct_init.c:40:20:40:29 | call to user_input | struct_init.c:15:12:15:12 | a | a flows from $@ | struct_init.c:40:20:40:29 | call to user_input | call to user_input | diff --git a/cpp/ql/test/library-tests/dataflow/fields/simple.cpp b/cpp/ql/test/library-tests/dataflow/fields/simple.cpp index e4d4f70edb0a..36756689855d 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/simple.cpp +++ b/cpp/ql/test/library-tests/dataflow/fields/simple.cpp @@ -94,4 +94,21 @@ void single_field_test_typedef(A_typedef a) sink(a2.i); //$ ast,ir } +namespace TestAdditionalCallTargets { + + using TakesIntReturnsVoid = void(*)(int); + template + void call_template_argument(int); + + void call_sink(int x) { + sink(x); // $ ir + } + + void test_additional_call_targets() { + int x = user_input(); + call_template_argument(x); + } + +} + } // namespace Simple diff --git a/csharp/ql/lib/qlpack.yml b/csharp/ql/lib/qlpack.yml index 1d7e566d0b4c..7fffa28c17d4 100644 --- a/csharp/ql/lib/qlpack.yml +++ b/csharp/ql/lib/qlpack.yml @@ -8,6 +8,7 @@ upgrades: upgrades dependencies: codeql/controlflow: ${workspace} codeql/dataflow: ${workspace} + codeql/dataflowstack: ${workspace} codeql/mad: ${workspace} codeql/ssa: ${workspace} codeql/tutorial: ${workspace} diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/DataFlowStack.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/DataFlowStack.qll new file mode 100644 index 000000000000..e9c11947c45f --- /dev/null +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/DataFlowStack.qll @@ -0,0 +1,10 @@ + +private import codeql.dataflow.DataFlow +private import semmle.code.csharp.dataflow.internal.DataFlowImplSpecific + +private import codeql.dataflowstack.DataFlowStack as DFS +private import DFS::DataFlowStackMake as DataFlowStackFactory + +module DataFlowStackMake{ + import DataFlowStackFactory::FlowStack +} \ No newline at end of file diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowDispatch.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowDispatch.qll index 0a5877d48974..5dbe19579d59 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowDispatch.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowDispatch.qll @@ -291,6 +291,9 @@ abstract class DataFlowCall extends TDataFlowCall { /** Gets the argument at position `pos` of this call. */ final ArgumentNode getArgument(ArgumentPosition pos) { result.argumentOf(this, pos) } + /** Gets an argument of this call. */ + final ArgumentNode getAnArgument() { result.argumentOf(this, _) } + /** Gets a textual representation of this call. */ abstract string toString(); diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll index 4b1069eff0e7..5d5e9016c183 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll @@ -2,157 +2,477 @@ * Provides a taint tracking configuration for reasoning about unsafe zip extraction. */ -import csharp -private import semmle.code.csharp.controlflow.Guards - -/** - * A data flow source for unsafe zip extraction. - */ -abstract class Source extends DataFlow::Node { } - -/** - * A data flow sink for unsafe zip extraction. - */ -abstract class Sink extends DataFlow::ExprNode { } - -/** - * A sanitizer for unsafe zip extraction. - */ -abstract class Sanitizer extends DataFlow::ExprNode { } - -/** - * DEPRECATED: Use `ZipSlip` instead. + import csharp + private import semmle.code.csharp.controlflow.Guards + private import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph + + private abstract class AbstractSanitizerMethod extends Method { } + + class MethodSystemStringStartsWith extends AbstractSanitizerMethod { + MethodSystemStringStartsWith() { this.getQualifiedName() = "System.String.StartsWith" } + } + + private abstract class UnsanitizedPathCombiner extends Expr { } + + class PathCombinerViaMethodCall extends UnsanitizedPathCombiner { + PathCombinerViaMethodCall() { + this.(MethodCall).getTarget().hasQualifiedName("System.IO.Path", "Combine") + } + } + + class PathCombinerViaStringInterpolation extends UnsanitizedPathCombiner { + PathCombinerViaStringInterpolation() { exists(InterpolatedStringExpr e | this = e) } + } + + class PathCombinerViaStringConcatenation extends UnsanitizedPathCombiner { + PathCombinerViaStringConcatenation() { exists(AddExpr e | this = e) } + } + + class MethodCallGetFullPath extends MethodCall { + MethodCallGetFullPath() { this.getTarget().hasQualifiedName("System.IO.Path", "GetFullPath") } + } + + /** + * A taint tracking module for GetFullPath to String.StartsWith. + */ + module GetFullPathToQualifierTT = TaintTracking::Global; + + private module GetFullPathToQualifierTaintTrackingConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node node) { + exists(MethodCallGetFullPath mcGetFullPath | node = DataFlow::exprNode(mcGetFullPath)) + } + predicate isSink(DataFlow::Node node) { + exists(MethodCall mc | + mc.getTarget() instanceof MethodSystemStringStartsWith and + node.asExpr() = mc.getQualifier() + ) + } + } + + /** + * DEPRECATED: Use `ZipSlip` instead. + * + * A taint tracking configuration for Zip Slip. + */ + deprecated class TaintTrackingConfiguration extends TaintTracking::Configuration { + TaintTrackingConfiguration() { this = "ZipSlipTaintTracking" } + + override predicate isSource(DataFlow::Node source) { source instanceof Source } + + override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + + override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer } + } + + /** + * A taint tracking configuration for Zip Slip. + */ + private module ZipSlipConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof Source } + + predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + + predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer } + } + + /** + * A taint tracking module for Zip Slip. + */ + module ZipSlip = TaintTracking::Global; + + /** An access to the `FullName` property of a `ZipArchiveEntry`. */ + class ArchiveFullNameSource extends Source { + ArchiveFullNameSource() { + exists(PropertyAccess pa | this.asExpr() = pa | + pa.getTarget().getDeclaringType().hasQualifiedName("System.IO.Compression", "ZipArchiveEntry") and + pa.getTarget().getName() = "FullName" + ) + } + } + + + /** + * A taint tracking module for String combining to GetFullPath. + */ + module PathCombinerToGetFullPathTT = TaintTracking::Global; + + /** + * PathCombinerToGetFullPathTaintTrackingConfiguration - A Taint Tracking configuration that tracks + * a File path combining expression (Such as string concatenation, Path.Combine, or string interpolation), + * to a Path.GetFullPath method call's argument. + * + * We need this because we need to find a safe sequence of operations wherein + * - An absolute path is created (uncanonicalized) + * - The Path is canonicalized + * + * If the operations are in the opposite order, the resultant may still contain path traversal characters, + * as you cannot fully resolve a relative path. So we must ascertain that they are conducted in this sequence. + */ + private module PathCombinerToGetFullPathTaintTrackingConfiguration implements DataFlow::ConfigSig { + /** + * We are looking for the result of some Path combining operation (String concat, Path.Combine, etc.) + */ + predicate isSource(DataFlow::Node node) { + exists(UnsanitizedPathCombiner pathCombiner | node = DataFlow::exprNode(pathCombiner)) + } + + /** + * Find the first (and only) argument of Path.GetFullPath, so we make sure that our expression + * first goes through some path combining function, and then is canonicalized. + */ + predicate isSink(DataFlow::Node node) { + exists(MethodCallGetFullPath mcGetFullPath | + node = DataFlow::exprNode(mcGetFullPath.getArgument(0)) + ) + } + } + + /** Predicate to check for a safe sequence of events + * Path.Combine THEN Path.GetFullPath is applied (with possibly arbitrary mutations) + */ + private predicate safeCombineGetFullPathSequence(MethodCallGetFullPath mcGetFullPath, Expr q) { + exists(UnsanitizedPathCombiner source | + PathCombinerToGetFullPathTT::flow(DataFlow::exprNode(source), DataFlow::exprNode(mcGetFullPath.getArgument(0))) + ) and + GetFullPathToQualifierTT::flow(DataFlow::exprNode(mcGetFullPath), DataFlow::exprNode(q)) + } + + /** + * The set of /valid/ Guards of RootSanitizerMethodCall. * - * A taint tracking configuration for Zip Slip. - */ -deprecated class TaintTrackingConfiguration extends TaintTracking::Configuration { - TaintTrackingConfiguration() { this = "ZipSlipTaintTracking" } - - override predicate isSource(DataFlow::Node source) { source instanceof Source } - - override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } - - override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer } -} - -/** - * A taint tracking configuration for Zip Slip. - */ -private module ZipSlipConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node source) { source instanceof Source } - - predicate isSink(DataFlow::Node sink) { sink instanceof Sink } - - predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer } -} - -/** - * A taint tracking module for Zip Slip. - */ -module ZipSlip = TaintTracking::Global; - -/** An access to the `FullName` property of a `ZipArchiveEntry`. */ -class ArchiveFullNameSource extends Source { - ArchiveFullNameSource() { - exists(PropertyAccess pa | this.asExpr() = pa | - pa.getTarget().getDeclaringType().hasQualifiedName("System.IO.Compression", "ZipArchiveEntry") and - pa.getTarget().getName() = "FullName" - ) - } -} - -/** An argument to the `ExtractToFile` extension method. */ -class ExtractToFileArgSink extends Sink { - ExtractToFileArgSink() { - exists(MethodCall mc | - mc.getTarget().hasQualifiedName("System.IO.Compression", "ZipFileExtensions", "ExtractToFile") and - this.asExpr() = mc.getArgumentForName("destinationFileName") - ) - } -} - -/** A path argument to a `File.Open`, `File.OpenWrite`, or `File.Create` method call. */ -class FileOpenArgSink extends Sink { - FileOpenArgSink() { - exists(MethodCall mc | - mc.getTarget().hasQualifiedName("System.IO", "File", "Open") or - mc.getTarget().hasQualifiedName("System.IO", "File", "OpenWrite") or - mc.getTarget().hasQualifiedName("System.IO", "File", "Create") - | - this.asExpr() = mc.getArgumentForName("path") - ) - } -} - -/** A path argument to a call to the `FileStream` constructor. */ -class FileStreamArgSink extends Sink { - FileStreamArgSink() { - exists(ObjectCreation oc | - oc.getTarget().getDeclaringType().hasQualifiedName("System.IO", "FileStream") - | - this.asExpr() = oc.getArgumentForName("path") - ) - } -} - -/** - * A path argument to a call to the `FileStream` constructor. + * IN CONJUNCTION with BOTH + * Path.Combine + * AND Path.GetFullPath + * OR + * There is a direct flow from Path.GetFullPath to qualifier of RootSanitizerMethodCall. * - * This constructor can accept a tainted file name and subsequently be used to open a file stream. + * It is not simply enough for the qualifier of String.StartsWith + * to pass through Path.Combine without also passing through GetFullPath AFTER. */ -class FileInfoArgSink extends Sink { - FileInfoArgSink() { - exists(ObjectCreation oc | - oc.getTarget().getDeclaringType().hasQualifiedName("System.IO", "FileInfo") - | - this.asExpr() = oc.getArgumentForName("fileName") - ) - } -} - -/** - * A call to `GetFileName`. - * - * This is considered a sanitizer because it extracts just the file name, not the full path. + class RootSanitizerMethodCall extends SanitizerMethodCall { + RootSanitizerMethodCall() { + exists(MethodSystemStringStartsWith sm | this.getTarget() = sm) and + exists(Expr q, AbstractValue v | + this.getQualifier() = q and + v.(AbstractValues::BooleanValue).getValue() = true and + exists(MethodCallGetFullPath mcGetFullPath | safeCombineGetFullPathSequence(mcGetFullPath, q)) + ) + } + + override Expr getFilePathArgument() { result = this.getQualifier() } + } + + /** + * The set of Guards of RootSanitizerMethodCall that are used IN CONJUNCTION with + * Path.GetFullPath - it is not simply enough for the qualifier of String.StartsWith + * to pass through Path.Combine without also passing through GetFullPath. */ -class GetFileNameSanitizer extends Sanitizer { - GetFileNameSanitizer() { - exists(MethodCall mc | mc.getTarget().hasQualifiedName("System.IO", "Path", "GetFileName") | - this.asExpr() = mc - ) - } -} - -/** - * A call to `Substring`. + class ZipSlipGuard extends Guard { + ZipSlipGuard() { this instanceof SanitizerMethodCall } + + Expr getFilePathArgument() { result = this.(SanitizerMethodCall).getFilePathArgument() } + } + + private abstract class SanitizerMethodCall extends MethodCall { + SanitizerMethodCall() { this instanceof MethodCall } + + abstract Expr getFilePathArgument(); + } + + /** + * A taint tracking module for Zip Slip Guard. + */ + module SanitizedGuardTT = TaintTracking::Global; + + /** + * SanitizedGuardTaintTrackingConfiguration - A Taint Tracking configuration class to trace + * parameters of a function to calls to RootSanitizerMethodCall (String.StartsWith). + * + * For example, the following function: + * void exampleFn(String somePath){ + * somePath = Path.GetFullPath(somePath); + * ... + * if(somePath.startsWith("aaaaa")) + * ... + * ... + * } + */ + private module SanitizedGuardTaintTrackingConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + source instanceof DataFlow::ParameterNode + } + + predicate isSink(DataFlow::Node sink) { + exists(RootSanitizerMethodCall smc | + smc.getAnArgument() = sink.asExpr() or + smc.getQualifier() = sink.asExpr() + ) + } + } + + /** + * An AbstractWrapperSanitizerMethod is a Method that + * is a suitable sanitizer for a ZipSlip path that may not have been canonicalized prior. + * + * If the return value of this Method correctly validates if a file path is in a valid location, + * or is a restricted subset of that validation, then any use of this Method is as valid as the Root + * sanitizer (Path.StartsWith). + */ + private abstract class AbstractWrapperSanitizerMethod extends AbstractSanitizerMethod { + + Parameter paramFilename; + + AbstractWrapperSanitizerMethod() { + this.getReturnType() instanceof BoolType and + this.getAParameter() = paramFilename + } + + Parameter paramFilePath() { result = paramFilename } + } + + /** + * A DirectWrapperSantizierMethod is a Method where + * The function can /only/ returns true when passes through the RootSanitizerGuard * - * This is considered a sanitizer because `Substring` may be used to extract a single component - * of a path to avoid ZipSlip. - */ -class SubstringSanitizer extends Sanitizer { - SubstringSanitizer() { - exists(MethodCall mc | mc.getTarget().hasQualifiedName("System", "String", "Substring") | - this.asExpr() = mc - ) - } -} - -private predicate stringCheckGuard(Guard g, Expr e, AbstractValue v) { - g.(MethodCall).getTarget().hasQualifiedName("System", "String", "StartsWith") and - g.(MethodCall).getQualifier() = e and - // A StartsWith check against Path.Combine is not sufficient, because the ".." elements have - // not yet been resolved. - not exists(MethodCall combineCall | - combineCall.getTarget().hasQualifiedName("System.IO", "Path", "Combine") and - DataFlow::localExprFlow(combineCall, e) - ) and - v.(AbstractValues::BooleanValue).getValue() = true -} - -/** - * A call to `String.StartsWith()` that indicates that the tainted path value is being - * validated to ensure that it occurs within a permitted output path. + * bool wrapperFn(a,b){ + * if(guard(a,b)) + * return true + * .... + * return false + * } + * + * bool wrapperFn(a,b){ + * ... + * return guard(a,b) + * } */ -class StringCheckSanitizer extends Sanitizer { - StringCheckSanitizer() { this = DataFlow::BarrierGuard::getABarrierNode() } -} + class DirectWrapperSantizierMethod extends AbstractWrapperSanitizerMethod { + + /** + * To be declared a Wrapper, a function must: + * - Be a predicate (return a boolean) + * - Accept and use a parameter which represents a File path + * - Contain a call to another sanitizer + * - And can only return true if the sanitizer also returns true. + */ + DirectWrapperSantizierMethod() { + // For every return statement in this Method, + forex(ReturnStmt ret | ret.getEnclosingCallable() = this | + // The function returns false (Fails the Guard) + ret.getExpr().(BoolLiteral).getBoolValue() = false + or + // It passes the guard, contraining the function argument to the Guard argument. + exists(ZipSlipGuard g, DataFlow::Node source, DataFlow::Node sink | + g.getEnclosingCallable() = this and + source = DataFlow::parameterNode(paramFilename) and + sink = DataFlow::exprNode(g.getFilePathArgument()) and + SanitizedGuardTT::flow(source, sink) and + ( + exists(AbstractValues::BooleanValue bv | + // If there exists a control block that guards against misuse + bv.getValue() = true and + g.controlsNode(ret.getAControlFlowNode(), bv) + ) + or + // Or if the function returns the resultant of the guard call + DataFlow::localFlow(DataFlow::exprNode(g), DataFlow::exprNode(ret.getExpr())) + ) + ) + ) + } + } + + /** + * An IndirectOverloadedWrapperSanitizerMethod is a Method in which simply wraps /another/ wrapper.class + * + * Usually this will look like the following stanza: + * boolean someWrapper(string s){ + * return someWrapper(s, true); + * } + */ + class IndirectOverloadedWrapperSantizierMethod extends AbstractWrapperSanitizerMethod { + + /** + * To be declared a Wrapper, a function must: + * - Be a predicate (return a boolean) + * - Accept and use a parameter which represents a File path (via delegation) + * - Contain a call to another sanitizer (via delegation) + * - And can only return true if the delegate sanitizer also returns true. + */ + IndirectOverloadedWrapperSantizierMethod() { + // For every return statement in our Method, + forex(ReturnStmt ret | ret.getEnclosingCallable() = this | + // The Return statement returns false OR + ret.getExpr().(BoolLiteral).getBoolValue() = false + or + // The Method returns the result of calling another known-good sanitizer, connecting + // the parameters of this function to the sanitizer MethodCall. + exists(ZipSlipGuard g | + // If the parameter flows directly to SanitizerMethodCall, and the resultant is returned + DataFlow::localFlow(DataFlow::parameterNode(paramFilename), + DataFlow::exprNode(g.getFilePathArgument())) and + DataFlow::localFlow(DataFlow::exprNode(g), DataFlow::exprNode(ret.getExpr())) + ) + ) + } + } + + /** + * A Wrapped Sanitizer Method call (some function that is equally or more restrictive than our root sanitizer) + * + * bool wrapperMethod(string path){ + * return realSanitizer(path); + * } + */ + class WrapperSanitizerMethodCall extends SanitizerMethodCall { + + AbstractWrapperSanitizerMethod wrapperMethod; + + WrapperSanitizerMethodCall() { + exists(AbstractWrapperSanitizerMethod sm | + this.getTarget() = sm and + wrapperMethod = sm + ) + } + + override Expr getFilePathArgument() { + result = this.getArgument(wrapperMethod.paramFilePath().getIndex()) + } + } + + private predicate wrapperCheckGuard(Guard g, Expr e, AbstractValue v) { + // A given wrapper method call, with the filePathArgument as a sink, that returns 'true' + g instanceof WrapperSanitizerMethodCall and + g.(WrapperSanitizerMethodCall).getFilePathArgument() = e and + v.(AbstractValues::BooleanValue).getValue() = true + } + + /** + * A sanitizer for unsafe zip extraction. + */ + private abstract class Sanitizer extends DataFlow::ExprNode { } + + class WrapperCheckSanitizer extends Sanitizer { + // A Wrapped RootSanitizer that is an explicit subset of RootSanitizer + WrapperCheckSanitizer() { this = DataFlow::BarrierGuard::getABarrierNode() } + } + + /** + * A data flow source for unsafe zip extraction. + */ + private abstract class Source extends DataFlow::Node { } + + /** + * Access to the `FullName` property of the archive item + */ + class ArchiveEntryFullName extends Source { + ArchiveEntryFullName() { + exists(PropertyAccess pa | + pa.getTarget().hasQualifiedName("System.IO.Compression.ZipArchiveEntry", "FullName") and + this = DataFlow::exprNode(pa) + ) + } + } + + /** + * A data flow sink for unsafe zip extraction. + */ + private abstract class Sink extends DataFlow::Node { } + + /** + * Argument to extract to file extension method + */ + class SinkCompressionExtractToFileArgument extends Sink { + SinkCompressionExtractToFileArgument() { + exists(MethodCall mc | + mc.getTarget().hasQualifiedName("System.IO.Compression.ZipFileExtensions", "ExtractToFile") and + this.asExpr() = mc.getArgumentForName("destinationFileName") + ) + } + } + + /** + * File Stream created from tainted file name through File.Open/File.Create + */ + class SinkFileOpenArgument extends Sink { + SinkFileOpenArgument() { + exists(MethodCall mc | + ( + mc.getTarget().hasQualifiedName("System.IO.File", "Open") or + mc.getTarget().hasQualifiedName("System.IO.File", "OpenWrite") or + mc.getTarget().hasQualifiedName("System.IO.File", "Create") + ) and + this.asExpr() = mc.getArgumentForName("path") + ) + } + } + + /** + * File Stream created from tainted file name passed directly to the constructor + */ + class SinkStreamConstructorArgument extends Sink { + SinkStreamConstructorArgument() { + exists(ObjectCreation oc | + oc.getTarget().getDeclaringType().hasQualifiedName("System.IO", "FileStream") and + this.asExpr() = oc.getArgumentForName("path") + ) + } + } + + + /** + * Constructor to FileInfo can take tainted file name and subsequently be used to open file stream + */ + class SinkFileInfoConstructorArgument extends Sink { + SinkFileInfoConstructorArgument() { + exists(ObjectCreation oc | + oc.getTarget().getDeclaringType().hasQualifiedName("System.IO", "FileInfo") and + this.asExpr() = oc.getArgumentForName("fileName") + ) + } + } + + /** + * Extracting just file name from a ZipEntry, not the full path + */ + class FileNameExtrationSanitizer extends Sanitizer { + FileNameExtrationSanitizer() { + exists(MethodCall mc | + mc.getTarget().hasQualifiedName("System.IO.Path", "GetFileName") and + this = DataFlow::exprNode(mc.getAnArgument()) + ) + } + } + + /** + * Checks the string for relative path, + * or checks the destination folder for whitelisted/target path, etc + */ + class StringCheckSanitizer extends Sanitizer { + StringCheckSanitizer() { + exists(MethodCall mc | + ( + mc instanceof RootSanitizerMethodCall or + mc.getTarget().hasQualifiedName("System.String", "Substring") + ) and + this = DataFlow::exprNode(mc.getQualifier()) + ) + } + } + + final class ZipSlipTaintTrackingConfiguration extends TaintTracking::Configuration { + ZipSlipTaintTrackingConfiguration() { this = "ZipSlipTaintTrackingConfiguration" } + + override predicate isSource(DataFlow::Node node) { node instanceof Source } + + override predicate isSink(DataFlow::Node node) { node instanceof Sink } + + override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + super.isAdditionalTaintStep(pred, succ) + or + exists(MethodCall mc | succ.asExpr() = mc and pred.asExpr() = mc.getAnArgument()) + } + + override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer } + } + diff --git a/csharp/ql/src/Security Features/CWE-022/ZipSlip.qhelp b/csharp/ql/src/Security Features/CWE-022/ZipSlip.qhelp index a1f39d27b8ce..9d9aedbdeac2 100644 --- a/csharp/ql/src/Security Features/CWE-022/ZipSlip.qhelp +++ b/csharp/ql/src/Security Features/CWE-022/ZipSlip.qhelp @@ -26,7 +26,7 @@ written to c:\sneaky-file.

Ensure that output paths constructed from zip archive entries are validated to prevent writing files to unexpected locations.

-

The recommended way of writing an output file from a zip archive entry is to:

+

The recommended way of writing an output file from a zip archive entry is to conduct the following in sequence:

  1. Use Path.Combine(destinationDirectory, archiveEntry.FullName) to determine the raw diff --git a/csharp/ql/src/Security Features/CWE-022/ZipSlip.ql b/csharp/ql/src/Security Features/CWE-022/ZipSlip.ql index 2e4d59aaf7eb..a794ecdf79cf 100644 --- a/csharp/ql/src/Security Features/CWE-022/ZipSlip.ql +++ b/csharp/ql/src/Security Features/CWE-022/ZipSlip.ql @@ -19,5 +19,5 @@ import ZipSlip::PathGraph from ZipSlip::PathNode source, ZipSlip::PathNode sink where ZipSlip::flowPath(source, sink) select source.getNode(), source, sink, - "Unsanitized archive entry, which may contain '..', is used in a $@.", sink.getNode(), - "file system operation" + "Unsanitized archive entry, which may contain '..', is used in a $@.", sink.getNode(), + "file system operation" diff --git a/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.cs b/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.cs index 1ec93bba3edd..9c40aa224e8d 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.cs @@ -6,44 +6,88 @@ namespace ZipSlip { class Program { + private static readonly char DirectorySeparatorChar = '\\'; - public static void UnzipFileByFile(ZipArchive archive, - string destDirectory) + public static void UnzipFileByFile(ZipArchive archive, string destDirectory) { foreach (var entry in archive.Entries) { - string fullPath = Path.GetFullPath(entry.FullName); - string fileName = Path.GetFileName(entry.FullName); - string filename = entry.Name; - string file = entry.FullName; - if (!string.IsNullOrEmpty(file)) + string fullPath_relative = Path.GetFullPath(entry.FullName); + string filename_filenameOnly = Path.GetFileName(entry.FullName); + string filename_noPathTraversal = entry.Name; + string file_badDirectoryTraversal = entry.FullName; + if (!string.IsNullOrEmpty(file_badDirectoryTraversal)) { // BAD - string destFileName = Path.Combine(destDirectory, file); + string destFileName = Path.Combine(destDirectory, file_badDirectoryTraversal); entry.ExtractToFile(destFileName, true); // GOOD - string sanitizedFileName = Path.Combine(destDirectory, fileName); + string sanitizedFileName = Path.Combine(destDirectory, filename_filenameOnly); entry.ExtractToFile(sanitizedFileName, true); // BAD - string destFilePath = Path.Combine(destDirectory, fullPath); + string destFilePath = Path.Combine(destDirectory, fullPath_relative); entry.ExtractToFile(destFilePath, true); - // BAD: destFilePath isn't fully resolved, so may still contain .. - if (destFilePath.StartsWith(destDirectory)) - entry.ExtractToFile(destFilePath, true); + unzipWrapperProtected(destDirectory, entry); - // BAD - destFilePath = Path.GetFullPath(Path.Combine(destDirectory, fullPath)); - entry.ExtractToFile(destFilePath, true); + string destFilePath_notCanonicalized = destDirectory + "/" + fullPath_relative; + if (destFilePath_notCanonicalized.StartsWith(destDirectory)){ + // BAD: no canonicalization has been applied. Directory traversal characters + // could still be present ie C:\some\dir\..\..\abc.exe + entry.ExtractToFile(destFilePath_notCanonicalized, true); + } - // GOOD: a check for StartsWith against a fully resolved path - if (destFilePath.StartsWith(destDirectory)) - entry.ExtractToFile(destFilePath, true); + string destFilePath_fullyCanonicalized = Path.GetFullPath(destFilePath_notCanonicalized); + if (destFilePath_fullyCanonicalized.StartsWith(destDirectory)){ + // GOOD: canonicalization has been applied by GetFullPath, +StartsWith Barrier. + entry.ExtractToFile(destFilePath_fullyCanonicalized, true); + } + + string destFilePath_fullyCanonicalized2 = Path.GetFullPath(destFileName); + if (destFilePath_fullyCanonicalized2.StartsWith(destDirectory)){ + // GOOD: canonicalization has been applied by GetFullPath, +StartsWith Barrier. + entry.ExtractToFile(destFilePath_fullyCanonicalized2, true); + } } } } + + private static void unzipWrapperProtected(string destinationPath, ZipArchiveEntry entry){ + string fullpath = Path.Combine(destinationPath, entry.FullName); + string entry_fullpath = Path.GetFullPath(entry.FullName); + + // BAD: no canonicalization, no validation/guard. + entry.ExtractToFile(fullpath, true); + + if(ContainsPath(fullpath, destinationPath, true)){ + // GOOD - Barrier guard applied (canonicalization applied in ContainsPath) + entry.ExtractToFile(fullpath, true); + } + + if(!ContainsPath(fullpath, destinationPath, true)){ + // BAD: Failed guard + entry.ExtractToFile(fullpath, true); + Console.WriteLine("Path traversal detected"); + return; + } + + // GOOD: Path has been sanitized above and guarded for (by returning early) + entry.ExtractToFile(fullpath, true); + + if(ContainsPath(fullpath, destinationPath, true)){ + // GOOD: guarded by ContainsPath (with delegate calls to StartsWith) + entry.ExtractToFile(fullpath, true); + } + + // GOOD: path checking applied above (and function terminates early). + string destFilePath = Path.Combine(destinationPath, entry_fullpath); + if (!destFilePath.StartsWith(destinationPath)){ + return; + } + entry.ExtractToFile(fullpath, true); + } private static int UnzipToStream(Stream zipStream, string installDir) { @@ -115,6 +159,43 @@ private static int UnzipToStream(Stream zipStream, string installDir) return returnCode; } + public static string? AddBackslashIfNotPresent(string? path) + { + if (!string.IsNullOrEmpty(path) && path![path.Length - 1] != DirectorySeparatorChar) + { + path += DirectorySeparatorChar; + } + return path; + } + + public static bool ContainsPath(string? fullPath, string? path){ + return ContainsPath(fullPath, path, true); + } + + public static bool ContainsPath(string? fullPath, string? path, bool excludeSame) + { + try + { + fullPath = Path.GetFullPath(fullPath); + path = Path.GetFullPath(path); + + fullPath = AddBackslashIfNotPresent(fullPath); + path = AddBackslashIfNotPresent(path); + + var result = fullPath!.StartsWith(path, StringComparison.OrdinalIgnoreCase); + if (result && excludeSame) + { + return !fullPath.Equals(path, StringComparison.OrdinalIgnoreCase); + } + return result; + } + catch + { + // If there is any error, just return false + return false; + } + } + static void Main(string[] args) { string zipFileName; diff --git a/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.expected b/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.expected index a5b2d7166f77..e47404352b7a 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.expected +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.expected @@ -1,55 +1,55 @@ edges -| ZipSlip.cs:15:35:15:66 | call to method GetFullPath : String | ZipSlip.cs:30:71:30:78 | access to local variable fullPath : String | -| ZipSlip.cs:15:35:15:66 | call to method GetFullPath : String | ZipSlip.cs:38:81:38:88 | access to local variable fullPath : String | -| ZipSlip.cs:15:52:15:65 | access to property FullName : String | ZipSlip.cs:15:35:15:66 | call to method GetFullPath : String | -| ZipSlip.cs:18:31:18:44 | access to property FullName : String | ZipSlip.cs:22:71:22:74 | access to local variable file : String | -| ZipSlip.cs:22:43:22:75 | call to method Combine : String | ZipSlip.cs:23:41:23:52 | access to local variable destFileName | -| ZipSlip.cs:22:71:22:74 | access to local variable file : String | ZipSlip.cs:22:43:22:75 | call to method Combine : String | -| ZipSlip.cs:30:43:30:79 | call to method Combine : String | ZipSlip.cs:31:41:31:52 | access to local variable destFilePath | -| ZipSlip.cs:30:43:30:79 | call to method Combine : String | ZipSlip.cs:35:45:35:56 | access to local variable destFilePath | -| ZipSlip.cs:30:71:30:78 | access to local variable fullPath : String | ZipSlip.cs:30:43:30:79 | call to method Combine : String | -| ZipSlip.cs:38:36:38:90 | call to method GetFullPath : String | ZipSlip.cs:39:41:39:52 | access to local variable destFilePath | -| ZipSlip.cs:38:53:38:89 | call to method Combine : String | ZipSlip.cs:38:36:38:90 | call to method GetFullPath : String | -| ZipSlip.cs:38:81:38:88 | access to local variable fullPath : String | ZipSlip.cs:38:53:38:89 | call to method Combine : String | -| ZipSlip.cs:61:47:61:86 | call to method Combine : String | ZipSlip.cs:68:74:68:85 | access to local variable destFilePath | -| ZipSlip.cs:61:47:61:86 | call to method Combine : String | ZipSlip.cs:75:71:75:82 | access to local variable destFilePath | -| ZipSlip.cs:61:47:61:86 | call to method Combine : String | ZipSlip.cs:82:57:82:68 | access to local variable destFilePath | -| ZipSlip.cs:61:47:61:86 | call to method Combine : String | ZipSlip.cs:90:58:90:69 | access to local variable destFilePath | -| ZipSlip.cs:61:72:61:85 | access to property FullName : String | ZipSlip.cs:61:47:61:86 | call to method Combine : String | +| ZipSlip.cs:15:44:15:75 | call to method GetFullPath : String | ZipSlip.cs:30:71:30:87 | access to local variable fullPath_relative : String | +| ZipSlip.cs:15:44:15:75 | call to method GetFullPath : String | ZipSlip.cs:39:45:39:73 | access to local variable destFilePath_notCanonicalized | +| ZipSlip.cs:15:61:15:74 | access to property FullName : String | ZipSlip.cs:15:44:15:75 | call to method GetFullPath : String | +| ZipSlip.cs:18:53:18:66 | access to property FullName : String | ZipSlip.cs:22:71:22:96 | access to local variable file_badDirectoryTraversal : String | +| ZipSlip.cs:22:43:22:97 | call to method Combine : String | ZipSlip.cs:23:41:23:52 | access to local variable destFileName | +| ZipSlip.cs:22:71:22:96 | access to local variable file_badDirectoryTraversal : String | ZipSlip.cs:22:43:22:97 | call to method Combine : String | +| ZipSlip.cs:30:43:30:88 | call to method Combine : String | ZipSlip.cs:31:41:31:52 | access to local variable destFilePath | +| ZipSlip.cs:30:71:30:87 | access to local variable fullPath_relative : String | ZipSlip.cs:30:43:30:88 | call to method Combine : String | +| ZipSlip.cs:58:31:58:75 | call to method Combine : String | ZipSlip.cs:62:33:62:40 | access to local variable fullpath | +| ZipSlip.cs:58:31:58:75 | call to method Combine : String | ZipSlip.cs:71:37:71:44 | access to local variable fullpath | +| ZipSlip.cs:58:61:58:74 | access to property FullName : String | ZipSlip.cs:58:31:58:75 | call to method Combine : String | +| ZipSlip.cs:105:47:105:86 | call to method Combine : String | ZipSlip.cs:112:74:112:85 | access to local variable destFilePath | +| ZipSlip.cs:105:47:105:86 | call to method Combine : String | ZipSlip.cs:119:71:119:82 | access to local variable destFilePath | +| ZipSlip.cs:105:47:105:86 | call to method Combine : String | ZipSlip.cs:126:57:126:68 | access to local variable destFilePath | +| ZipSlip.cs:105:47:105:86 | call to method Combine : String | ZipSlip.cs:134:58:134:69 | access to local variable destFilePath | +| ZipSlip.cs:105:72:105:85 | access to property FullName : String | ZipSlip.cs:105:47:105:86 | call to method Combine : String | | ZipSlipBad.cs:9:31:9:73 | call to method Combine : String | ZipSlipBad.cs:10:29:10:40 | access to local variable destFileName | | ZipSlipBad.cs:9:59:9:72 | access to property FullName : String | ZipSlipBad.cs:9:31:9:73 | call to method Combine : String | nodes -| ZipSlip.cs:15:35:15:66 | call to method GetFullPath : String | semmle.label | call to method GetFullPath : String | -| ZipSlip.cs:15:52:15:65 | access to property FullName : String | semmle.label | access to property FullName : String | -| ZipSlip.cs:18:31:18:44 | access to property FullName : String | semmle.label | access to property FullName : String | -| ZipSlip.cs:22:43:22:75 | call to method Combine : String | semmle.label | call to method Combine : String | -| ZipSlip.cs:22:71:22:74 | access to local variable file : String | semmle.label | access to local variable file : String | +| ZipSlip.cs:15:44:15:75 | call to method GetFullPath : String | semmle.label | call to method GetFullPath : String | +| ZipSlip.cs:15:61:15:74 | access to property FullName : String | semmle.label | access to property FullName : String | +| ZipSlip.cs:18:53:18:66 | access to property FullName : String | semmle.label | access to property FullName : String | +| ZipSlip.cs:22:43:22:97 | call to method Combine : String | semmle.label | call to method Combine : String | +| ZipSlip.cs:22:71:22:96 | access to local variable file_badDirectoryTraversal : String | semmle.label | access to local variable file_badDirectoryTraversal : String | | ZipSlip.cs:23:41:23:52 | access to local variable destFileName | semmle.label | access to local variable destFileName | -| ZipSlip.cs:30:43:30:79 | call to method Combine : String | semmle.label | call to method Combine : String | -| ZipSlip.cs:30:71:30:78 | access to local variable fullPath : String | semmle.label | access to local variable fullPath : String | +| ZipSlip.cs:30:43:30:88 | call to method Combine : String | semmle.label | call to method Combine : String | +| ZipSlip.cs:30:71:30:87 | access to local variable fullPath_relative : String | semmle.label | access to local variable fullPath_relative : String | | ZipSlip.cs:31:41:31:52 | access to local variable destFilePath | semmle.label | access to local variable destFilePath | -| ZipSlip.cs:35:45:35:56 | access to local variable destFilePath | semmle.label | access to local variable destFilePath | -| ZipSlip.cs:38:36:38:90 | call to method GetFullPath : String | semmle.label | call to method GetFullPath : String | -| ZipSlip.cs:38:53:38:89 | call to method Combine : String | semmle.label | call to method Combine : String | -| ZipSlip.cs:38:81:38:88 | access to local variable fullPath : String | semmle.label | access to local variable fullPath : String | -| ZipSlip.cs:39:41:39:52 | access to local variable destFilePath | semmle.label | access to local variable destFilePath | -| ZipSlip.cs:61:47:61:86 | call to method Combine : String | semmle.label | call to method Combine : String | -| ZipSlip.cs:61:72:61:85 | access to property FullName : String | semmle.label | access to property FullName : String | -| ZipSlip.cs:68:74:68:85 | access to local variable destFilePath | semmle.label | access to local variable destFilePath | -| ZipSlip.cs:75:71:75:82 | access to local variable destFilePath | semmle.label | access to local variable destFilePath | -| ZipSlip.cs:82:57:82:68 | access to local variable destFilePath | semmle.label | access to local variable destFilePath | -| ZipSlip.cs:90:58:90:69 | access to local variable destFilePath | semmle.label | access to local variable destFilePath | +| ZipSlip.cs:39:45:39:73 | access to local variable destFilePath_notCanonicalized | semmle.label | access to local variable destFilePath_notCanonicalized | +| ZipSlip.cs:58:31:58:75 | call to method Combine : String | semmle.label | call to method Combine : String | +| ZipSlip.cs:58:61:58:74 | access to property FullName : String | semmle.label | access to property FullName : String | +| ZipSlip.cs:62:33:62:40 | access to local variable fullpath | semmle.label | access to local variable fullpath | +| ZipSlip.cs:71:37:71:44 | access to local variable fullpath | semmle.label | access to local variable fullpath | +| ZipSlip.cs:105:47:105:86 | call to method Combine : String | semmle.label | call to method Combine : String | +| ZipSlip.cs:105:72:105:85 | access to property FullName : String | semmle.label | access to property FullName : String | +| ZipSlip.cs:112:74:112:85 | access to local variable destFilePath | semmle.label | access to local variable destFilePath | +| ZipSlip.cs:119:71:119:82 | access to local variable destFilePath | semmle.label | access to local variable destFilePath | +| ZipSlip.cs:126:57:126:68 | access to local variable destFilePath | semmle.label | access to local variable destFilePath | +| ZipSlip.cs:134:58:134:69 | access to local variable destFilePath | semmle.label | access to local variable destFilePath | | ZipSlipBad.cs:9:31:9:73 | call to method Combine : String | semmle.label | call to method Combine : String | | ZipSlipBad.cs:9:59:9:72 | access to property FullName : String | semmle.label | access to property FullName : String | | ZipSlipBad.cs:10:29:10:40 | access to local variable destFileName | semmle.label | access to local variable destFileName | subpaths #select -| ZipSlip.cs:15:52:15:65 | access to property FullName | ZipSlip.cs:15:52:15:65 | access to property FullName : String | ZipSlip.cs:31:41:31:52 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:31:41:31:52 | access to local variable destFilePath | file system operation | -| ZipSlip.cs:15:52:15:65 | access to property FullName | ZipSlip.cs:15:52:15:65 | access to property FullName : String | ZipSlip.cs:35:45:35:56 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:35:45:35:56 | access to local variable destFilePath | file system operation | -| ZipSlip.cs:15:52:15:65 | access to property FullName | ZipSlip.cs:15:52:15:65 | access to property FullName : String | ZipSlip.cs:39:41:39:52 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:39:41:39:52 | access to local variable destFilePath | file system operation | -| ZipSlip.cs:18:31:18:44 | access to property FullName | ZipSlip.cs:18:31:18:44 | access to property FullName : String | ZipSlip.cs:23:41:23:52 | access to local variable destFileName | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:23:41:23:52 | access to local variable destFileName | file system operation | -| ZipSlip.cs:61:72:61:85 | access to property FullName | ZipSlip.cs:61:72:61:85 | access to property FullName : String | ZipSlip.cs:68:74:68:85 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:68:74:68:85 | access to local variable destFilePath | file system operation | -| ZipSlip.cs:61:72:61:85 | access to property FullName | ZipSlip.cs:61:72:61:85 | access to property FullName : String | ZipSlip.cs:75:71:75:82 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:75:71:75:82 | access to local variable destFilePath | file system operation | -| ZipSlip.cs:61:72:61:85 | access to property FullName | ZipSlip.cs:61:72:61:85 | access to property FullName : String | ZipSlip.cs:82:57:82:68 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:82:57:82:68 | access to local variable destFilePath | file system operation | -| ZipSlip.cs:61:72:61:85 | access to property FullName | ZipSlip.cs:61:72:61:85 | access to property FullName : String | ZipSlip.cs:90:58:90:69 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:90:58:90:69 | access to local variable destFilePath | file system operation | +| ZipSlip.cs:15:61:15:74 | access to property FullName | ZipSlip.cs:15:61:15:74 | access to property FullName : String | ZipSlip.cs:31:41:31:52 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:31:41:31:52 | access to local variable destFilePath | file system operation | +| ZipSlip.cs:15:61:15:74 | access to property FullName | ZipSlip.cs:15:61:15:74 | access to property FullName : String | ZipSlip.cs:39:45:39:73 | access to local variable destFilePath_notCanonicalized | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:39:45:39:73 | access to local variable destFilePath_notCanonicalized | file system operation | +| ZipSlip.cs:18:53:18:66 | access to property FullName | ZipSlip.cs:18:53:18:66 | access to property FullName : String | ZipSlip.cs:23:41:23:52 | access to local variable destFileName | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:23:41:23:52 | access to local variable destFileName | file system operation | +| ZipSlip.cs:58:61:58:74 | access to property FullName | ZipSlip.cs:58:61:58:74 | access to property FullName : String | ZipSlip.cs:62:33:62:40 | access to local variable fullpath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:62:33:62:40 | access to local variable fullpath | file system operation | +| ZipSlip.cs:58:61:58:74 | access to property FullName | ZipSlip.cs:58:61:58:74 | access to property FullName : String | ZipSlip.cs:71:37:71:44 | access to local variable fullpath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:71:37:71:44 | access to local variable fullpath | file system operation | +| ZipSlip.cs:105:72:105:85 | access to property FullName | ZipSlip.cs:105:72:105:85 | access to property FullName : String | ZipSlip.cs:112:74:112:85 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:112:74:112:85 | access to local variable destFilePath | file system operation | +| ZipSlip.cs:105:72:105:85 | access to property FullName | ZipSlip.cs:105:72:105:85 | access to property FullName : String | ZipSlip.cs:119:71:119:82 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:119:71:119:82 | access to local variable destFilePath | file system operation | +| ZipSlip.cs:105:72:105:85 | access to property FullName | ZipSlip.cs:105:72:105:85 | access to property FullName : String | ZipSlip.cs:126:57:126:68 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:126:57:126:68 | access to local variable destFilePath | file system operation | +| ZipSlip.cs:105:72:105:85 | access to property FullName | ZipSlip.cs:105:72:105:85 | access to property FullName : String | ZipSlip.cs:134:58:134:69 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:134:58:134:69 | access to local variable destFilePath | file system operation | | ZipSlipBad.cs:9:59:9:72 | access to property FullName | ZipSlipBad.cs:9:59:9:72 | access to property FullName : String | ZipSlipBad.cs:10:29:10:40 | access to local variable destFileName | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlipBad.cs:10:29:10:40 | access to local variable destFileName | file system operation | diff --git a/shared/dataflow/codeql/dataflow/DataFlow.qll b/shared/dataflow/codeql/dataflow/DataFlow.qll index f7a2429ee8ac..2438229e8421 100644 --- a/shared/dataflow/codeql/dataflow/DataFlow.qll +++ b/shared/dataflow/codeql/dataflow/DataFlow.qll @@ -54,10 +54,20 @@ signature module InputSig { Node exprNode(DataFlowExpr e); class DataFlowCall { + + /** + * Gets a run-time target of this call. A target is always a source + * declaration, and if the callable has both CIL and source code, only + * the source code version is returned. + */ + DataFlowCallable getARuntimeTarget(); + /** Gets a textual representation of this element. */ string toString(); DataFlowCallable getEnclosingCallable(); + + ArgumentNode getAnArgument(); } class DataFlowCallable { @@ -465,7 +475,11 @@ module DataFlowMake { * A `Node` augmented with a call context (except for sinks) and an access path. * Only those `PathNode`s that are reachable from a source, and which can reach a sink, are generated. */ - class PathNode; + class PathNode{ + Node getNode(); + + PathNode getASuccessor(); + } /** * Holds if data can flow from `source` to `sink`. diff --git a/shared/dataflowstack/codeql/dataflowstack/DataFlowStack.qll b/shared/dataflowstack/codeql/dataflowstack/DataFlowStack.qll new file mode 100644 index 000000000000..dd2eabc2bad9 --- /dev/null +++ b/shared/dataflowstack/codeql/dataflowstack/DataFlowStack.qll @@ -0,0 +1,197 @@ + +private import codeql.dataflow.DataFlow as DF + +module DataFlowStackMake{ + + import DF::DataFlowMake as DataFlow + + module FlowStack{ + + predicate eitherStackSubset(Flow::PathNode nodeA, Flow::PathNode nodeB){ + exists(StackFrame topNodeA, StackFrame topNodeB | + topNodeA = stackFrameForFlow(nodeA) and + topNodeB = stackFrameForFlow(nodeB) and ( + stackIsSubsetOf(topNodeA, topNodeB) + or + stackIsSubsetOf(topNodeB, topNodeA) + ) + ) + } + + predicate stackIsSubsetOf(StackFrame stackA, StackFrame stackB){ + // If the top of stackA is in stackB at all, and the bottom of stackA is some successor of stackB. + exists(StackFrame stackBIntermediary | + stackBIntermediary = stackB.getSuccessor*() and + stackA.getCall() = stackBIntermediary.getCall() and + stackA.getBottom().getCall() = stackBIntermediary.getSuccessor*().getCall() + ) + } + + predicate eitherStackTerminatingSubset(Flow::PathNode nodeA, Flow::PathNode nodeB){ + exists(StackFrame topNodeA, StackFrame topNodeB | + topNodeA = stackFrameForFlow(nodeA) and + topNodeB = stackFrameForFlow(nodeB) and ( + stackIsCovergingTerminatingSubsetOf(topNodeA, topNodeB) + or + stackIsCovergingTerminatingSubsetOf(topNodeB, topNodeA) + ) + ) + } + + predicate stackIsCovergingTerminatingSubsetOf(StackFrame stackA, StackFrame stackB){ + // If the top of stackA is in stackB at any location, and the bottoms of the stack are the same call. + stackA.getCall() = stackB.getSuccessor*().getCall() and + stackA.getBottom().getCall() = stackB.getBottom().getCall() + } + + + private newtype TStackFrameType = + TStackFrame(CallFrame callFrame, CallFrame bottom){ + callFrame.getSuccessor*() = bottom and + not exists(bottom.getSuccessor()) + } + + class StackFrame extends TStackFrameType, TStackFrame{ + string toString(){ + exists(CallFrame callFrame | + this = TStackFrame(callFrame, _) and + result = callFrame.toString() + ) + } + + StackFrame getSuccessor(){ + exists(StackFrame succ | + this = succ.getPredecessor() and + result = succ + ) + } + + /** Filter subsection of callframes that are parent calls in callgraph, that is, + * limit to only the functions that indirectly/directly call the sink. + */ + StackFrame getPredecessor(){ + exists(CallFrame callFrame, CallFrame bottom, CallFrame callFramePredecessor | + // Get the predecessor callframe which has a direct call inside the body to the current callFrame + this = TStackFrame(callFrame, bottom) and + callFramePredecessor = callFrame.getPredecessor*() and + callFramePredecessor.getCall().getARuntimeTarget() = callFrame.getCall().getEnclosingCallable() and + result = TStackFrame(callFramePredecessor, bottom) + ) + } + + Flow::PathNode getPathNode(){ + exists(CallFrame callFrame | + this = TStackFrame(callFrame, _) and + result = callFrame.getPathNode() + ) + } + + Lang::DataFlowCall getCall(){ + exists(CallFrame callFrame | + this = TStackFrame(callFrame, _) and + result = callFrame.getCall() + ) + } + + predicate matchesCallFrame(CallFrame callFrame){ + this.getPathNode() = callFrame.getPathNode() + } + + StackFrame getBottom(){ + result = this.getSuccessor*() and + not exists(result.getSuccessor()) + } + } + + StackFrame fromCallFrame(CallFrame callFrame){ + result = TStackFrame(callFrame, getBottomCallFrame(callFrame)) + } + + CallFrame getBottomCallFrame(CallFrame callFrame){ + exists(CallFrame bottom | + bottom = callFrame.getSuccessor*() and + not exists(bottom.getSuccessor()) and + result = bottom + ) + } + + private StackFrame stackFrameForFlow(Flow::PathNode node){ + // Get the CallFrame, then from the last callframe, track back up whereby each call's target should contain the next CallFrame. + exists(CallFrame callFrame | + callFrame = flowNodeCallFrames(node) and + callFrame.getCall().getAnArgument() = node.getNode() and + result = TStackFrame(callFrame, _) + ) + } + + private newtype TCallFrameType = + TCallFrame(Flow::PathNode node) { + exists(Lang::DataFlowCall c | + c.getAnArgument() = node.getNode() + ) + } + + private class CallFrame extends TCallFrameType, TCallFrame{ + string toString(){ + exists(Lang::DataFlowCall call | + call = this.getCall() and + // result = "l-" + call.getLocation().getStartLine() + ": " + call.toString() + result = call.toString() + ) + } + + CallFrame getSuccessor(){ + exists(Flow::PathNode node | + this = TCallFrame(node) and + exists(Flow::PathNode succ | + succ = getSuccessorCall(node) and + result = TCallFrame(succ) + ) + ) + } + + CallFrame getPredecessor(){ + exists(CallFrame prior | + prior.getSuccessor() = this and + result = prior + ) + } + + Lang::DataFlowCall getPredecessorCall(){ + result = this.getPredecessor().getCall() + } + + Lang::DataFlowCall getCall(){ + exists(Lang::DataFlowCall call, Flow::PathNode node | + this = TCallFrame(node) and + call.getAnArgument() = node.getNode() and + result = call + ) + } + + Flow::PathNode getPathNode(){ + exists(Flow::PathNode n | + this = TCallFrame(n) and + result = n + ) + } + } + + private Flow::PathNode getSuccessorCall(Flow::PathNode n){ + exists(Flow::PathNode succ | + succ = n.getASuccessor() and + if exists(Lang::DataFlowCall c | c.getAnArgument() = succ.getNode()) + then result = succ + else result = getSuccessorCall(succ) + ) + } + + private CallFrame flowNodeCallFrames(Flow::PathNode node){ + exists(Flow::PathNode subnode, Lang::DataFlowCall call | + subnode = node.getASuccessor*() and + call.getAnArgument() = subnode.getNode() and + result = TCallFrame(subnode) + ) + } + } +} \ No newline at end of file diff --git a/shared/dataflowstack/qlpack.yml b/shared/dataflowstack/qlpack.yml new file mode 100644 index 000000000000..8319c18e88b5 --- /dev/null +++ b/shared/dataflowstack/qlpack.yml @@ -0,0 +1,7 @@ +name: codeql/dataflowstack +version: 0.0.1 +groups: shared +library: true +dependencies: + codeql/dataflow: ${workspace} +warnOnImplicitThis: true \ No newline at end of file