Skip to content

Commit 761aede

Browse files
am0o0am0o0
committed
perfomed review suggestions, make Decompression Sink simpler, uncomment the isBarrier, fix some naming issues in tests
1 parent ac5e9c7 commit 761aede

6 files changed

Lines changed: 37 additions & 118 deletions

File tree

‎go/ql/src/experimental/CWE-522-DecompressionBombs/DecompressionBombs.ql‎

Lines changed: 8 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -47,37 +47,17 @@ module DecompressionBombsConfig implements DataFlow::StateConfigSig {
4747
)
4848
}
4949

50-
predicate isBarrier(DataFlow::Node node, FlowState state) {
51-
// //here I want to the CopyN return value be compared with < or > but I can't reach the tainted result
52-
// // exists(Function f | f.hasQualifiedName("io", "CopyN") |
53-
// // node = f.getACall().getArgument([0, 1]) and
54-
// // TaintTracking::localExprTaint(f.getACall().getResult(_).asExpr(),
55-
// // any(RelationalComparisonExpr e).getAChildExpr*())
56-
// // )
57-
// // or
58-
// exists(Function f | f.hasQualifiedName("io", "LimitReader") |
59-
// node = f.getACall().getArgument(0) and f.getACall().getArgument(1).isConst()
60-
// ) and
61-
// state =
62-
// [
63-
// "ZstdNewReader", "XzNewReader", "GzipNewReader", "S2NewReader", "SnapyNewReader",
64-
// "ZlibNewReader", "FlateNewReader", "Bzip2NewReader", "ZipOpenReader", "ZipKlauspost"
65-
// ]
66-
none()
50+
predicate isBarrier(DataFlow::Node node) {
51+
// here I want to the CopyN return value be compared with < or > but I can't reach the tainted result
52+
exists(Function f | f.hasQualifiedName("io", "CopyN") |
53+
node = f.getACall().getArgument(1) and
54+
TaintTracking::localExprTaint(f.getACall().getResult(0).asExpr(),
55+
// only >=, <=,>,<
56+
any(RelationalComparisonExpr rce).getAnOperand())
57+
)
6758
}
6859
}
6960

70-
// // here I want to the CopyN return value be compared with < or > but I can't reach the tainted result
71-
// predicate test(DataFlow::Node n2) { any(Test testconfig).hasFlowTo(n2) }
72-
// class Test extends DataFlow::Configuration {
73-
// Test() { this = "test" }
74-
// override predicate isSource(DataFlow::Node source) {
75-
// exists(Function f | f.hasQualifiedName("io", "CopyN") |
76-
// f.getACall().getResult(0) = source
77-
// )
78-
// }
79-
// override predicate isSink(DataFlow::Node sink) { sink instanceof DataFlow::Node }
80-
// }
8161
module DecompressionBombsFlow = TaintTracking::GlobalWithState<DecompressionBombsConfig>;
8262

8363
import DecompressionBombsFlow::PathGraph

‎go/ql/src/experimental/CWE-522-DecompressionBombs/MultipartAndFormRemoteSource.qll‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,19 +3,18 @@ import semmle.go.dataflow.Properties
33

44
class MimeMultipartFileHeader extends UntrustedFlowSource::Range {
55
MimeMultipartFileHeader() {
6-
exists(DataFlow::Variable v |
7-
v.hasQualifiedName("mime/multipart.FileHeader", ["Filename", "Header"]) and
8-
this = v.getARead()
6+
exists(DataFlow::FieldReadNode frn | this = frn |
7+
frn.getField()
8+
.hasQualifiedName("mime/multipart.FileHeader", ["Filename", "Header"], "RequestBody")
99
)
1010
or
1111
exists(DataFlow::Method m |
1212
m.hasQualifiedName("mime/multipart.FileHeader", "Open") and
13-
this = m.getACall()
13+
this = m.getACall().getResult(0)
1414
)
1515
or
16-
exists(DataFlow::Variable v |
17-
v.hasQualifiedName("mime/multipart.Form", "Value") and
18-
this = v.getARead()
16+
exists(DataFlow::FieldReadNode frn |
17+
frn.getField().hasQualifiedName("mime/multipart.Form", "Value")
1918
)
2019
}
2120
}

‎go/ql/src/experimental/CWE-522-DecompressionBombs/example_good.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,9 @@ func ZipOpenReader(filename string) {
1212
r, _ := zip.OpenReader(filename)
1313
var totalBytes int64
1414
for _, f := range r.File {
15+
rc, _ := f.Open()
16+
totalBytes = 0
1517
for {
16-
rc, _ := f.Open()
1718
result, _ := io.CopyN(os.Stdout, rc, 68)
1819
if result == 0 {
1920
break

‎go/ql/src/experimental/frameworks/DecompressionBombs.qll‎

Lines changed: 19 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,6 @@ import go
33
module DecompressionBombs {
44
class FlowState = DataFlow::FlowState;
55

6-
/**
7-
* The Sinks of uncontrolled data decompression
8-
*/
9-
class Sink extends DataFlow::Node {
10-
Sink() { this = any(Range r).sink() }
11-
}
12-
136
/**
147
* The additional taint steps that need for creating taint tracking or dataflow.
158
*/
@@ -30,30 +23,20 @@ module DecompressionBombs {
3023
}
3124

3225
/**
33-
* A abstract class responsible for extending new decompression sinks
26+
* The Sinks of uncontrolled data decompression
3427
*/
35-
abstract private class Range extends DataFlow::Node {
36-
/**
37-
* Gets the sink of responsible for decompression node
38-
*
39-
* it can be a path, stream of compressed data,
40-
* or a call to function that use pipe
41-
*/
42-
abstract DataFlow::Node sink();
43-
}
28+
abstract class Sink extends DataFlow::Node { }
4429

4530
/**
4631
* Provides Decompression Sinks and additional flow steps for `github.com/DataDog/zstd` package
4732
*/
4833
module DataDogZstd {
49-
class TheSink extends Range {
34+
class TheSink extends Sink {
5035
TheSink() {
5136
exists(Method f | f.hasQualifiedName("github.com/DataDog/zstd", "reader", "Read") |
5237
this = f.getACall().getReceiver()
5338
)
5439
}
55-
56-
override DataFlow::Node sink() { result = this }
5740
}
5841

5942
class TheAdditionalTaintStep extends AdditionalTaintStep {
@@ -83,7 +66,7 @@ module DecompressionBombs {
8366
* Provides Decompression Sinks and additional flow steps for `github.com/klauspost/compress/zstd` package
8467
*/
8568
module KlauspostZstd {
86-
class TheSink extends Range {
69+
class TheSink extends Sink {
8770
TheSink() {
8871
exists(Method f |
8972
f.hasQualifiedName("github.com/klauspost/compress/zstd", "Decoder",
@@ -98,8 +81,6 @@ module DecompressionBombs {
9881
this = f.getACall().getReceiver()
9982
)
10083
}
101-
102-
override DataFlow::Node sink() { result = this }
10384
}
10485

10586
class TheAdditionalTaintStep extends AdditionalTaintStep {
@@ -195,14 +176,12 @@ module DecompressionBombs {
195176
* Provides Decompression Sinks and additional taint steps for `github.com/ulikunitz/xz` package
196177
*/
197178
module UlikunitzXz {
198-
class TheSink extends Range {
179+
class TheSink extends Sink {
199180
TheSink() {
200181
exists(Method f | f.hasQualifiedName("github.com/ulikunitz/xz", "Reader", "Read") |
201182
this = f.getACall().getReceiver()
202183
)
203184
}
204-
205-
override DataFlow::Node sink() { result = this }
206185
}
207186

208187
class TheAdditionalTaintStep extends AdditionalTaintStep {
@@ -231,14 +210,12 @@ module DecompressionBombs {
231210
* Provides Decompression Sinks and additional taint steps for `compress/gzip` package
232211
*/
233212
module CompressGzip {
234-
class TheSink extends Range {
213+
class TheSink extends Sink {
235214
TheSink() {
236215
exists(Method f | f.hasQualifiedName("compress/gzip", "Reader", "Read") |
237216
this = f.getACall().getReceiver()
238217
)
239218
}
240-
241-
override DataFlow::Node sink() { result = this }
242219
}
243220

244221
class TheAdditionalTaintStep extends AdditionalTaintStep {
@@ -268,7 +245,7 @@ module DecompressionBombs {
268245
* Provides Decompression Sinks and additional taint steps for `github.com/klauspost/compress/gzip` package
269246
*/
270247
module KlauspostGzip {
271-
class TheSink extends Range {
248+
class TheSink extends Sink {
272249
TheSink() {
273250
exists(Method f |
274251
f.hasQualifiedName("github.com/klauspost/compress/gzip", "Reader", "Read")
@@ -283,8 +260,6 @@ module DecompressionBombs {
283260
this = f.getACall().getReceiver()
284261
)
285262
}
286-
287-
override DataFlow::Node sink() { result = this }
288263
}
289264

290265
class TheAdditionalTaintStep extends AdditionalTaintStep {
@@ -315,14 +290,12 @@ module DecompressionBombs {
315290
* Provides Decompression Sinks and additional taint steps for `compress/bzip2` package
316291
*/
317292
module CompressBzip2 {
318-
class TheSink extends Range {
293+
class TheSink extends Sink {
319294
TheSink() {
320295
exists(Method f | f.hasQualifiedName("compress/bzip2", "reader", "Read") |
321296
this = f.getACall().getReceiver()
322297
)
323298
}
324-
325-
override DataFlow::Node sink() { result = this }
326299
}
327300

328301
class TheAdditionalTaintStep extends AdditionalTaintStep {
@@ -352,14 +325,12 @@ module DecompressionBombs {
352325
* Provides Decompression Sinks and additional taint steps for `github.com/dsnet/compress/bzip2` package
353326
*/
354327
module DsnetBzip2 {
355-
class TheSink extends Range {
328+
class TheSink extends Sink {
356329
TheSink() {
357330
exists(Method f | f.hasQualifiedName("github.com/dsnet/compress/bzip2", "Reader", "Read") |
358331
this = f.getACall().getReceiver()
359332
)
360333
}
361-
362-
override DataFlow::Node sink() { result = this }
363334
}
364335

365336
class TheAdditionalTaintStep extends AdditionalTaintStep {
@@ -389,14 +360,12 @@ module DecompressionBombs {
389360
* Provides Decompression Sinks and additional taint steps for `github.com/dsnet/compress/flate` package
390361
*/
391362
module DsnetFlate {
392-
class TheSink extends Range {
363+
class TheSink extends Sink {
393364
TheSink() {
394365
exists(Method f | f.hasQualifiedName("github.com/dsnet/compress/flate", "Reader", "Read") |
395366
this = f.getACall().getReceiver()
396367
)
397368
}
398-
399-
override DataFlow::Node sink() { result = this }
400369
}
401370

402371
class TheAdditionalTaintStep extends AdditionalTaintStep {
@@ -426,14 +395,12 @@ module DecompressionBombs {
426395
* Provides Decompression Sinks and additional taint steps for `compress/flate` package
427396
*/
428397
module CompressFlate {
429-
class TheSink extends Range {
398+
class TheSink extends Sink {
430399
TheSink() {
431400
exists(Method f | f.hasQualifiedName("compress/flate", "decompressor", "Read") |
432401
this = f.getACall().getReceiver()
433402
)
434403
}
435-
436-
override DataFlow::Node sink() { result = this }
437404
}
438405

439406
class TheAdditionalTaintStep extends AdditionalTaintStep {
@@ -463,16 +430,14 @@ module DecompressionBombs {
463430
* Provides Decompression Sinks and additional taint steps for `github.com/klauspost/compress/flate` package
464431
*/
465432
module KlauspostFlate {
466-
class TheSink extends Range {
433+
class TheSink extends Sink {
467434
TheSink() {
468435
exists(Method f |
469436
f.hasQualifiedName("github.com/klauspost/compress/flate", "decompressor", "Read")
470437
|
471438
this = f.getACall().getReceiver()
472439
)
473440
}
474-
475-
override DataFlow::Node sink() { result = this }
476441
}
477442

478443
class TheAdditionalTaintStep extends AdditionalTaintStep {
@@ -482,7 +447,7 @@ module DecompressionBombs {
482447
DataFlow::Node fromNode, FlowState fromState, DataFlow::Node toNode, FlowState toState
483448
) {
484449
exists(Function f, DataFlow::CallNode call |
485-
f.hasQualifiedName(["github.com/klauspost/compress/flate"], ["NewReaderDict", "NewReader"]) and
450+
f.hasQualifiedName("github.com/klauspost/compress/flate", ["NewReaderDict", "NewReader"]) and
486451
call = f.getACall()
487452
|
488453
fromNode = call.getArgument(0) and
@@ -502,16 +467,14 @@ module DecompressionBombs {
502467
* Provides Decompression Sinks and additional taint steps for `github.com/klauspost/compress/zlib` package
503468
*/
504469
module KlauspostZlib {
505-
class TheSink extends Range {
470+
class TheSink extends Sink {
506471
TheSink() {
507472
exists(Method f |
508473
f.hasQualifiedName("github.com/klauspost/compress/zlib", "reader", "Read")
509474
|
510475
this = f.getACall().getReceiver()
511476
)
512477
}
513-
514-
override DataFlow::Node sink() { result = this }
515478
}
516479

517480
class TheAdditionalTaintStep extends AdditionalTaintStep {
@@ -541,14 +504,12 @@ module DecompressionBombs {
541504
* Provides Decompression Sinks and additional taint steps for `compress/zlib` package
542505
*/
543506
module CompressZlib {
544-
class TheSink extends Range {
507+
class TheSink extends Sink {
545508
TheSink() {
546509
exists(Method f | f.hasQualifiedName("compress/zlib", "reader", "Read") |
547510
this = f.getACall().getReceiver()
548511
)
549512
}
550-
551-
override DataFlow::Node sink() { result = this }
552513
}
553514

554515
class TheAdditionalTaintStep extends AdditionalTaintStep {
@@ -578,16 +539,14 @@ module DecompressionBombs {
578539
* Provides Decompression Sinks and additional taint steps for `github.com/golang/snappy` package
579540
*/
580541
module GolangSnappy {
581-
class TheSink extends Range {
542+
class TheSink extends Sink {
582543
TheSink() {
583544
exists(Method f |
584545
f.hasQualifiedName("github.com/golang/snappy", "Reader", ["Read", "ReadByte"])
585546
|
586547
this = f.getACall().getReceiver()
587548
)
588549
}
589-
590-
override DataFlow::Node sink() { result = this }
591550
}
592551

593552
class TheAdditionalTaintStep extends AdditionalTaintStep {
@@ -644,7 +603,7 @@ module DecompressionBombs {
644603
* Provides Decompression Sinks and additional taint steps for `github.com/klauspost/compress/s2` package
645604
*/
646605
module KlauspostS2 {
647-
class TheSink extends Range {
606+
class TheSink extends Sink {
648607
TheSink() {
649608
exists(Method m |
650609
m.hasQualifiedName("github.com/klauspost/compress/s2", "Reader",
@@ -653,8 +612,6 @@ module DecompressionBombs {
653612
this = m.getACall().getReceiver()
654613
)
655614
}
656-
657-
override DataFlow::Node sink() { result = this }
658615
}
659616

660617
class TheAdditionalTaintStep extends AdditionalTaintStep {
@@ -684,22 +641,20 @@ module DecompressionBombs {
684641
* Provides Decompression Sinks for `"archive/tar` package
685642
*/
686643
module ArchiveTar {
687-
class TheSink extends Range {
644+
class TheSink extends Sink {
688645
TheSink() {
689646
exists(Method f | f.hasQualifiedName("archive/tar", "Reader", "Read") |
690647
this = f.getACall().getReceiver()
691648
)
692649
}
693-
694-
override DataFlow::Node sink() { result = this }
695650
}
696651
}
697652

698653
/**
699654
* Provides Decompression Sinks for packages that use some standard IO interfaces/methods for reading decompressed data
700655
*/
701656
module GeneralReadIoSink {
702-
class TheSink extends Range {
657+
class TheSink extends Sink {
703658
TheSink() {
704659
exists(Function f | f.hasQualifiedName("io", ["Copy", "CopyBuffer", "CopyN"]) |
705660
this = f.getACall().getArgument(1)
@@ -726,8 +681,6 @@ module DecompressionBombs {
726681
this = f.getACall().getArgument(0)
727682
)
728683
}
729-
730-
override DataFlow::Node sink() { result = this }
731684
}
732685
}
733686
}

0 commit comments

Comments
 (0)