Skip to content

Commit 4dc1a10

Browse files
committed
update tests for zip4j, add aditional flow steps for zip4j, remove BombTypeInputStream class since we don't need it anymore, add a predicate which was for testing porpose and was junk
1 parent c8749ff commit 4dc1a10

File tree

2 files changed

+25
-31
lines changed

2 files changed

+25
-31
lines changed

java/ql/src/experimental/semmle/code/java/security/DecompressionBomb.qll

Lines changed: 21 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,6 @@ module DecompressionBomb {
2323
private class ReadInputStreamQualifierSink extends DecompressionBomb::Sink {
2424
ReadInputStreamQualifierSink() { this.asExpr() = any(BombReadInputStreamCall r).getQualifier() }
2525
}
26-
27-
abstract class BombTypeInputStream extends RefType { }
2826
}
2927

3028
/**
@@ -34,7 +32,7 @@ module XerialSnappy {
3432
/**
3533
* A type that is responsible for `SnappyInputStream` Class
3634
*/
37-
class TypeInputStream extends DecompressionBomb::BombTypeInputStream {
35+
class TypeInputStream extends RefType {
3836
TypeInputStream() {
3937
this.getASupertype*().hasQualifiedName("org.xerial.snappy", "SnappyInputStream")
4038
}
@@ -99,7 +97,7 @@ module ApacheCommons {
9997
/**
10098
* The types that are responsible for specific compression format of `CompressorInputStream` Class
10199
*/
102-
class TypeCompressors extends DecompressionBomb::BombTypeInputStream {
100+
class TypeCompressors extends RefType {
103101
TypeCompressors() {
104102
this.getASupertype*()
105103
.hasQualifiedName("org.apache.commons.compress.compressors.gzip",
@@ -163,15 +161,6 @@ module ApacheCommons {
163161
)
164162
}
165163
}
166-
167-
predicate step(DataFlow::Node n1, DataFlow::Node n2) {
168-
exists(Call call |
169-
// Constructors
170-
call.getCallee().getDeclaringType() instanceof TypeCompressors and
171-
call.getArgument(0) = n1.asExpr() and
172-
call = n2.asExpr()
173-
)
174-
}
175164
}
176165

177166
/**
@@ -181,7 +170,7 @@ module ApacheCommons {
181170
/**
182171
* The types that are responsible for specific compression format of `ArchiveInputStream` Class
183172
*/
184-
class TypeArchivers extends DecompressionBomb::BombTypeInputStream {
173+
class TypeArchivers extends RefType {
185174
TypeArchivers() {
186175
this.getASupertype*()
187176
.hasQualifiedName("org.apache.commons.compress.archivers.ar", "ArArchiveInputStream") or
@@ -235,7 +224,7 @@ module ApacheCommons {
235224
/**
236225
* A type that is responsible for `ArchiveInputStream` Class
237226
*/
238-
class TypeArchivers extends DecompressionBomb::BombTypeInputStream {
227+
class TypeArchivers extends RefType {
239228
TypeArchivers() {
240229
this.getASupertype*()
241230
.hasQualifiedName("org.apache.commons.compress.archivers", "ArchiveStreamFactory")
@@ -253,11 +242,7 @@ module ApacheCommons {
253242
}
254243

255244
/**
256-
* Gets `n1` and `n2` which `CompressorInputStream n2 = new CompressorStreamFactory().createCompressorInputStream(n1)`
257-
* or `ArchiveInputStream n2 = new ArchiveStreamFactory().createArchiveInputStream(n1)` or
258-
* `n1.read(n2)`,
259-
* second one is added because of sanitizer, we want to compare return value of each `read` or similar method
260-
* that whether there is a flow to a comparison between total read of decompressed stream and a constant value
245+
* Gets `n1` and `n2` which `ZipInputStream n2 = new ZipInputStream(n1)`
261246
*/
262247
private class CompressorsAndArchiversAdditionalTaintStep extends DecompressionBomb::AdditionalStep
263248
{
@@ -314,12 +299,21 @@ module Zip4j {
314299
}
315300
}
316301

317-
class Sink extends DecompressionBomb::Sink {
318-
Sink() {
319-
this.asExpr() = any(ReadInputStreamCall r).getQualifier()
320-
or
321-
exists(ConstructorCall call | call.getConstructedType() instanceof TypeZipInputStream |
322-
this.asExpr() = call.getArgument(0)
302+
/**
303+
* Gets `n1` and `n2` which `CompressorInputStream n2 = new CompressorStreamFactory().createCompressorInputStream(n1)`
304+
* or `ArchiveInputStream n2 = new ArchiveStreamFactory().createArchiveInputStream(n1)` or
305+
* `n1.read(n2)`,
306+
* second one is added because of sanitizer, we want to compare return value of each `read` or similar method
307+
* that whether there is a flow to a comparison between total read of decompressed stream and a constant value
308+
*/
309+
private class CompressorsAndArchiversAdditionalTaintStep extends DecompressionBomb::AdditionalStep
310+
{
311+
override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
312+
exists(Call call |
313+
// Constructors
314+
call.getCallee().getDeclaringType() instanceof TypeZipInputStream and
315+
call.getArgument(0) = n1.asExpr() and
316+
call = n2.asExpr()
323317
)
324318
}
325319
}
@@ -332,7 +326,7 @@ module Zip {
332326
/**
333327
* The Types that are responsible for `ZipInputStream`, `GZIPInputStream`, `InflaterInputStream` Classes
334328
*/
335-
class TypeInputStream extends DecompressionBomb::BombTypeInputStream {
329+
class TypeInputStream extends RefType {
336330
TypeInputStream() {
337331
this.getASupertype*()
338332
.hasQualifiedName("java.util.zip",

java/ql/test/experimental/query-tests/security/CWE-522-DecompressionBombs/Zip4jHandler.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,11 @@ public static void zip4jZipInputStream(InputStream inputStream) throws IOExcepti
1212
LocalFileHeader localFileHeader;
1313
int readLen;
1414
byte[] readBuffer = new byte[4096];
15-
try (ZipInputStream zipInputStream = new ZipInputStream(inputStream)) { // $ hasTaintFlow="inputStream"
15+
try (ZipInputStream zipInputStream = new ZipInputStream(inputStream)) {
1616
while ((localFileHeader = zipInputStream.getNextEntry()) != null) {
1717
File extractedFile = new File(localFileHeader.getFileName());
1818
try (OutputStream outputStream = new FileOutputStream(extractedFile)) {
19-
while ((readLen = zipInputStream.read(readBuffer)) != -1) {
19+
while ((readLen = zipInputStream.read(readBuffer)) != -1) { // $ hasTaintFlow="zipInputStream"
2020
outputStream.write(readBuffer, 0, readLen);
2121
}
2222
}
@@ -28,12 +28,12 @@ public static void zip4jZipInputStreamSafe(InputStream inputStream) throws IOExc
2828
LocalFileHeader localFileHeader;
2929
int readLen;
3030
byte[] readBuffer = new byte[4096];
31-
try (ZipInputStream zipInputStream = new ZipInputStream(inputStream)) { // $ hasTaintFlow="inputStream"
31+
try (ZipInputStream zipInputStream = new ZipInputStream(inputStream)) {
3232
while ((localFileHeader = zipInputStream.getNextEntry()) != null) {
3333
File extractedFile = new File(localFileHeader.getFileName());
3434
try (OutputStream outputStream = new FileOutputStream(extractedFile)) {
3535
int totallRead = 0;
36-
while ((readLen = zipInputStream.read(readBuffer)) != -1) {
36+
while ((readLen = zipInputStream.read(readBuffer)) != -1) { // $ hasTaintFlow="zipInputStream"
3737
totallRead += readLen;
3838
if (totallRead > 1024 * 1024 * 4) {
3939
System.out.println("potential Bomb");

0 commit comments

Comments
 (0)