From 5231a1fcd9e224090ebe9a48f6863384629a6e8d Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Thu, 21 Aug 2025 17:03:23 -0700 Subject: [PATCH 1/4] Add fialing test cases, whereby there is a call that validates and throws an exception prior to the sink --- .../CWE-022/ZipSlip/ZipSlip.cs | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) 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 089aa410a96d..f08547c42f1b 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 @@ -196,6 +196,22 @@ public static bool ContainsPath(string? fullPath, string? path, bool excludeSame } } + /* Test that the given `fullPath` exists within the given `path` directory. + * If it does not, throw an exception to terminate the request. + */ + public static void ContainsPathValidationThrowing(string? fullPath, string? path) + { + fullPath = Path.GetFullPath(fullPath); + path = Path.GetFullPath(path); + + fullPath = AddBackslashIfNotPresent(fullPath); + path = AddBackslashIfNotPresent(path); + + if (!fullPath!.StartsWith(path, StringComparison.OrdinalIgnoreCase)) { + throw new Exception("Attempting path traversal"); + } + } + static void Main(string[] args) { string zipFileName; @@ -231,5 +247,19 @@ static void fp_throw(ZipArchive archive, string root){ entry.ExtractToFile(destinationOnDisk, true); } } + + /** + * Negative - dangerous path terminates early due to exception thrown by guarded condition in descendent call. + */ + static void fp_throw_nested_exception(ZipArchive archive, string root){ + foreach (var entry in archive.Entries){ + string destinationOnDisk = Path.GetFullPath(Path.Combine(root, entry.FullName)); + string fullRoot = Path.GetFullPath(root + Path.DirectorySeparatorChar); + ContainsPathValidationThrowing(destinationOnDisk, fullRoot); + // NEGATIVE, above exception short circuits by exception on invalid input by path traversal. + entry.ExtractToFile(destinationOnDisk, true); + } + } + } } From 1f79f381559f7c73d829af7bb41bb885718c0828 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Thu, 21 Aug 2025 17:03:58 -0700 Subject: [PATCH 2/4] Breaking down some logic into primitives, remove dead comment --- .../csharp/security/dataflow/ZipSlipQuery.qll | 47 ++++++++++++------- 1 file changed, 29 insertions(+), 18 deletions(-) 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 fea4a1a46e8b..e991613d87bd 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll @@ -51,13 +51,26 @@ private module GetFullPathToQualifierTaintTrackingConfiguration implements DataF } } +class ZipArchiveEntryClass extends Class{ + ZipArchiveEntryClass(){ + this.hasFullyQualifiedName("System.IO.Compression", "ZipArchiveEntry") + } +} + +/** + * The `FullName` property of `System.IO.Compression.ZipArchiveEntry`. + */ +class ZipArchiveEntryFullNameAccess extends Property{ + ZipArchiveEntryFullNameAccess(){ + this.getDeclaringType() instanceof ZipArchiveEntryClass and + this.getName() = "FullName" + } +} + /** An access to the `FullName` property of a `ZipArchiveEntry`. */ class ArchiveFullNameSource extends Source { ArchiveFullNameSource() { - exists(PropertyAccess pa | this.asExpr() = pa | - pa.getTarget().getDeclaringType().hasFullyQualifiedName("System.IO.Compression", "ZipArchiveEntry") and - pa.getTarget().getName() = "FullName" - ) + exists(ZipArchiveEntryFullNameAccess pa | pa.getAnAccess() = this.asExpr()) } } @@ -125,10 +138,12 @@ private predicate safeCombineGetFullPathSequence(MethodCallGetFullPath mcGetFull class RootSanitizerMethodCall extends SanitizerMethodCall { RootSanitizerMethodCall() { exists(MethodSystemStringStartsWith sm | this.getTarget() = sm) and - exists(Expr q, AbstractValue v | + exists(Expr q, MethodCallGetFullPath mcGetFullPath | this.getQualifier() = q and - v.(AbstractValues::BooleanValue).getValue() = true and - exists(MethodCallGetFullPath mcGetFullPath | safeCombineGetFullPathSequence(mcGetFullPath, q)) + // JB1: Try and detect existentials with non-interelated variables + // , AbstractValue v + // v.(AbstractValues::BooleanValue).getValue() = true and + safeCombineGetFullPathSequence(mcGetFullPath, q) ) } @@ -179,9 +194,12 @@ private module SanitizedGuardTaintTrackingConfiguration implements DataFlow::Con } predicate isSink(DataFlow::Node sink) { - exists(RootSanitizerMethodCall smc | - smc.getAnArgument() = sink.asExpr() or - smc.getQualifier() = sink.asExpr() + exists(RootSanitizerMethodCall smc, Expr e | + e = sink.asExpr() and + e = [ + smc.getAnArgument(), + smc.getQualifier() + ] ) } } @@ -199,19 +217,12 @@ abstract private class AbstractWrapperSanitizerMethod extends AbstractSanitizerM AbstractWrapperSanitizerMethod() { this.getReturnType() instanceof BoolType and - this.getAParameter() = paramFilename + paramFilename = this.getAParameter() } Parameter paramFilePath() { result = paramFilename } } -/* predicate aaaa(ZipSlipGuard g, DataFlow::ParameterNode source){ - exists(DataFlow::Node sink | - sink = DataFlow::exprNode(g.getFilePathArgument()) and - SanitizedGuardTT::flow(source, sink) and - ) -} */ - /** * A DirectWrapperSantizierMethod is a Method where * The function can /only/ returns true when passes through the RootSanitizerGuard From 3840d928e9cb41bd03f98df250ff44be379f94b2 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Fri, 22 Aug 2025 16:45:58 -0700 Subject: [PATCH 3/4] Barrier node whereby there is a call that performs valid sanitization, and throws uncaught exception --- .../csharp/security/dataflow/ZipSlipQuery.qll | 37 +++++++++++++++ .../CWE-022/ZipSlip/ZipSlip.cs | 47 +++++++++++++++++++ .../CWE-022/ZipSlip/ZipSlip.expected | 14 ++++++ 3 files changed, 98 insertions(+) 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 e991613d87bd..dd8711ce0eb6 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll @@ -204,6 +204,32 @@ private module SanitizedGuardTaintTrackingConfiguration implements DataFlow::Con } } +/** + * A Callable that successfully validates a path will resolve under a given directory, + * and if it does not, throws an exception. + */ +private class ValidatingCallableThrowing extends Callable{ + Parameter paramFilename; + ValidatingCallableThrowing(){ + paramFilename = this.getAParameter() and + // It passes the guard, contraining the function argument to the Guard argument. + exists(ZipSlipGuard g, DataFlow::ParameterNode 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, ThrowStmt throw | + throw.getEnclosingCallable() = this and + forall(TryStmt try | try.getEnclosingCallable() = this | not throw.getParent+() = try) and + // If there exists a control block that guards against misuse + bv.getValue() = false and + g.controlsNode(throw.getAControlFlowNode(), bv) + ) + ) + } + Parameter paramFilePath() { result = paramFilename } +} + /** * An AbstractWrapperSanitizerMethod is a Method that * is a suitable sanitizer for a ZipSlip path that may not have been canonicalized prior. @@ -361,6 +387,17 @@ class WrapperCheckSanitizer extends Sanitizer { WrapperCheckSanitizer() { this = DataFlow::BarrierGuard::getABarrierNode() } } +/** + * A Call to `ValidatingCallableThrowing` which acts as a barrier in a DataFlow + */ +class ValidatingCallableThrowingSanitizer extends Sanitizer { + ValidatingCallableThrowingSanitizer(){ + exists(ValidatingCallableThrowing validator, Call validatorCall | validatorCall = validator.getACall() | + this = DataFlow::exprNode(validatorCall.getAnArgument()) + ) + } +} + /** * A data flow source for unsafe zip extraction. */ 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 f08547c42f1b..e7d7b5b50a71 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 @@ -261,5 +261,52 @@ static void fp_throw_nested_exception(ZipArchive archive, string root){ } } + /** + * Negative - no extraction, only sanitization + */ + static void fp_throw_sanitizer_valid(string file, string root){ + string destinationOnDisk = Path.GetFullPath(file); + string fullRoot = Path.GetFullPath(root + Path.DirectorySeparatorChar); + if (!destinationOnDisk.StartsWith(fullRoot)){ + throw new Exception("Entry is outside of target directory. There may have been some directory traversal sequences in filename."); + } + } + + /** + * Negative - dangerous path terminates early due to throw in fp_throw_sanitizer_valid + */ + static void fp_throw_nested_exception_uncaught(ZipArchive archive, string root){ + foreach (var entry in archive.Entries){ + string destinationOnDisk = Path.GetFullPath(Path.Combine(root, entry.FullName)); + string fullRoot = Path.GetFullPath(root + Path.DirectorySeparatorChar); + fp_throw_sanitizer_valid(destinationOnDisk, fullRoot); + entry.ExtractToFile(destinationOnDisk, true); + } + } + + /** + * Negative - no extraction, only sanitization + */ + static void fp_throw_sanitizer_invalid(string file, string root){ + try{ + string destinationOnDisk = Path.GetFullPath(file); + string fullRoot = Path.GetFullPath(root); + if (!destinationOnDisk.StartsWith(fullRoot)){ + throw new Exception("Entry is outside of target directory. There may have been some directory traversal sequences in filename."); + } + }catch(Exception e){} + } + + /** + * Positive - dangerous path does not terminate early due to try block in fp_throw_sanitizer_invalid + */ + static void tp_throw_nested_exception_caught(ZipArchive archive, string root){ + foreach (var entry in archive.Entries){ + string destinationOnDisk = Path.GetFullPath(Path.Combine(root, entry.FullName)); + string fullRoot = Path.GetFullPath(root + Path.DirectorySeparatorChar); + fp_throw_sanitizer_invalid(destinationOnDisk, fullRoot); + entry.ExtractToFile(destinationOnDisk, true); + } + } } } 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 0d84dccb6499..51ae3486d3b1 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 @@ -8,6 +8,7 @@ | 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 | +| ZipSlip.cs:305:80:305:93 | access to property FullName | ZipSlip.cs:305:80:305:93 | access to property FullName : String | ZipSlip.cs:308:37:308:53 | access to local variable destinationOnDisk | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:308:37:308:53 | access to local variable destinationOnDisk | 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 | edges | ZipSlip.cs:15:24:15:40 | access to local variable fullPath_relative : String | ZipSlip.cs:30:71:30:87 | access to local variable fullPath_relative : String | provenance | | @@ -47,6 +48,13 @@ edges | ZipSlip.cs:121:71:121:82 | access to local variable destFilePath : String | ZipSlip.cs:126:57:126:68 | access to local variable destFilePath | provenance | | | ZipSlip.cs:121:71:121:82 | access to local variable destFilePath : String | ZipSlip.cs:129:71:129:82 | access to local variable destFilePath : String | provenance | | | ZipSlip.cs:129:71:129:82 | access to local variable destFilePath : String | ZipSlip.cs:134:58:134:69 | access to local variable destFilePath | provenance | | +| ZipSlip.cs:305:24:305:40 | access to local variable destinationOnDisk : String | ZipSlip.cs:307:44:307:60 | access to local variable destinationOnDisk : String | provenance | | +| ZipSlip.cs:305:44:305:95 | call to method GetFullPath : String | ZipSlip.cs:305:24:305:40 | access to local variable destinationOnDisk : String | provenance | | +| ZipSlip.cs:305:61:305:94 | call to method Combine : String | ZipSlip.cs:305:44:305:95 | call to method GetFullPath : String | provenance | Config | +| ZipSlip.cs:305:61:305:94 | call to method Combine : String | ZipSlip.cs:305:44:305:95 | call to method GetFullPath : String | provenance | MaD:2 | +| ZipSlip.cs:305:80:305:93 | access to property FullName : String | ZipSlip.cs:305:61:305:94 | call to method Combine : String | provenance | Config | +| ZipSlip.cs:305:80:305:93 | access to property FullName : String | ZipSlip.cs:305:61:305:94 | call to method Combine : String | provenance | MaD:1 | +| ZipSlip.cs:307:44:307:60 | access to local variable destinationOnDisk : String | ZipSlip.cs:308:37:308:53 | access to local variable destinationOnDisk | provenance | | | ZipSlipBad.cs:9:16:9:27 | access to local variable destFileName : String | ZipSlipBad.cs:10:29:10:40 | access to local variable destFileName | provenance | | | ZipSlipBad.cs:9:31:9:73 | call to method Combine : String | ZipSlipBad.cs:9:16:9:27 | access to local variable destFileName : String | provenance | | | ZipSlipBad.cs:9:59:9:72 | access to property FullName : String | ZipSlipBad.cs:9:31:9:73 | call to method Combine : String | provenance | Config | @@ -91,6 +99,12 @@ nodes | ZipSlip.cs:126:57:126:68 | access to local variable destFilePath | semmle.label | access to local variable destFilePath | | ZipSlip.cs:129:71:129:82 | access to local variable destFilePath : String | semmle.label | access to local variable destFilePath : String | | ZipSlip.cs:134:58:134:69 | access to local variable destFilePath | semmle.label | access to local variable destFilePath | +| ZipSlip.cs:305:24:305:40 | access to local variable destinationOnDisk : String | semmle.label | access to local variable destinationOnDisk : String | +| ZipSlip.cs:305:44:305:95 | call to method GetFullPath : String | semmle.label | call to method GetFullPath : String | +| ZipSlip.cs:305:61:305:94 | call to method Combine : String | semmle.label | call to method Combine : String | +| ZipSlip.cs:305:80:305:93 | access to property FullName : String | semmle.label | access to property FullName : String | +| ZipSlip.cs:307:44:307:60 | access to local variable destinationOnDisk : String | semmle.label | access to local variable destinationOnDisk : String | +| ZipSlip.cs:308:37:308:53 | access to local variable destinationOnDisk | semmle.label | access to local variable destinationOnDisk | | ZipSlipBad.cs:9:16:9:27 | access to local variable destFileName : String | semmle.label | access to local variable destFileName : String | | 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 | From 1bdd3668094cf7b9734e010e4ec2419565e0402f Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Mon, 25 Aug 2025 09:22:52 -0700 Subject: [PATCH 4/4] Remove unused code --- .../lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll | 3 --- 1 file changed, 3 deletions(-) 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 dd8711ce0eb6..cdb5e2e440cf 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll @@ -140,9 +140,6 @@ class RootSanitizerMethodCall extends SanitizerMethodCall { exists(MethodSystemStringStartsWith sm | this.getTarget() = sm) and exists(Expr q, MethodCallGetFullPath mcGetFullPath | this.getQualifier() = q and - // JB1: Try and detect existentials with non-interelated variables - // , AbstractValue v - // v.(AbstractValues::BooleanValue).getValue() = true and safeCombineGetFullPathSequence(mcGetFullPath, q) ) }