Review custom archive extraction (Zip Slip)

Rule ND3135 Security

Matches methods that read the name of an archive entry (ZipArchiveEntry.FullName, TarEntry.Name, SharpZipLib ZipEntry.Name, SharpCompress IEntry.Key), build a path with it (Path.Combine(), concatenation, interpolation) and write to the file system, without both resolving the path with Path.GetFullPath() and checking it with String.StartsWith() against the destination root, and without reducing the name with Path.GetFileName().

Archive entry names are data supplied by whoever built the archive. A name such as ..\..\..\Windows\Startup\evil.cmd combined with the destination directory escapes it and overwrites an arbitrary file: the Zip Slip vulnerability, CWE-22 Improper Limitation of a Pathname to a Restricted Directory (OWASP Top 10 A01 Broken Access Control). Path.Combine() makes it worse by discarding the destination when the entry name is an absolute path.

Path.GetFullPath() alone does not suppress the issue: it resolves the .. segments but verifies nothing.

Flagged Code
static void Extract(ZipArchive archive, string destination) {
   foreach (var entry in archive.Entries) {
      var path = Path.Combine(destination, entry.FullName);   // flagged: no containment
      entry.ExtractToFile(path, overwrite: true);
   }
}
Fixed Code
static void Extract(ZipArchive archive, string destination) {
   var root = Path.GetFullPath(destination + Path.DirectorySeparatorChar);
   foreach (var entry in archive.Entries) {
      var path = Path.GetFullPath(Path.Combine(root, entry.FullName));
      if (!path.StartsWith(root, StringComparison.Ordinal))
         throw new IOException("Entry outside of the destination: " + entry.FullName);
      entry.ExtractToFile(path, overwrite: true);
   }
}

How to fix

Prefer the framework extraction that already verifies containment: ZipFile.ExtractToDirectory() and ZipArchive.ExtractToDirectory() reject entries that escape the destination. When entries must be processed one by one, resolve the combined path with Path.GetFullPath() and verify that it starts with the destination root (terminated by a directory separator) before writing, or reduce the entry name with Path.GetFileName() when the archive layout is flat.

Suppress the issue when the containment check is done in a helper called by the method.

CQLinq source

This rule is written in CQLinq, C# LINQ over the NDepend code model. It ships with every new NDepend project: you can read it, adapt it, or use it as a starting point for your own custom rules.

// <Name>Review custom archive extraction (Zip Slip)</Name>
// <Id>ND3135:ReviewCustomArchiveExtraction</Id>
warnif count > 0

// Third-party members are part of the code model only when the application uses them.
// The entry name comes from the archive, hence from whoever produced the archive.
let entryNameGetters = ThirdParty.Methods.Where(m =>
   m.IsPropertyGetter && (
   (m.ParentType.FullName == "System.IO.Compression.ZipArchiveEntry" && m.SimpleName == "get_FullName") ||
   (m.ParentType.FullName == "System.Formats.Tar.TarEntry" && m.SimpleName == "get_Name") ||
   (m.ParentType.FullNameEqualsAny("ICSharpCode.SharpZipLib.Zip.ZipEntry", "ICSharpCode.SharpZipLib.Tar.TarEntry") && m.SimpleName == "get_Name") ||
   (m.ParentType.FullName.StartsWith("SharpCompress.") && m.SimpleName == "get_Key")))
   .ToHashSetEx()
where entryNameGetters.Any()

// The destination path is assembled with the entry name.
let pathBuildingMembers = ThirdParty.Methods.Where(m =>
   (m.ParentType.FullName == "System.IO.Path" && m.SimpleNameEqualsAny("Combine", "Join")) ||
   (m.ParentType.FullName == "System.String" && m.SimpleNameEqualsAny("Concat", "Format")) ||
    m.ParentType.FullName == "System.Runtime.CompilerServices.DefaultInterpolatedStringHandler").ToHashSetEx()

// Something is written at that path.
let fileWriteMembers = ThirdParty.Methods.Where(m =>
   (m.ParentType.FullName == "System.IO.File" && m.SimpleName.StartsWithAny("Create", "Write", "Open", "Copy", "Move", "AppendAll")) ||
   (m.ParentType.FullNameEqualsAny("System.IO.FileStream", "System.IO.StreamWriter") && m.IsConstructor) ||
   (m.ParentType.FullName == "System.IO.Directory" && m.SimpleName == "CreateDirectory") ||
    m.SimpleName.StartsWithAny("ExtractToFile", "WriteToFile", "WriteToDirectory"))
   .ToHashSetEx()

// Containment verification: the combined path is resolved AND compared with the destination root.
// Path.GetFullPath() alone resolves the '..' segments but verifies nothing.
let getFullPath = ThirdParty.Methods.Where(m => m.ParentType.FullName == "System.IO.Path" && m.SimpleName == "GetFullPath").ToHashSetEx()
let startsWith = ThirdParty.Methods.Where(m => m.ParentType.FullName == "System.String" && m.SimpleName == "StartsWith").ToHashSetEx()
// Reducing the entry name to a file name discards any directory segment.
let getFileName = ThirdParty.Methods.Where(m => m.ParentType.FullName == "System.IO.Path" && m.SimpleName == "GetFileName").ToHashSetEx()

from m in Application.Methods.UsingAny(entryNameGetters)
where m.MethodsCalled.Intersect(pathBuildingMembers).Any() &&
      m.MethodsCalled.Intersect(fileWriteMembers).Any() &&
     !(m.MethodsCalled.Intersect(getFullPath).Any() && m.MethodsCalled.Intersect(startsWith).Any()) &&
     !m.MethodsCalled.Intersect(getFileName).Any()

select new {
  m,
  fileWriteMembersUsed = m.MethodsCalled.Intersect(fileWriteMembers).ToArray(),
  Debt = 20.ToMinutes().ToDebt(),
  Severity = Severity.High
}

//<Expl>
// The method *{0}* builds a destination path from an archive entry name
// and writes through *{1}*, without verifying that the resolved path
// stays under the destination directory. An entry named
// *..\..\startup.cmd* is written outside that directory.
//</Expl>

// <Desc>
// Matches methods that read the name of an archive entry
// (*ZipArchiveEntry.FullName*, *TarEntry.Name*, SharpZipLib *ZipEntry.Name*,
// SharpCompress *IEntry.Key*), build a path with it (*Path.Combine()*,
// concatenation, interpolation) and write to the file system, without
// both resolving the path with *Path.GetFullPath()* and checking it with
// *String.StartsWith()* against the destination root, and without
// reducing the name with *Path.GetFileName()*.
//
// Archive entry names are data supplied by whoever built the archive. A
// name such as *..\..\..\Windows\Startup\evil.cmd* combined with the
// destination directory escapes it and overwrites an arbitrary file:
// the **Zip Slip** vulnerability, CWE-22 *Improper Limitation of a
// Pathname to a Restricted Directory* (OWASP Top 10 A01 Broken Access
// Control). *Path.Combine()* makes it worse by discarding the destination
// when the entry name is an absolute path.
//
// *Path.GetFullPath()* alone does not suppress the issue: it resolves
// the *..* segments but verifies nothing.
//
// **Flagged Code**
// ```csharp
// static void Extract(ZipArchive archive, string destination) {
//    foreach (var entry in archive.Entries) {
//       var path = Path.Combine(destination, entry.FullName);   // flagged: no containment
//       entry.ExtractToFile(path, overwrite: true);
//    }
// }
// ```
//
// **Fixed Code**
// ```csharp
// static void Extract(ZipArchive archive, string destination) {
//    var root = Path.GetFullPath(destination + Path.DirectorySeparatorChar);
//    foreach (var entry in archive.Entries) {
//       var path = Path.GetFullPath(Path.Combine(root, entry.FullName));
//       if (!path.StartsWith(root, StringComparison.Ordinal))
//          throw new IOException("Entry outside of the destination: " + entry.FullName);
//       entry.ExtractToFile(path, overwrite: true);
//    }
// }
// ```
// </Desc>

// <HowToFix>
// Prefer the framework extraction that already verifies containment:
// *ZipFile.ExtractToDirectory()* and *ZipArchive.ExtractToDirectory()*
// reject entries that escape the destination. When entries must be
// processed one by one, resolve the combined path with
// *Path.GetFullPath()* and verify that it starts with the destination
// root (terminated by a directory separator) before writing, or reduce
// the entry name with *Path.GetFileName()* when the archive layout is
// flat.
//
// Suppress the issue when the containment check is done in a helper
// called by the method.
// </HowToFix>

Open in the interactive Rules Explorer · All default rules

Security: all 39 items