diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll index dc2fb36c47ae..1b0d90f92a06 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll @@ -229,14 +229,14 @@ private class AspNetCoreRemoteFlowSourceMember extends TaintTracking::TaintedMem } /** A data flow source of remote user input (ASP.NET query collection). */ -class AspNetCoreQueryRemoteFlowSource extends AspNetCoreRemoteFlowSource, DataFlow::ExprNode { +class AspNetCoreQueryRemoteFlowSource extends AspNetCoreRemoteFlowSource, DataFlow::Node { AspNetCoreQueryRemoteFlowSource() { exists(ValueOrRefType t | t instanceof MicrosoftAspNetCoreHttpHttpRequest or t instanceof MicrosoftAspNetCoreHttpQueryCollection or t instanceof MicrosoftAspNetCoreHttpQueryString | - this.getExpr().(Call).getTarget().getDeclaringType() = t or + this.asExpr().(Call).getTarget().getDeclaringType() = t or this.asExpr().(Access).getTarget().getDeclaringType() = t ) or @@ -245,7 +245,7 @@ class AspNetCoreQueryRemoteFlowSource extends AspNetCoreRemoteFlowSource, DataFl .getDeclaringType() .hasFullyQualifiedName("Microsoft.AspNetCore.Http", "IQueryCollection") and c.getTarget().getName() = "TryGetValue" and - this.asExpr() = c.getArgumentForName("value") + this.asDefinition().getTargetAccess() = c.getArgument(1) ) } diff --git a/csharp/ql/src/experimental/CWE-502-DecompressionBombs/DecompressionBombs.qhelp b/csharp/ql/src/experimental/CWE-502-DecompressionBombs/DecompressionBombs.qhelp new file mode 100644 index 000000000000..c3c874cddf72 --- /dev/null +++ b/csharp/ql/src/experimental/CWE-502-DecompressionBombs/DecompressionBombs.qhelp @@ -0,0 +1,26 @@ + + + +

Extracting Compressed files with any compression algorithm like gzip can cause to denial of service attacks.

+

Attackers can compress a huge file which created by repeated similiar byte and convert it to a small compressed file.

+ +
+ + +

When you want to decompress a user-provided compressed file you must be careful about the decompression ratio or read these files within a loop byte by byte to be able to manage the decompressed size in each cycle of the loop.

+ +
+ + +

A good Blog Post about decompression bombs and recommended method is already written by Gérald Barré in this blog post

+ + + +
  • +A great research to gain more impact by this kind of attack +
  • + +
    +
    diff --git a/csharp/ql/src/experimental/CWE-502-DecompressionBombs/DecompressionBombs.ql b/csharp/ql/src/experimental/CWE-502-DecompressionBombs/DecompressionBombs.ql new file mode 100644 index 000000000000..63bc33ba5052 --- /dev/null +++ b/csharp/ql/src/experimental/CWE-502-DecompressionBombs/DecompressionBombs.ql @@ -0,0 +1,38 @@ +/** + * @name Uncontrolled file decompression + * @description Uncontrolled decompression without checking the compression rate is dangerous + * @kind path-problem + * @problem.severity error + * @security-severity 7.8 + * @precision medium + * @id cs/user-controlled-file-decompression + * @tags security + * experimental + * external/cwe/cwe-409 + */ + +import csharp +import semmle.code.csharp.security.dataflow.flowsources.Remote +import experimental.dataflow.security.DecompressionBomb +import RemoteFlowSource + +/** + * A taint tracking configuration for Decompression Bomb. + */ +module DecompressionBombModule implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + predicate isSink(DataFlow::Node sink) { sink instanceof DecompressionBomb::Sink } + + predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + any(DecompressionBomb::AdditionalStep a).isAdditionalFlowStep(node1, node2) + } +} + +module DecompressionBombConfig = TaintTracking::Global; + +import DecompressionBombConfig::PathGraph + +from DecompressionBombConfig::PathNode source, DecompressionBombConfig::PathNode sink +where DecompressionBombConfig::flowPath(source, sink) +select sink.getNode(), source, sink, "This uncontrolled depends on a $@.", source.getNode(), "this" diff --git a/csharp/ql/src/experimental/CWE-502-DecompressionBombs/RemoteFlowSource.qll b/csharp/ql/src/experimental/CWE-502-DecompressionBombs/RemoteFlowSource.qll new file mode 100644 index 000000000000..470ac4ee9ae5 --- /dev/null +++ b/csharp/ql/src/experimental/CWE-502-DecompressionBombs/RemoteFlowSource.qll @@ -0,0 +1,62 @@ +import csharp +import semmle.code.csharp.security.dataflow.flowsources.Remote + +/** A data flow source of remote user input by Form File (ASP.NET core request data). */ +class FormFile extends AspNetRemoteFlowSource { + FormFile() { + // openReadStream is already implemented but I'm putting this here because of having a uniform class for FormFile + exists(MethodCall mc | + mc.getTarget().hasFullyQualifiedName("Microsoft.AspNetCore.Http", "IFormFile", "OpenReadStream") and + this.asExpr() = mc + ) + or + exists(MethodCall mc | + mc.getTarget() + .hasFullyQualifiedName("Microsoft.AspNetCore.Http", "IFormFile", ["CopyTo", "CopyToAsync"]) and + this.asParameter() = mc.getTarget().getParameter(0) + ) + or + exists(Property fa | + fa.getDeclaringType().hasFullyQualifiedName("Microsoft.AspNetCore.Http", "IFormFile") and + fa.hasName(["ContentType", "ContentDisposition", "Name", "FileName"]) and + this.asExpr() = fa.getAnAccess() + ) + } + + override string getSourceType() { result = "ASP.NET core request data from multipart request" } +} + +/** A data flow source of remote user input by Form (ASP.NET core request data). */ +class FormCollection extends AspNetRemoteFlowSource { + FormCollection() { + exists(Property fa | + fa.getDeclaringType().hasFullyQualifiedName("Microsoft.AspNetCore.Http", "IFormCollection") and + fa.hasName("Keys") and + this.asExpr() = fa.getAnAccess() + ) + or + exists(Call c | + c.getTarget() + .getDeclaringType() + .hasFullyQualifiedName("Microsoft.AspNetCore.Http", "IFormCollection") and + c.getTarget().getName() = "TryGetValue" and + this.asDefinition().getTargetAccess() = c.getArgument(1) + ) + } + + override string getSourceType() { + result = "ASP.NET core request data from multipart request Form Keys" + } +} + +/** A data flow source of remote user input by Headers (ASP.NET core request data). */ +class HeaderDictionary extends AspNetRemoteFlowSource { + HeaderDictionary() { + exists(Property fa | + fa.getDeclaringType().hasFullyQualifiedName("Microsoft.AspNetCore.Http", "IHeaderDictionary") and + this.asExpr() = fa.getAnAccess() + ) + } + + override string getSourceType() { result = "ASP.NET core request data from Headers of request" } +} diff --git a/csharp/ql/src/experimental/dataflow/security/DecompressionBomb.qll b/csharp/ql/src/experimental/dataflow/security/DecompressionBomb.qll new file mode 100644 index 000000000000..803c0c9154ec --- /dev/null +++ b/csharp/ql/src/experimental/dataflow/security/DecompressionBomb.qll @@ -0,0 +1,277 @@ +import csharp + +module DecompressionBomb { + /** + * A data flow sink for unsafe zip extraction. + */ + abstract class Sink extends DataFlow::Node { } + + /** + * A data flow Additional step for unsafe zip extraction. + */ + class AdditionalStep extends string { + AdditionalStep() { this = "DecompressionBomb AdditionalStep" } + + abstract predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2); + } +} + +module SystemIoCompression { + /** A `Open` method of ZipFile. */ + class ZipArchiveOpen extends MethodCall { + ZipArchiveOpen() { + exists(MethodCall c | + c.getTarget().hasFullyQualifiedName("System.IO.Compression", "ZipFile", "Open") and + this = c + ) + } + } + + /** A Call to `Open` method considered as Decompression Bomb sink. */ + class ZipArchiveEntryOpen extends DecompressionBomb::Sink { + ZipArchiveEntryOpen() { + exists(MethodCall c | + c.getTarget().hasFullyQualifiedName("System.IO.Compression", "ZipArchiveEntry", "Open") and + this.asExpr() = c + ) + } + } + + /** A Caller of the `ExtractToFile` method. */ + class ZipFileExtensionsCallSink extends DecompressionBomb::Sink { + ZipFileExtensionsCallSink() { + exists(MethodCall mc | + mc.getTarget() + .hasFullyQualifiedName("System.IO.Compression", "ZipFileExtensions", "ExtractToFile") and + this.asExpr() = mc.getArgumentForName("source") and + mc.getArgumentForName("source") + .getType() + .hasFullyQualifiedName("System.IO.Compression", "ZipArchiveEntry") + or + mc.getTarget() + .hasFullyQualifiedName("System.IO.Compression", "ZipFileExtensions", + "ExtractToDirectory") and + this.asExpr() = mc.getArgumentForName("source") and + mc.getArgumentForName("source") + .getType() + .hasFullyQualifiedName("System.IO.Compression", "ZipArchive") + ) + } + } + + /** A Call to the `GZipStream` first argument of Constructor Call . */ + class GZipStreamSink extends DecompressionBomb::Sink { + GZipStreamSink() { + exists(Constructor mc | + mc.getDeclaringType().hasFullyQualifiedName("System.IO.Compression", "GZipStream") and + this.asExpr() = mc.getACall().getArgument(0) and + mc.getACall().getArgument(1) = + any(EnumConstant e | + e.hasFullyQualifiedName("System.IO.Compression", "CompressionMode", "Decompress") + ).getAnAccess() + ) + } + } + + /** A Call to the `BrotliStream` first argument of Constructor Call . */ + class BrotliStreamSink extends DecompressionBomb::Sink { + BrotliStreamSink() { + exists(Constructor mc | + mc.getDeclaringType().hasFullyQualifiedName("System.IO.Compression", "BrotliStream") and + this.asExpr() = mc.getACall().getArgument(0) and + mc.getACall().getArgument(1) = + any(EnumConstant e | + e.hasFullyQualifiedName("System.IO.Compression", "CompressionMode", "Decompress") + ).getAnAccess() + ) + } + } + + /** A Call to the `BrotliStream` first argument of Constructor Call . */ + class DeflateStreamSink extends DecompressionBomb::Sink { + DeflateStreamSink() { + exists(Constructor mc | + mc.getDeclaringType().hasFullyQualifiedName("System.IO.Compression", "DeflateStream") and + this.asExpr() = mc.getACall().getArgument(0) and + mc.getACall().getArgument(1) = + any(EnumConstant e | + e.hasFullyQualifiedName("System.IO.Compression", "CompressionMode", "Decompress") + ).getAnAccess() + ) + } + } + + /** A Call to the `ZLibStream` first argument of Constructor Call . */ + class ZLibStreamSink extends DecompressionBomb::Sink { + ZLibStreamSink() { + exists(Constructor mc | + mc.getDeclaringType().hasFullyQualifiedName("System.IO.Compression", "ZLibStream") and + this.asExpr() = mc.getACall().getArgument(0) and + mc.getACall().getArgument(1) = + any(EnumConstant e | + e.hasFullyQualifiedName("System.IO.Compression", "CompressionMode", "Decompress") + ).getAnAccess() + ) + } + } + + class DecompressionAdditionalStep extends DecompressionBomb::AdditionalStep { + override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(MethodCall mc | + mc.getTarget() + .hasFullyQualifiedName("System.IO.Compression", "ZipFileExtensions", "ExtractToFile") and + node2.asExpr() = mc and + node1.asExpr() = mc.getArgumentForName("source") + ) + or + exists(MethodCall mc | + mc.getTarget().hasFullyQualifiedName("System.IO.Compression", "ZipArchive", "GetEntry") and + node1.asExpr() = mc.getQualifier() and + node2.asExpr() = mc + ) + or + exists(Call mc | + mc.getTarget() + .getDeclaringType() + .hasFullyQualifiedName("System.IO.Compression", "ZipArchive") and + node1.asExpr() = mc.getArgument(0) and + node2.asExpr() = mc + ) + or + exists(ZipArchiveOpen zo | + node1.asExpr() = zo.getArgument(0) and + node2.asExpr() = zo + ) + or + exists(MethodCall mc | + mc.getTarget().hasFullyQualifiedName("System.IO.Compression", "ZipArchiveEntry", "Open") + | + node1.asExpr() = mc.getQualifier() and + node2.asExpr() = mc + ) + } + } +} + +module K4osLz4 { + /** A Decode MethodCall of LZ4Stream */ + class K4osLZ4InitializeMethod extends MethodCall { + K4osLZ4InitializeMethod() { + this.getTarget().hasFullyQualifiedName("K4os.Compression.LZ4.Streams", "LZ4Stream", "Decode") + } + } + + /** The Methods responsible for reading Decompressed data */ + class K4osLZ4DecompressionMethod extends MethodCall { + K4osLZ4DecompressionMethod() { + exists(string methodName | methodName = ["Read", "ReadAsync", "CopyToAsync", "CopyTo"] | + this.getTarget() + .hasFullyQualifiedName("K4os.Compression.LZ4.Streams", "LZ4DecoderStream", methodName) + ) + } + } + + /** The first argument of LZ4DecoderStream decompression methods as decompression bomb sink */ + class K4osLZ4LibSink extends DecompressionBomb::Sink { + K4osLZ4LibSink() { exists(K4osLZ4DecompressionMethod mc | this.asExpr() = mc.getArgument(0)) } + } + + class DecompressionAdditionalStep extends DecompressionBomb::AdditionalStep { + override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(K4osLZ4DecompressionMethod mc | + node1.asExpr() = mc.getQualifier() and + node2.asExpr() = mc.getArgument(0) + ) + or + exists(K4osLZ4InitializeMethod mc | + node2.asExpr() = mc and + node1.asExpr() = mc.getArgument(0) + ) + } + } +} + +/** The first argument of LZ4DecoderStream decompression methods as decompression bomb sink */ +module SharpZip { + class SharpZipLibType extends RefType { + SharpZipLibType() { + this.hasFullyQualifiedName("ICSharpCode.SharpZipLib.Zip", "ZipInputStream") or + this.hasFullyQualifiedName("ICSharpCode.SharpZipLib.BZip2", "BZip2InputStream") or + this.hasFullyQualifiedName("ICSharpCode.SharpZipLib.GZip", "GZipInputStream") or + this.hasFullyQualifiedName("ICSharpCode.SharpZipLib.Lzw", "LzwInputStream") or + this.hasFullyQualifiedName("ICSharpCode.SharpZipLib.Tar", "TarInputStream") or + this.hasFullyQualifiedName("ICSharpCode.SharpZipLib.Zip.Compression.Streams", + "InflaterInputStream") + } + } + + /** The methods for decompressing data */ + class SharpZipLibDecompressionMethod extends MethodCall { + SharpZipLibDecompressionMethod() { + exists(string methodName | + methodName = + [ + "Read", "ReadAsync", "ReadAtLeast", "CopyToAsync", "CopyTo", "CopyEntryContentsAsync", + "CopyEntryContents" + ] + | + this.getTarget() + .hasFullyQualifiedName("ICSharpCode.SharpZipLib.Zip", "ZipInputStream", methodName) or + this.getTarget() + .hasFullyQualifiedName("ICSharpCode.SharpZipLib.BZip2", "BZip2InputStream", methodName) or + this.getTarget() + .hasFullyQualifiedName("ICSharpCode.SharpZipLib.GZip", "GZipInputStream", methodName) or + this.getTarget() + .hasFullyQualifiedName("ICSharpCode.SharpZipLib.Lzw", "LzwInputStream", methodName) or + this.getTarget() + .hasFullyQualifiedName("ICSharpCode.SharpZipLib.Tar", "TarInputStream", methodName) or + this.getTarget() + .hasFullyQualifiedName("ICSharpCode.SharpZipLib.Zip.Compression.Streams", + "InflaterInputStream", methodName) + ) + } + } + + /** The first argument of SharpZipLib decompression methods as decompression bomb sink */ + class SharpZipLibSink extends DecompressionBomb::Sink { + SharpZipLibSink() { + exists(SharpZipLibDecompressionMethod mc | this.asExpr() = mc.getArgument(0)) + } + } + + /** The `ExtractZip` first argument or method call considered as decompression sink bomb */ + class SharpZipLibFastZipSink extends DecompressionBomb::Sink { + SharpZipLibFastZipSink() { + exists(MethodCall mc | + mc.getTarget() + .hasFullyQualifiedName("ICSharpCode.SharpZipLib.Zip", "ZipInputStream", "ExtractZip") and + this.asExpr() = mc.getArgument(0) + ) + } + } + + class DecompressionAdditionalStep extends DecompressionBomb::AdditionalStep { + override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(MethodCall mc, string methodName | + methodName = + [ + "Read", "ReadAsync", "ReadAtLeast", "CopyToAsync", "CopyTo", "CopyEntryContentsAsync", + "CopyEntryContents" + ] + | + ( + mc.getTarget().getDeclaringType() instanceof SharpZipLibType or + mc.getTarget().hasFullyQualifiedName("System.IO", "Stream", methodName) + ) and + node1.asExpr() = mc.getQualifier() and + node2.asExpr() = mc.getArgument(0) + ) + or + exists(Constructor c | + c.getDeclaringType() instanceof SharpZipLibType and + node1.asExpr() = c.getACall().getArgument(0) and + node2.asExpr() = c.getACall() + ) + } + } +} diff --git a/csharp/ql/test/experimental/CWE-502-DecompressionBombs/DecompressionBombs.cs b/csharp/ql/test/experimental/CWE-502-DecompressionBombs/DecompressionBombs.cs new file mode 100644 index 000000000000..94ef64b717f0 --- /dev/null +++ b/csharp/ql/test/experimental/CWE-502-DecompressionBombs/DecompressionBombs.cs @@ -0,0 +1,151 @@ +using System.IO.Compression; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.Primitives; + +namespace MultipartFormWebAPITest.Controllers; + +[Route("api/[controller]")] +[ApiController] +public class ZipFile1Controller : ControllerBase +{ + [HttpPost] + public IActionResult Post(List files) + { + foreach (var formFile in files) + { + ZipHelpers.Bomb2(formFile.FileName); + ZipHelpers.BombSafe(formFile.FileName); + ZipHelpers.Bomb3(formFile.OpenReadStream()); + } + + + StringValues file1; + StringValues header1; + Request.Form.TryGetValue("file", out file1); + Request.Headers.TryGetValue("Header1", out header1); + Request.Query.TryGetValue("queryParam1", out var qParam); + + var a = Request.Headers["a"]; + var b = Request.Headers.ContentType; + + var d = Array.Empty>(); + Request.Headers.CopyTo(d, 0); + + ZipHelpers.Bomb2(file1!); + ZipHelpers.Bomb2(header1!); + ZipHelpers.Bomb2(qParam!); + + return Accepted(); + } +} + +internal static class ZipHelpers +{ + public static void Bomb3(Stream compressedFileStream) + { + using var decompressor = new GZipStream(compressedFileStream, CompressionMode.Decompress); + using var ms = new MemoryStream(); + decompressor.CopyTo(ms); + using var decompressor2 = new BrotliStream(compressedFileStream, CompressionMode.Decompress); + using var ms2 = new MemoryStream(); + decompressor2.CopyTo(ms2); + using var decompressor3 = new DeflateStream(compressedFileStream, CompressionMode.Decompress); + using var ms3 = new MemoryStream(); + decompressor3.CopyTo(ms3); + } + + public static void Bomb2(string filename) + { + using var zipToOpen = new FileStream(filename, FileMode.Open); + using var archive = new ZipArchive(zipToOpen, ZipArchiveMode.Read); + archive.ExtractToDirectory("./tmp/", true); + (archive.GetEntry("aaa") ?? throw new InvalidOperationException()).ExtractToFile("/tmp/tmp"); + foreach (var entry in archive.Entries) entry.ExtractToFile("./output.txt", true); // Sensitive + } + + public static void BombSafe(string filename) + { + const long maxLength = 10 * 1024 * 1024; // 10MB + using var zipFile = ZipFile.Open(filename, ZipArchiveMode.Read); + // Quickly check the value from the zip header + var declaredSize = zipFile.Entries.Sum(entry => entry.Length); + if (declaredSize > maxLength) + throw new Exception("Archive is too big"); + foreach (var entry in zipFile.Entries) + { + using var entryStream = entry.Open(); + // Use MaxLengthStream to ensure we don't read more than the declared length + using var maxLengthStream = new MaxLengthStream(entryStream, entry.Length); + // Be sure to use the maxLengthSteam variable to read the content of the entry, not entryStream + using var ms = new MemoryStream(); + maxLengthStream.CopyTo(ms); + } + + const string zipPath = @"c:\users\exampleuser\start.zip"; + const string extractPath = @"c:\users\exampleuser\extract"; + using var archive = ZipFile.Open(zipPath, ZipArchiveMode.Update); + archive.ExtractToDirectory(extractPath); + } +} + +internal sealed class MaxLengthStream : Stream +{ + private readonly Stream _stream; + private long _length; + + public MaxLengthStream(Stream stream, long maxLength) + { + _stream = stream ?? throw new ArgumentNullException(nameof(stream)); + MaxLength = maxLength; + } + + private long MaxLength { get; } + + public override bool CanRead => _stream.CanRead; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => _stream.Length; + + public override long Position + { + get => _stream.Position; + set => throw new NotSupportedException(); + } + + public override int Read(byte[] buffer, int offset, int count) + { + var result = _stream.Read(buffer, offset, count); + _length += result; + if (_length > MaxLength) + throw new Exception("Stream is larger than the maximum allowed size"); + + return result; + } + + + public override void Flush() + { + throw new NotSupportedException(); + } + + public override long Seek(long offset, SeekOrigin origin) + { + throw new NotSupportedException(); + } + + public override void SetLength(long value) + { + throw new NotSupportedException(); + } + + public override void Write(byte[] buffer, int offset, int count) + { + throw new NotSupportedException(); + } + + protected override void Dispose(bool disposing) + { + _stream.Dispose(); + base.Dispose(disposing); + } +} \ No newline at end of file diff --git a/csharp/ql/test/experimental/CWE-502-DecompressionBombs/DecompressionBombs.qlref b/csharp/ql/test/experimental/CWE-502-DecompressionBombs/DecompressionBombs.qlref new file mode 100644 index 000000000000..23c61806317b --- /dev/null +++ b/csharp/ql/test/experimental/CWE-502-DecompressionBombs/DecompressionBombs.qlref @@ -0,0 +1 @@ +experimental/CWE-502-DecompressionBombs/DecompressionBombs.ql \ No newline at end of file