From b0e4305c95fcd6600a2a1aa605445c5d3ad1050d Mon Sep 17 00:00:00 2001 From: Dilan Bhalla Date: Mon, 24 Apr 2023 14:49:27 -0700 Subject: [PATCH] josh zipslip improvements --- .../csharp/security/dataflow/ZipSlipQuery.qll | 478 ++++++++++++++---- .../Security Features/CWE-022/ZipSlip.qhelp | 2 +- .../src/Security Features/CWE-022/ZipSlip.ql | 6 +- .../CWE-022/ZipSlip/ZipSlip.cs | 119 ++++- .../CWE-022/ZipSlip/ZipSlip.expected | 98 ++-- 5 files changed, 540 insertions(+), 163 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 de5a5bbd837..53009a6c4aa 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll @@ -3,147 +3,431 @@ */ import csharp -private import semmle.code.csharp.controlflow.Guards +import semmle.code.csharp.dataflow.TaintTracking2 +import semmle.code.csharp.dataflow.TaintTracking3 +import semmle.code.csharp.controlflow.Guards +import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph -/** - * A data flow source for unsafe zip extraction. - */ -abstract class Source extends DataFlow::Node { } +private abstract class AbstractSanitizerMethod extends Method { } -/** - * A data flow sink for unsafe zip extraction. - */ -abstract class Sink extends DataFlow::ExprNode { } +class MethodSystemStringStartsWith extends AbstractSanitizerMethod { + MethodSystemStringStartsWith() { this.getQualifiedName() = "System.String.StartsWith" } +} -/** - * A sanitizer for unsafe zip extraction. - */ -abstract class Sanitizer extends DataFlow::ExprNode { } +private abstract class UnsanitizedPathCombiner extends Expr { } -/** - * DEPRECATED: Use `Sanitizer` instead. - * - * A guard for unsafe zip extraction. - */ -abstract deprecated class SanitizerGuard extends DataFlow::BarrierGuard { } - -/** A taint tracking configuration for Zip Slip */ -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 } - - deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { - guard instanceof SanitizerGuard +class PathCombinerViaMethodCall extends UnsanitizedPathCombiner { + PathCombinerViaMethodCall() { + this.(MethodCall).getTarget().hasQualifiedName("System.IO.Path", "Combine") } } -/** 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" +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") } +} + +class GetFullPathToQualifierTaintTrackingConfiguration extends TaintTracking3::Configuration { + GetFullPathToQualifierTaintTrackingConfiguration() { + this = "GetFullPathToQualifierTaintTrackingConfiguration" + } + + override predicate isSource(DataFlow::Node node) { + exists(MethodCallGetFullPath mcGetFullPath | node = DataFlow::exprNode(mcGetFullPath)) + } + + override predicate isSink(DataFlow::Node node) { + exists(MethodCall mc | + mc.getTarget() instanceof MethodSystemStringStartsWith and + node.asExpr() = mc.getQualifier() ) } } -/** An argument to the `ExtractToFile` extension method. */ -class ExtractToFileArgSink extends Sink { - ExtractToFileArgSink() { +/** + * 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. + */ +class PathCombinerToGetFullPathTaintTrackingConfiguration extends TaintTracking3::Configuration { + PathCombinerToGetFullPathTaintTrackingConfiguration() { + this = "PathCombinerToGetFullPathTaintTrackingConfiguration " + } + + /** + * We are looking for the result of some Path combining operation (String concat, Path.Combine, etc.) + */ + override 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. + */ + override 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, PathCombinerToGetFullPathTaintTrackingConfiguration taintTracking | + taintTracking.hasFlow(DataFlow::exprNode(source), DataFlow::exprNode(mcGetFullPath.getArgument(0))) + ) and + exists(GetFullPathToQualifierTaintTrackingConfiguration qualifierTaintTracker | + qualifierTaintTracker.hasFlow(DataFlow::exprNode(mcGetFullPath), DataFlow::exprNode(q)) + ) +} + +/** +* The set of /valid/ Guards of RootSanitizerMethodCall. +* +* IN CONJUNCTION with BOTH +* Path.Combine +* AND Path.GetFullPath +* OR +* There is a direct flow from Path.GetFullPath to qualifier of RootSanitizerMethodCall. +* +* It is not simply enough for the qualifier of String.StartsWith +* to pass through Path.Combine without also passing through GetFullPath AFTER. +*/ +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 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(); +} + +/** + * 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")) + * ... + * ... + * } + */ +class SanitizedGuardTaintTrackingConfiguration extends TaintTracking2::Configuration { + SanitizedGuardTaintTrackingConfiguration() { this = "SanitizedGuardTaintTrackingConfiguration" } + + override predicate isSource(DataFlow::Node source) { source instanceof DataFlow::ParameterNode } + + override 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 +* +* bool wrapperFn(a,b){ +* if(guard(a,b)) +* return true +* .... +* return false +* } +* +* bool wrapperFn(a,b){ +* ... +* return guard(a,b) +* } +*/ +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, SanitizedGuardTaintTrackingConfiguration taintTracker | + g.getEnclosingCallable() = this and + taintTracker + .hasFlow(DataFlow::parameterNode(paramFilename), + DataFlow::exprNode(g.getFilePathArgument())) 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 + 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() { +/** + * 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") - | + ( + 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") ) } } -/** A path argument to a call to the `FileStream` constructor. */ -class FileStreamArgSink extends Sink { - FileStreamArgSink() { +/** + * 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") - | + oc.getTarget().getDeclaringType().hasQualifiedName("System.IO", "FileStream") and this.asExpr() = oc.getArgumentForName("path") ) } } + /** - * A path argument to a call to the `FileStream` constructor. - * - * This constructor can accept a tainted file name and subsequently be used to open a file stream. + * Constructor to FileInfo can take tainted file name and subsequently be used to open file stream */ -class FileInfoArgSink extends Sink { - FileInfoArgSink() { +class SinkFileInfoConstructorArgument extends Sink { + SinkFileInfoConstructorArgument() { exists(ObjectCreation oc | - oc.getTarget().getDeclaringType().hasQualifiedName("System.IO", "FileInfo") - | + oc.getTarget().getDeclaringType().hasQualifiedName("System.IO", "FileInfo") and 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. + * Extracting just file name from a ZipEntry, not the full path */ -class GetFileNameSanitizer extends Sanitizer { - GetFileNameSanitizer() { - exists(MethodCall mc | mc.getTarget().hasQualifiedName("System.IO", "Path", "GetFileName") | - this.asExpr() = mc +class FileNameExtrationSanitizer extends Sanitizer { + FileNameExtrationSanitizer() { + exists(MethodCall mc | + mc.getTarget().hasQualifiedName("System.IO.Path", "GetFileName") and + this = DataFlow::exprNode(mc.getAnArgument()) ) } } /** - * A call to `Substring`. - * - * 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. + * Checks the string for relative path, + * or checks the destination folder for whitelisted/target path, etc */ class StringCheckSanitizer extends Sanitizer { - StringCheckSanitizer() { this = DataFlow::BarrierGuard::getABarrierNode() } + 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 bee5d819836..61f7fc89267 100644 --- a/csharp/ql/src/Security Features/CWE-022/ZipSlip.qhelp +++ b/csharp/ql/src/Security Features/CWE-022/ZipSlip.qhelp @@ -27,7 +27,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 f33c54ce6aa..d9c6d6fc113 100644 --- a/csharp/ql/src/Security Features/CWE-022/ZipSlip.ql +++ b/csharp/ql/src/Security Features/CWE-022/ZipSlip.ql @@ -16,8 +16,8 @@ import csharp import semmle.code.csharp.security.dataflow.ZipSlipQuery import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph -from TaintTrackingConfiguration zipTaintTracking, DataFlow::PathNode source, DataFlow::PathNode sink +from ZipSlipTaintTrackingConfiguration zipTaintTracking, DataFlow::PathNode source, DataFlow::PathNode sink where zipTaintTracking.hasFlowPath(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 1ec93bba3ed..9c40aa224e8 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 @@ namespace ZipSlip 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 a5b2d7166f7..297cfee7c34 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,67 @@ 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:31:41:31:52 | access to local variable destFilePath | +| 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:15:61:15:74 | access to property FullName : String | ZipSlip.cs:30:71:30:87 | access to local variable fullPath_relative : String | +| ZipSlip.cs:15:61:15:74 | access to property FullName : String | ZipSlip.cs:31:41:31:52 | access to local variable destFilePath | +| ZipSlip.cs:15:61:15:74 | access to property FullName : String | ZipSlip.cs:39:45:39:73 | access to local variable destFilePath_notCanonicalized | +| 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:18:53:18:66 | access to property FullName : String | ZipSlip.cs:23:41:23:52 | access to local variable destFileName | +| 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:58:61:58:74 | access to property FullName : String | ZipSlip.cs:62:33:62:40 | access to local variable fullpath | +| ZipSlip.cs:58:61:58:74 | access to property FullName : String | ZipSlip.cs:71:37:71:44 | access to local variable fullpath | +| 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 | +| ZipSlip.cs:105:72:105:85 | access to property FullName : String | ZipSlip.cs:112:74:112:85 | access to local variable destFilePath | +| ZipSlip.cs:105:72:105:85 | access to property FullName : String | ZipSlip.cs:119:71:119:82 | access to local variable destFilePath | +| ZipSlip.cs:105:72:105:85 | access to property FullName : String | ZipSlip.cs:126:57:126:68 | access to local variable destFilePath | +| ZipSlip.cs:105:72:105:85 | access to property FullName : String | ZipSlip.cs:134:58:134:69 | access to local variable destFilePath | | 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 | +| ZipSlipBad.cs:9:59:9:72 | access to property FullName : String | ZipSlipBad.cs:10:29:10:40 | access to local variable destFileName | 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 |