Skip to content

Commit 83c539a

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #54 from denislevin/denisl/cs/ZipSlip
Approved by calumgrant
2 parents 692f416 + a09e7db commit 83c539a

File tree

7 files changed

+221
-1
lines changed

7 files changed

+221
-1
lines changed

‎csharp/ql/src/Security Features/CWE-022/ZipSlip.ql

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
/**
2+
* @name Potential ZipSlip vulnerability
3+
* @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
4+
* 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
5+
* @kind problem
6+
* @id cs/zipslip
7+
* @problem.severity error
8+
* @tags security
9+
* external/cwe/cwe-022
10+
*/
11+
12+
import csharp
13+
14+
// access to full name of the archive item
15+
Expr archiveFullName(PropertyAccess pa) {
16+
pa.getTarget().getDeclaringType().hasQualifiedName("System.IO.Compression.ZipArchiveEntry")
17+
and pa.getTarget().getName() = "FullName"
18+
and result = pa
19+
}
20+
21+
// argument to extract to file extension method
22+
Expr compressionExtractToFileArgument(MethodCall mc) {
23+
mc.getTarget().hasQualifiedName("System.IO.Compression.ZipFileExtensions", "ExtractToFile")
24+
and result = mc.getArgumentForName("destinationFileName")
25+
}
26+
27+
// File Stream created from tainted file name through File.Open/File.Create
28+
Expr fileOpenArgument(MethodCall mc) {
29+
(mc.getTarget().hasQualifiedName("System.IO.File", "Open") or
30+
mc.getTarget().hasQualifiedName("System.IO.File", "OpenWrite") or
31+
mc.getTarget().hasQualifiedName("System.IO.File", "Create"))
32+
and result = mc.getArgumentForName("path")
33+
}
34+
35+
// File Stream created from tainted file name passed directly to the constructor
36+
Expr streamConstructorArgument(ObjectCreation oc) {
37+
oc.getTarget().getDeclaringType().hasQualifiedName("System.IO.FileStream")
38+
and result = oc.getArgumentForName("path")
39+
}
40+
41+
// constructor to FileInfo can take tainted file name and subsequently be used to open file stream
42+
Expr fileInfoConstructorArgument(ObjectCreation oc) {
43+
oc.getTarget().getDeclaringType().hasQualifiedName("System.IO.FileInfo")
44+
and result = oc.getArgumentForName("fileName")
45+
}
46+
// extracting just file name, not the full path
47+
Expr fileNameExtraction(MethodCall mc) {
48+
mc.getTarget().hasQualifiedName("System.IO.Path", "GetFileName")
49+
and result = mc.getAnArgument()
50+
}
51+
52+
// Checks the string for relative path, or checks the destination folder for whitelisted/target path, etc.
53+
Expr stringCheck(MethodCall mc) {
54+
(mc.getTarget().hasQualifiedName("System.String", "StartsWith") or
55+
mc.getTarget().hasQualifiedName("System.String", "Substring"))
56+
and result = mc.getQualifier()
57+
}
58+
59+
// Taint tracking configuration for ZipSlip
60+
class ZipSlipTaintTrackingConfiguration extends TaintTracking::Configuration {
61+
ZipSlipTaintTrackingConfiguration() {
62+
this = "ZipSlipTaintTracking"
63+
}
64+
65+
override predicate isSource(DataFlow::Node source) {
66+
exists(PropertyAccess pa |
67+
source.asExpr() = archiveFullName(pa))
68+
}
69+
70+
override predicate isSink(DataFlow::Node sink) {
71+
exists(MethodCall mc |
72+
sink.asExpr() = compressionExtractToFileArgument(mc) or
73+
sink.asExpr() = fileOpenArgument(mc))
74+
or
75+
exists(ObjectCreation oc |
76+
sink.asExpr() = streamConstructorArgument(oc) or
77+
sink.asExpr() = fileInfoConstructorArgument(oc))
78+
}
79+
80+
override predicate isSanitizer(DataFlow::Node node) {
81+
exists(MethodCall mc |
82+
node.asExpr() = fileNameExtraction(mc) or
83+
node.asExpr() = stringCheck(mc))
84+
}
85+
}
86+
87+
88+
from ZipSlipTaintTrackingConfiguration zipTaintTracking, DataFlow::Node source, DataFlow::Node sink
89+
where zipTaintTracking.hasFlow(source, sink)
90+
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 renamed to ‎csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// semmle-extractor-options: /r:System.IO.FileSystem.dll /r:System.Runtime.Extensions.dll /r:System.Collections.Specialized.dll ${testdir}/../../../resources/stubs/System.Web.cs
1+
// semmle-extractor-options: /r:System.IO.FileSystem.dll /r:System.Runtime.Extensions.dll /r:System.Collections.Specialized.dll ${testdir}/../../../../resources/stubs/System.Web.cs
22

33
using System;
44
using System.IO;

‎csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath.expected renamed to ‎csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.expected

File renamed without changes.

‎csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath.qlref renamed to ‎csharp/ql/test/query-tests/Security Features/CWE-022/TaintedPath/TaintedPath.qlref

File renamed without changes.

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

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
// 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
2+
using System;
3+
using System.IO;
4+
using System.IO.Compression;
5+
6+
namespace ZipSlip
7+
{
8+
class Program
9+
{
10+
11+
public static void UnzipFileByFile(ZipArchive archive,
12+
string destDirectory)
13+
{
14+
foreach (var entry in archive.Entries)
15+
{
16+
string fullPath = Path.GetFullPath(entry.FullName);
17+
string fileName = Path.GetFileName(entry.FullName);
18+
string filename = entry.Name;
19+
string file = entry.FullName;
20+
if (!string.IsNullOrEmpty(file))
21+
{
22+
// BAD
23+
string destFileName = Path.Combine(destDirectory, file);
24+
entry.ExtractToFile(destFileName, true);
25+
26+
// GOOD
27+
string sanitizedFileName = Path.Combine(destDirectory, fileName);
28+
entry.ExtractToFile(sanitizedFileName, true);
29+
30+
// BAD
31+
string destFilePath = Path.Combine(destDirectory, fullPath);
32+
entry.ExtractToFile(destFilePath, true);
33+
34+
// GOOD: some check on destination.
35+
if (destFilePath.StartsWith(destDirectory))
36+
entry.ExtractToFile(destFilePath, true);
37+
}
38+
}
39+
}
40+
41+
private static int UnzipToStream(Stream zipStream, string installDir)
42+
{
43+
int returnCode = 0;
44+
try
45+
{
46+
// normalize InstallDir for use in check below
47+
var InstallDir = Path.GetFullPath(installDir + Path.DirectorySeparatorChar);
48+
49+
using (ZipArchive archive = new ZipArchive(zipStream, ZipArchiveMode.Read))
50+
{
51+
foreach (ZipArchiveEntry entry in archive.Entries)
52+
{
53+
// figure out where we are putting the file
54+
string destFilePath = Path.Combine(InstallDir, entry.FullName);
55+
56+
Directory.CreateDirectory(Path.GetDirectoryName(destFilePath));
57+
58+
using (Stream archiveFileStream = entry.Open())
59+
{
60+
// BAD: writing to file stream
61+
using (Stream tfsFileStream = new FileStream(destFilePath, FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None))
62+
{
63+
Console.WriteLine(@"Writing ""{0}""", destFilePath);
64+
archiveFileStream.CopyTo(tfsFileStream);
65+
}
66+
67+
// BAD: can do it this way too
68+
using (Stream tfsFileStream = File.Create(destFilePath))
69+
{
70+
Console.WriteLine(@"Writing ""{0}""", destFilePath);
71+
archiveFileStream.CopyTo(tfsFileStream);
72+
}
73+
74+
// BAD: creating stream using fileInfo
75+
var fileInfo = new FileInfo(destFilePath);
76+
using (FileStream fs = fileInfo.OpenWrite())
77+
{
78+
Console.WriteLine(@"Writing ""{0}""", destFilePath);
79+
archiveFileStream.CopyTo(fs);
80+
}
81+
82+
// BAD: creating stream using fileInfo
83+
var fileInfo1 = new FileInfo(destFilePath);
84+
using (FileStream fs = fileInfo1.Open(FileMode.Create))
85+
{
86+
Console.WriteLine(@"Writing ""{0}""", destFilePath);
87+
archiveFileStream.CopyTo(fs);
88+
}
89+
}
90+
}
91+
}
92+
}
93+
catch (Exception ex)
94+
{
95+
Console.WriteLine("Error patching files: {0}", ex.ToString());
96+
returnCode = -1;
97+
}
98+
99+
return returnCode;
100+
}
101+
102+
static void Main(string[] args)
103+
{
104+
string zipFileName;
105+
zipFileName = args[0];
106+
107+
string targetPath = args.Length == 2 ? args[1] : ".";
108+
109+
using (FileStream file = new FileStream(zipFileName, FileMode.Open))
110+
{
111+
using (ZipArchive archive = new ZipArchive(file, ZipArchiveMode.Read))
112+
{
113+
UnzipFileByFile(archive, targetPath);
114+
115+
// GOOD: the path is checked in this extension method
116+
archive.ExtractToDirectory(targetPath);
117+
118+
UnzipToStream(file, targetPath);
119+
}
120+
}
121+
}
122+
}
123+
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
| 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 |
2+
| 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 |
3+
| 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 |
4+
| 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 |
5+
| 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 |
6+
| 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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security Features/CWE-022/ZipSlip.ql

0 commit comments

Comments
 (0)