diff --git a/csharp/ql/src/Security Features/CWE-022/ZipSlip.ql b/csharp/ql/src/Security Features/CWE-022/ZipSlip.ql new file mode 100644 index 000000000000..ca96fa8b163e --- /dev/null +++ b/csharp/ql/src/Security Features/CWE-022/ZipSlip.ql @@ -0,0 +1,90 @@ +/** + * @name Potential ZipSlip vulnerability + * @description When extracting files from an archive, don't add archive item's path to the target file system path. Archive path can be relative and can lead to + * file system access outside of the expected file system target path, leading to malicious config changes and remote code execution via lay-and-wait technique + * @kind problem + * @id cs/zipslip + * @problem.severity error + * @tags security + * external/cwe/cwe-022 + */ + +import csharp + +// access to full name of the archive item +Expr archiveFullName(PropertyAccess pa) { + pa.getTarget().getDeclaringType().hasQualifiedName("System.IO.Compression.ZipArchiveEntry") + and pa.getTarget().getName() = "FullName" + and result = pa +} + +// argument to extract to file extension method +Expr compressionExtractToFileArgument(MethodCall mc) { + mc.getTarget().hasQualifiedName("System.IO.Compression.ZipFileExtensions", "ExtractToFile") + and result = mc.getArgumentForName("destinationFileName") +} + +// File Stream created from tainted file name through File.Open/File.Create +Expr fileOpenArgument(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 result = mc.getArgumentForName("path") +} + +// File Stream created from tainted file name passed directly to the constructor +Expr streamConstructorArgument(ObjectCreation oc) { + oc.getTarget().getDeclaringType().hasQualifiedName("System.IO.FileStream") + and result = oc.getArgumentForName("path") +} + +// constructor to FileInfo can take tainted file name and subsequently be used to open file stream +Expr fileInfoConstructorArgument(ObjectCreation oc) { + oc.getTarget().getDeclaringType().hasQualifiedName("System.IO.FileInfo") + and result = oc.getArgumentForName("fileName") +} +// extracting just file name, not the full path +Expr fileNameExtraction(MethodCall mc) { + mc.getTarget().hasQualifiedName("System.IO.Path", "GetFileName") + and result = mc.getAnArgument() +} + +// Checks the string for relative path, or checks the destination folder for whitelisted/target path, etc. +Expr stringCheck(MethodCall mc) { + (mc.getTarget().hasQualifiedName("System.String", "StartsWith") or + mc.getTarget().hasQualifiedName("System.String", "Substring")) + and result = mc.getQualifier() +} + +// Taint tracking configuration for ZipSlip +class ZipSlipTaintTrackingConfiguration extends TaintTracking::Configuration { + ZipSlipTaintTrackingConfiguration() { + this = "ZipSlipTaintTracking" + } + + override predicate isSource(DataFlow::Node source) { + exists(PropertyAccess pa | + source.asExpr() = archiveFullName(pa)) + } + + override predicate isSink(DataFlow::Node sink) { + exists(MethodCall mc | + sink.asExpr() = compressionExtractToFileArgument(mc) or + sink.asExpr() = fileOpenArgument(mc)) + or + exists(ObjectCreation oc | + sink.asExpr() = streamConstructorArgument(oc) or + sink.asExpr() = fileInfoConstructorArgument(oc)) + } + + override predicate isSanitizer(DataFlow::Node node) { + exists(MethodCall mc | + node.asExpr() = fileNameExtraction(mc) or + node.asExpr() = stringCheck(mc)) + } + } + + +from ZipSlipTaintTrackingConfiguration zipTaintTracking, DataFlow::Node source, DataFlow::Node sink +where zipTaintTracking.hasFlow(source, sink) +select sink, "Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted", source, "zip archive" diff --git a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath.cs b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs similarity index 97% rename from csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath.cs rename to csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs index 68d4f76608b7..4aac88ad402b 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs @@ -1,4 +1,4 @@ -// semmle-extractor-options: /r:System.IO.FileSystem.dll /r:System.Runtime.Extensions.dll /r:System.Collections.Specialized.dll ${testdir}/../../../resources/stubs/System.Web.cs +// semmle-extractor-options: /r:System.IO.FileSystem.dll /r:System.Runtime.Extensions.dll /r:System.Collections.Specialized.dll ${testdir}/../../../../resources/stubs/System.Web.cs using System; using System.IO; diff --git a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath.expected b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected similarity index 100% rename from csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath.expected rename to csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected diff --git a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath.qlref b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.qlref similarity index 100% rename from csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath.qlref rename to csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.qlref 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 new file mode 100644 index 000000000000..2206333d4143 --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.cs @@ -0,0 +1,123 @@ +// semmle-extractor-options: /r:System.IO.Compression.dll /r:System.IO.Compression.FileSystem.dll /r:System.IO.Compression.ZipFile.dll /r:System.IO.FileSystem.dll +using System; +using System.IO; +using System.IO.Compression; + +namespace ZipSlip +{ + class Program + { + + 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)) + { + // BAD + string destFileName = Path.Combine(destDirectory, file); + entry.ExtractToFile(destFileName, true); + + // GOOD + string sanitizedFileName = Path.Combine(destDirectory, fileName); + entry.ExtractToFile(sanitizedFileName, true); + + // BAD + string destFilePath = Path.Combine(destDirectory, fullPath); + entry.ExtractToFile(destFilePath, true); + + // GOOD: some check on destination. + if (destFilePath.StartsWith(destDirectory)) + entry.ExtractToFile(destFilePath, true); + } + } + } + + private static int UnzipToStream(Stream zipStream, string installDir) + { + int returnCode = 0; + try + { + // normalize InstallDir for use in check below + var InstallDir = Path.GetFullPath(installDir + Path.DirectorySeparatorChar); + + using (ZipArchive archive = new ZipArchive(zipStream, ZipArchiveMode.Read)) + { + foreach (ZipArchiveEntry entry in archive.Entries) + { + // figure out where we are putting the file + string destFilePath = Path.Combine(InstallDir, entry.FullName); + + Directory.CreateDirectory(Path.GetDirectoryName(destFilePath)); + + using (Stream archiveFileStream = entry.Open()) + { + // BAD: writing to file stream + using (Stream tfsFileStream = new FileStream(destFilePath, FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None)) + { + Console.WriteLine(@"Writing ""{0}""", destFilePath); + archiveFileStream.CopyTo(tfsFileStream); + } + + // BAD: can do it this way too + using (Stream tfsFileStream = File.Create(destFilePath)) + { + Console.WriteLine(@"Writing ""{0}""", destFilePath); + archiveFileStream.CopyTo(tfsFileStream); + } + + // BAD: creating stream using fileInfo + var fileInfo = new FileInfo(destFilePath); + using (FileStream fs = fileInfo.OpenWrite()) + { + Console.WriteLine(@"Writing ""{0}""", destFilePath); + archiveFileStream.CopyTo(fs); + } + + // BAD: creating stream using fileInfo + var fileInfo1 = new FileInfo(destFilePath); + using (FileStream fs = fileInfo1.Open(FileMode.Create)) + { + Console.WriteLine(@"Writing ""{0}""", destFilePath); + archiveFileStream.CopyTo(fs); + } + } + } + } + } + catch (Exception ex) + { + Console.WriteLine("Error patching files: {0}", ex.ToString()); + returnCode = -1; + } + + return returnCode; + } + + static void Main(string[] args) + { + string zipFileName; + zipFileName = args[0]; + + string targetPath = args.Length == 2 ? args[1] : "."; + + using (FileStream file = new FileStream(zipFileName, FileMode.Open)) + { + using (ZipArchive archive = new ZipArchive(file, ZipArchiveMode.Read)) + { + UnzipFileByFile(archive, targetPath); + + // GOOD: the path is checked in this extension method + archive.ExtractToDirectory(targetPath); + + UnzipToStream(file, targetPath); + } + } + } + } +} 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 new file mode 100644 index 000000000000..6ac906c76875 --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.expected @@ -0,0 +1,6 @@ +| ZipSlip.cs:24:41:24:52 | access to local variable destFileName | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:19:31:19:44 | access to property FullName | zip archive | +| ZipSlip.cs:32:41:32:52 | access to local variable destFilePath | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:16:52:16:65 | access to property FullName | zip archive | +| ZipSlip.cs:61:74:61:85 | access to local variable destFilePath | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:54:72:54:85 | access to property FullName | zip archive | +| ZipSlip.cs:68:71:68:82 | access to local variable destFilePath | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:54:72:54:85 | access to property FullName | zip archive | +| ZipSlip.cs:75:57:75:68 | access to local variable destFilePath | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:54:72:54:85 | access to property FullName | zip archive | +| ZipSlip.cs:83:58:83:69 | access to local variable destFilePath | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:54:72:54:85 | access to property FullName | zip archive | diff --git a/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.qlref b/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.qlref new file mode 100644 index 000000000000..08e51efc3512 --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.qlref @@ -0,0 +1 @@ +Security Features/CWE-022/ZipSlip.ql \ No newline at end of file