Skip to content

cs: Query for ZipSlip vulnerability (CVE-2018-1002200) #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 16, 2018
Merged

cs: Query for ZipSlip vulnerability (CVE-2018-1002200) #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
242fba3
cs: Query for ZipSlip vulnerability (CVE-2018-1002200)
Aug 13, 2018
cee996c
Adding .expected file to QLTest
Aug 13, 2018
82f0c38
C#: Update test references to use .NET Core, and change relative dire…
calumgrant Aug 14, 2018
fc5963b
C#: Rename filename in expected test output.
calumgrant Aug 14, 2018
cdc065c
Merge pull request #1 from calumgrant/cs/ZipSlip
denislevin Aug 15, 2018
a09e7db
Removing @precision high tag
Aug 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 90 additions & 0 deletions csharp/ql/src/Security Features/CWE-022/ZipSlip.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
2 changes: 1 addition & 1 deletion .../Security Features/CWE-022/TaintedPath.cs → ...atures/CWE-022/TaintedPath/TaintedPath.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down
0 ...ity Features/CWE-022/TaintedPath.expected → .../CWE-022/TaintedPath/TaintedPath.expected
View file
Open in desktop
File renamed without changes.
0 ...curity Features/CWE-022/TaintedPath.qlref → ...res/CWE-022/TaintedPath/TaintedPath.qlref
View file
Open in desktop
File renamed without changes.
123 changes: 123 additions & 0 deletions csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
}
}
6 changes: 6 additions & 0 deletions csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 |
1 change: 1 addition & 0 deletions csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.qlref
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Security Features/CWE-022/ZipSlip.ql