From 242fba3fd20dc177d6cb4eb8d73899e633f3d76a Mon Sep 17 00:00:00 2001 From: Denis Levin Date: Mon, 13 Aug 2018 14:56:45 -0700 Subject: [PATCH 1/5] cs: Query for ZipSlip vulnerability (CVE-2018-1002200) Initial check in to validate the tests --- .../src/Security Features/CWE-022/ZipSlip.ql | 91 +++++++++++++ .../CWE-022/{ => TaintedPath}/TaintedPath.cs | 0 .../{ => TaintedPath}/TaintedPath.expected | 0 .../{ => TaintedPath}/TaintedPath.qlref | 0 .../CWE-022/ZipSlip/ZipSlip.cs | 123 ++++++++++++++++++ .../CWE-022/ZipSlip/ZipSlip.qlref | 1 + 6 files changed, 215 insertions(+) create mode 100644 csharp/ql/src/Security Features/CWE-022/ZipSlip.ql rename csharp/ql/test/query-tests/Security Features/CWE-022/{ => TaintedPath}/TaintedPath.cs (100%) rename csharp/ql/test/query-tests/Security Features/CWE-022/{ => TaintedPath}/TaintedPath.expected (100%) rename csharp/ql/test/query-tests/Security Features/CWE-022/{ => TaintedPath}/TaintedPath.qlref (100%) create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.cs create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.qlref 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..21e4d71529d8 --- /dev/null +++ b/csharp/ql/src/Security Features/CWE-022/ZipSlip.ql @@ -0,0 +1,91 @@ +/** + * @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 + * @precision high + * @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 100% 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 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..96cdaba0792f --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.cs @@ -0,0 +1,123 @@ +// semmle-extractor-options: /nostdlib /noconfig /r:${env.windir}\Microsoft.NET\Framework64\v4.0.30319\mscorlib.dll /r:${env.windir}\Microsoft.NET\Framework64\v4.0.30319\System.dll /r:${env.windir}\Microsoft.NET\Framework64\v4.0.30319\System.IO.Compression.dll /r:${env.windir}\Microsoft.NET\Framework64\v4.0.30319\System.IO.Compression.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.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 From cee996c5438110197c3b09e2ce01d1a0132d1372 Mon Sep 17 00:00:00 2001 From: Denis Levin Date: Mon, 13 Aug 2018 15:04:15 -0700 Subject: [PATCH 2/5] Adding .expected file to QLTest --- .../Security Features/CWE-022/ZipSlip/ZipSlip.expected | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.expected 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..cff1cd5eeb64 --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.expected @@ -0,0 +1,6 @@ +| Program.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 | Program.cs:19:31:19:44 | access to property FullName | zip archive | +| Program.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 | Program.cs:16:52:16:65 | access to property FullName | zip archive | +| Program.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 | Program.cs:54:72:54:85 | access to property FullName | zip archive | +| Program.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 | Program.cs:54:72:54:85 | access to property FullName | zip archive | +| Program.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 | Program.cs:54:72:54:85 | access to property FullName | zip archive | +| Program.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 | Program.cs:54:72:54:85 | access to property FullName | zip archive | From 82f0c389c771ab29a1e1a1951c43a6446f662dbb Mon Sep 17 00:00:00 2001 From: calum Date: Tue, 14 Aug 2018 12:52:26 +0100 Subject: [PATCH 3/5] C#: Update test references to use .NET Core, and change relative directory of moved test file. --- .../Security Features/CWE-022/TaintedPath/TaintedPath.cs | 2 +- .../query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs index 68d4f76608b7..4aac88ad402b 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs @@ -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/ZipSlip/ZipSlip.cs b/csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.cs index 96cdaba0792f..2206333d4143 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 @@ -1,4 +1,4 @@ -// semmle-extractor-options: /nostdlib /noconfig /r:${env.windir}\Microsoft.NET\Framework64\v4.0.30319\mscorlib.dll /r:${env.windir}\Microsoft.NET\Framework64\v4.0.30319\System.dll /r:${env.windir}\Microsoft.NET\Framework64\v4.0.30319\System.IO.Compression.dll /r:${env.windir}\Microsoft.NET\Framework64\v4.0.30319\System.IO.Compression.FileSystem.dll +// 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; From fc5963b83123af14c7382198d0d7cbcb47e99035 Mon Sep 17 00:00:00 2001 From: calum Date: Tue, 14 Aug 2018 13:00:25 +0100 Subject: [PATCH 4/5] C#: Rename filename in expected test output. --- .../CWE-022/ZipSlip/ZipSlip.expected | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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 cff1cd5eeb64..6ac906c76875 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,6 +1,6 @@ -| Program.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 | Program.cs:19:31:19:44 | access to property FullName | zip archive | -| Program.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 | Program.cs:16:52:16:65 | access to property FullName | zip archive | -| Program.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 | Program.cs:54:72:54:85 | access to property FullName | zip archive | -| Program.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 | Program.cs:54:72:54:85 | access to property FullName | zip archive | -| Program.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 | Program.cs:54:72:54:85 | access to property FullName | zip archive | -| Program.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 | Program.cs:54:72:54:85 | access to property FullName | zip archive | +| 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 | From a09e7db08d3a96e9146dd28fbde462f90667c34f Mon Sep 17 00:00:00 2001 From: Denis Levin Date: Tue, 14 Aug 2018 18:41:21 -0700 Subject: [PATCH 5/5] Removing @precision high tag --- csharp/ql/src/Security Features/CWE-022/ZipSlip.ql | 1 - 1 file changed, 1 deletion(-) diff --git a/csharp/ql/src/Security Features/CWE-022/ZipSlip.ql b/csharp/ql/src/Security Features/CWE-022/ZipSlip.ql index 21e4d71529d8..ca96fa8b163e 100644 --- a/csharp/ql/src/Security Features/CWE-022/ZipSlip.ql +++ b/csharp/ql/src/Security Features/CWE-022/ZipSlip.ql @@ -5,7 +5,6 @@ * @kind problem * @id cs/zipslip * @problem.severity error - * @precision high * @tags security * external/cwe/cwe-022 */