Merge pull request #54 from denislevin/denisl/cs/ZipSlip

Approved by calumgrant

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"

csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath.cs → 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;

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);
+                }
+            }
+        }
+    }
+}

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 |

csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.qlref

@@ -0,0 +1 @@
+Security Features/CWE-022/ZipSlip.ql