diff --git a/java/ql/src/experimental/Security/CWE/CWE-522-DecompressionBombs/DecompressionBomb.ql b/java/ql/src/experimental/Security/CWE/CWE-522-DecompressionBombs/DecompressionBomb.ql index 328355c9136..a1217343bdf 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-522-DecompressionBombs/DecompressionBomb.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-522-DecompressionBombs/DecompressionBomb.ql @@ -37,6 +37,8 @@ module DecompressionBombsConfig implements DataFlow::StateConfigSig { state instanceof ApacheCommons or state instanceof XerialSnappy + or + state instanceof UtilZip ) } diff --git a/java/ql/src/experimental/semmle/code/java/security/DecompressionBomb.qll b/java/ql/src/experimental/semmle/code/java/security/DecompressionBomb.qll index 1d97eca4d4a..e2258e35163 100644 --- a/java/ql/src/experimental/semmle/code/java/security/DecompressionBomb.qll +++ b/java/ql/src/experimental/semmle/code/java/security/DecompressionBomb.qll @@ -81,16 +81,11 @@ module XerialSnappy { this.getReceiverType() instanceof TypeInputStream and this.getCallee().hasName(["read", "readNBytes", "readAllBytes"]) } - - /** - * A method Access as a sink which responsible for reading bytes - */ - MethodCall getAByteRead() { result = this } } class Sink extends DecompressionBomb::Sink { override predicate sink(DataFlow::Node sink, DecompressionBomb::DecompressionState state) { - sink.asExpr() = any(ReadInputStreamCall r).getAByteRead() and + sink.asExpr() = any(ReadInputStreamCall r) and state instanceof DecompressionBomb::XerialSnappy } } @@ -203,16 +198,11 @@ module ApacheCommons { this.getReceiverType() instanceof TypeCompressors and this.getCallee().hasName(["read", "readNBytes", "readAllBytes"]) } - - /** - * A method Access as a sink which responsible for reading bytes - */ - MethodCall getAByteRead() { result = this } } class Sink extends DecompressionBomb::Sink { override predicate sink(DataFlow::Node sink, DecompressionBomb::DecompressionState state) { - sink.asExpr() = any(ReadInputStreamCall r).getAByteRead() and + sink.asExpr() = any(ReadInputStreamCall r) and state instanceof DecompressionBomb::ApacheCommons } } @@ -278,16 +268,11 @@ module ApacheCommons { this.getReceiverType() instanceof TypeArchivers and this.getCallee().hasName(["read", "readNBytes", "readAllBytes"]) } - - /** - * A method Access as a sink which responsible for reading bytes - */ - MethodCall getAByteRead() { result = this } } class Sink extends DecompressionBomb::Sink { override predicate sink(DataFlow::Node sink, DecompressionBomb::DecompressionState state) { - sink.asExpr() = any(ReadInputStreamCall r).getAByteRead() and + sink.asExpr() = any(ReadInputStreamCall r) and state instanceof DecompressionBomb::ApacheCommons } } @@ -367,16 +352,11 @@ module ApacheCommons { ) and this.getCallee().hasName(["read", "readNBytes", "readAllBytes"]) } - - /** - * A method Access as a sink which responsible for reading bytes - */ - MethodCall getAByteRead() { result = this } } class Sink extends DecompressionBomb::Sink { override predicate sink(DataFlow::Node sink, DecompressionBomb::DecompressionState state) { - sink.asExpr() = any(ReadInputStreamCall r).getAByteRead() and + sink.asExpr() = any(ReadInputStreamCall r) and state instanceof DecompressionBomb::ApacheCommons } } @@ -404,16 +384,11 @@ module Zip4j { this.getReceiverType() instanceof TypeZipInputStream and this.getMethod().hasName(["read", "readNBytes", "readAllBytes"]) } - - /** - * A method Access as a sink which responsible for reading bytes - */ - MethodCall getAByteRead() { result = this } } class Sink extends DecompressionBomb::Sink { override predicate sink(DataFlow::Node sink, DecompressionBomb::DecompressionState state) { - sink.asExpr() = any(ReadInputStreamCall r).getAByteRead() and + sink.asExpr() = any(ReadInputStreamCall r) and state instanceof DecompressionBomb::Zip4j } } @@ -446,40 +421,6 @@ module Zip4j { } } -/** - * Providing sinks that can be related to reading uncontrolled buffer and bytes for `org.apache.commons.io` package - */ -module CommonsIO { - /** - * The Access to Methods which work with byes and inputStreams and buffers - */ - class IOUtils extends MethodCall { - IOUtils() { - this.getMethod() - .hasName([ - "copy", "copyLarge", "read", "readFully", "readLines", "toBufferedInputStream", - "toByteArray", "toCharArray", "toString", "buffer" - ]) and - this.getMethod().getDeclaringType().hasQualifiedName("org.apache.commons.io", "IOUtils") - } - } - - class Sink extends DecompressionBomb::Sink { - override predicate sink(DataFlow::Node sink, DecompressionBomb::DecompressionState state) { - sink.asExpr() = any(IOUtils r).getArgument(0) and - ( - state instanceof DecompressionBomb::Zip4j - or - state instanceof DecompressionBomb::Inflator - or - state instanceof DecompressionBomb::ApacheCommons - or - state instanceof DecompressionBomb::XerialSnappy - ) - } - } -} - /** * Providing Decompression sinks and additional taint steps for `java.util.zip` package */ @@ -503,16 +444,11 @@ module Zip { this.getReceiverType() instanceof TypeInputStream and this.getCallee().hasName(["read", "readNBytes", "readAllBytes"]) } - - /** - * A method Access as a sink which responsible for reading bytes - */ - MethodCall getAByteRead() { result = this } } class ReadInputStreamSink extends DecompressionBomb::Sink { override predicate sink(DataFlow::Node sink, DecompressionBomb::DecompressionState state) { - sink.asExpr() = any(ReadInputStreamCall r).getAByteRead() and + sink.asExpr() = any(ReadInputStreamCall r) and state instanceof DecompressionBomb::UtilZip } } @@ -602,16 +538,11 @@ module Zip { this.getReceiverType() instanceof TypeInflator and this.getCallee().hasName("inflate") } - - /** - * A method Access as a sink which responsible for reading bytes - */ - MethodCall getAByteRead() { result = this } } class InflateSink extends DecompressionBomb::Sink { override predicate sink(DataFlow::Node sink, DecompressionBomb::DecompressionState state) { - sink.asExpr() = any(InflateCall r).getAByteRead() and + sink.asExpr() = any(InflateCall r) and state instanceof DecompressionBomb::Inflator } } diff --git a/java/ql/test/experimental/query-tests/security/CWE-522-DecompressionBombs/src/main/java/com/Bombs/CommonsCompressHandler.java b/java/ql/test/experimental/query-tests/security/CWE-522-DecompressionBombs/src/main/java/com/Bombs/CommonsCompressHandler.java index 0f2c2e296cc..e1bf7820666 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-522-DecompressionBombs/src/main/java/com/Bombs/CommonsCompressHandler.java +++ b/java/ql/test/experimental/query-tests/security/CWE-522-DecompressionBombs/src/main/java/com/Bombs/CommonsCompressHandler.java @@ -8,7 +8,6 @@ import org.apache.commons.compress.compressors.CompressorException; import org.apache.commons.compress.compressors.CompressorInputStream; import org.apache.commons.compress.compressors.CompressorStreamFactory; import org.apache.commons.compress.compressors.gzip.*; -import org.apache.commons.io.IOUtils; public class CommonsCompressHandler { public static void commonsCompressorInputStream(InputStream inputStream) throws IOException { @@ -37,16 +36,6 @@ public class CommonsCompressHandler { } out.close(); gzIn.close(); - - try (GzipCompressorInputStream gzIn2 = - new org.apache.commons.compress.compressors.gzip.GzipCompressorInputStream(in)) { - File f = new File("tmpfile"); - try (OutputStream o = Files.newOutputStream(f.toPath())) { - IOUtils.copy(gzIn2, o); // BAD - } - } catch (IOException e) { - throw new RuntimeException(e); - } } static void commonsCompressArchiveInputStream(InputStream inputStream) throws ArchiveException { @@ -54,21 +43,7 @@ public class CommonsCompressHandler { new org.apache.commons.compress.archivers.arj.ArjArchiveInputStream(inputStream); new org.apache.commons.compress.archivers.cpio.CpioArchiveInputStream(inputStream); new org.apache.commons.compress.archivers.jar.JarArchiveInputStream(inputStream); - try (org.apache.commons.compress.archivers.zip.ZipArchiveInputStream zipInputStream = - new org.apache.commons.compress.archivers.zip.ZipArchiveInputStream(inputStream)) { - ArchiveEntry entry = null; - while ((entry = zipInputStream.getNextEntry()) != null) { - if (!zipInputStream.canReadEntryData(entry)) { - continue; - } - File f = new File("tmpfile"); - try (OutputStream o = Files.newOutputStream(f.toPath())) { - IOUtils.copy(zipInputStream, o); // BAD - } - } - } catch (IOException e) { - throw new RuntimeException(e); - } + new org.apache.commons.compress.archivers.zip.ZipArchiveInputStream(inputStream); } static void commonsCompressArchiveInputStream2(InputStream inputStream) { @@ -83,7 +58,7 @@ public class CommonsCompressHandler { File f = new File("tmpfile"); try (OutputStream outputStream = new FileOutputStream(f)) { int readLen; - while ((readLen = zipInputStream.read(readBuffer)) != -1) { // BAD + while ((readLen = zipInputStream.read(readBuffer)) != -1) { // BAD outputStream.write(readBuffer, 0, readLen); } } @@ -106,7 +81,7 @@ public class CommonsCompressHandler { File f = new File("tmpfile"); try (OutputStream outputStream = new FileOutputStream(f)) { int readLen; - while ((readLen = zipInputStream.read(readBuffer)) != -1) { // BAD + while ((readLen = zipInputStream.read(readBuffer)) != -1) { // BAD outputStream.write(readBuffer, 0, readLen); } } @@ -121,7 +96,7 @@ public class CommonsCompressHandler { int buffersize = 4096; final byte[] buffer = new byte[buffersize]; int n = 0; - while (-1 != (n = in.read(buffer))) { // BAD + while (-1 != (n = in.read(buffer))) { // BAD out.write(buffer, 0, n); } out.close();