From a4d7b3628b84690bf5c37e0b395cc28a39f67ccf Mon Sep 17 00:00:00 2001 From: Hiroki Suezawa Date: Mon, 20 Jan 2020 04:40:19 +0900 Subject: [PATCH] Add G110(Potential DoS vulnerability via decompression bomb) Signed-off-by: Hiroki Suezawa --- README.md | 1 + issue.go | 1 + output/formatter_test.go | 6 +- rules/decompression-bomb.go | 111 ++++++++++++++++++++++++++++++++++++ rules/rulelist.go | 1 + rules/rules_test.go | 4 ++ testutils/source.go | 88 ++++++++++++++++++++++++++++ 7 files changed, 209 insertions(+), 3 deletions(-) create mode 100644 rules/decompression-bomb.go diff --git a/README.md b/README.md index 7e6e12e..d42f363 100644 --- a/README.md +++ b/README.md @@ -72,6 +72,7 @@ directory you can supply `./...` as the input argument. - G107: Url provided to HTTP request as taint input - G108: Profiling endpoint automatically exposed on /debug/pprof - G109: Potential Integer overflow made by strconv.Atoi result conversion to int16/32 +- G110: Potential DoS vulnerability via decompression bomb - G201: SQL query construction using format string - G202: SQL query construction using string concatenation - G203: Use of unescaped data in HTML templates diff --git a/issue.go b/issue.go index 62a4859..829bbb5 100644 --- a/issue.go +++ b/issue.go @@ -54,6 +54,7 @@ var IssueToCWE = map[string]Cwe{ "G106": GetCwe("322"), "G107": GetCwe("88"), "G109": GetCwe("190"), + "G110": GetCwe("409"), "G201": GetCwe("89"), "G202": GetCwe("89"), "G203": GetCwe("79"), diff --git a/output/formatter_test.go b/output/formatter_test.go index 46bfe62..9b1ce0f 100644 --- a/output/formatter_test.go +++ b/output/formatter_test.go @@ -251,9 +251,9 @@ var _ = Describe("Formatter", func() { Context("When using different report formats", func() { grules := []string{"G101", "G102", "G103", "G104", "G106", - "G107", "G109", "G201", "G202", "G203", "G204", - "G301", "G302", "G303", "G304", "G305", "G401", - "G402", "G403", "G404", "G501", "G502", "G503", "G504", "G505"} + "G107", "G109", "G110", "G201", "G202", "G203", "G204", + "G301", "G302", "G303", "G304", "G305", "G401", "G402", + "G403", "G404", "G501", "G502", "G503", "G504", "G505"} It("csv formatted report should contain the CWE mapping", func() { for _, rule := range grules { diff --git a/rules/decompression-bomb.go b/rules/decompression-bomb.go new file mode 100644 index 0000000..2c71be9 --- /dev/null +++ b/rules/decompression-bomb.go @@ -0,0 +1,111 @@ +// (c) Copyright 2016 Hewlett Packard Enterprise Development LP +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rules + +import ( + "fmt" + "github.com/securego/gosec" + "go/ast" +) + +type decompressionBombCheck struct { + gosec.MetaData + readerCalls gosec.CallList + copyCalls gosec.CallList +} + +func (d *decompressionBombCheck) ID() string { + return d.MetaData.ID +} + +func containsReaderCall(node ast.Node, ctx *gosec.Context, list gosec.CallList) bool { + if list.ContainsCallExpr(node, ctx, false) != nil { + return true + } + // Resolve type info of ident (for *archive/zip.File.Open) + s, idt, _ := gosec.GetCallInfo(node, ctx) + if list.Contains(s, idt) { + return true + } + return false +} + +func (d *decompressionBombCheck) Match(node ast.Node, ctx *gosec.Context) (*gosec.Issue, error) { + var readerVarObj map[*ast.Object]struct{} + + // To check multiple lines, ctx.PassedValues is used to store temporary data. + if _, ok := ctx.PassedValues[d.ID()]; !ok { + readerVarObj = make(map[*ast.Object]struct{}) + ctx.PassedValues[d.ID()] = readerVarObj + } else if pv, ok := ctx.PassedValues[d.ID()].(map[*ast.Object]struct{}); ok { + readerVarObj = pv + } else { + return nil, fmt.Errorf("PassedValues[%s] of Context is not map[*ast.Object]struct{}, but %T", d.ID(), ctx.PassedValues[d.ID()]) + } + + // io.Copy is a common function. + // To reduce false positives, This rule detects code which is used for compressed data only. + switch n := node.(type) { + case *ast.AssignStmt: + for _, expr := range n.Rhs { + if callExpr, ok := expr.(*ast.CallExpr); ok && containsReaderCall(callExpr, ctx, d.readerCalls) { + if idt, ok := n.Lhs[0].(*ast.Ident); ok && idt.Name != "_" { + // Example: + // r, _ := zlib.NewReader(buf) + // Add r's Obj to readerVarObj map + readerVarObj[idt.Obj] = struct{}{} + } + } + } + case *ast.CallExpr: + if d.copyCalls.ContainsCallExpr(n, ctx, false) != nil { + if idt, ok := n.Args[1].(*ast.Ident); ok { + if _, ok := readerVarObj[idt.Obj]; ok { + // Detect io.Copy(x, r) + return gosec.NewIssue(ctx, n, d.ID(), d.What, d.Severity, d.Confidence), nil + } + } + } + } + + return nil, nil +} + +// NewDecompressionBombCheck detects if there is potential DoS vulnerability via decompression bomb +func NewDecompressionBombCheck(id string, conf gosec.Config) (gosec.Rule, []ast.Node) { + readerCalls := gosec.NewCallList() + readerCalls.Add("compress/gzip", "NewReader") + readerCalls.AddAll("compress/zlib", "NewReader", "NewReaderDict") + readerCalls.Add("compress/bzip2", "NewReader") + readerCalls.AddAll("compress/flate", "NewReader", "NewReaderDict") + readerCalls.Add("compress/lzw", "NewReader") + readerCalls.Add("archive/tar", "NewReader") + readerCalls.Add("archive/zip", "NewReader") + readerCalls.Add("*archive/zip.File", "Open") + + copyCalls := gosec.NewCallList() + copyCalls.Add("io", "Copy") + + return &decompressionBombCheck{ + MetaData: gosec.MetaData{ + ID: id, + Severity: gosec.Medium, + Confidence: gosec.Medium, + What: "Potential DoS vulnerability via decompression bomb", + }, + readerCalls: readerCalls, + copyCalls: copyCalls, + }, []ast.Node{(*ast.FuncDecl)(nil), (*ast.AssignStmt)(nil), (*ast.CallExpr)(nil)} +} diff --git a/rules/rulelist.go b/rules/rulelist.go index d04a3bc..3835f97 100644 --- a/rules/rulelist.go +++ b/rules/rulelist.go @@ -67,6 +67,7 @@ func Generate(filters ...RuleFilter) RuleList { {"G107", "Url provided to HTTP request as taint input", NewSSRFCheck}, {"G108", "Profiling endpoint is automatically exposed", NewPprofCheck}, {"G109", "Converting strconv.Atoi result to int32/int16", NewIntegerOverflowCheck}, + {"G110", "Detect io.Copy instead of io.CopyN when decompression", NewDecompressionBombCheck}, // injection {"G201", "SQL query construction using format string", NewSQLStrFormat}, diff --git a/rules/rules_test.go b/rules/rules_test.go index 6a4cd70..a52edd5 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -87,6 +87,10 @@ var _ = Describe("gosec rules", func() { runner("G109", testutils.SampleCodeG109) }) + It("should detect DoS vulnerability via decompression bomb", func() { + runner("G110", testutils.SampleCodeG110) + }) + It("should detect sql injection via format strings", func() { runner("G201", testutils.SampleCodeG201) }) diff --git a/testutils/source.go b/testutils/source.go index 4897f2a..7c58da3 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -594,6 +594,94 @@ func test() { fmt.Println(value) }`}, 0, gosec.NewConfig()}} + // SampleCodeG110 - potential DoS vulnerability via decompression bomb + SampleCodeG110 = []CodeSample{ + {[]string{` +package main + +import ( + "bytes" + "compress/zlib" + "io" + "os" +) + +func main() { + buff := []byte{120, 156, 202, 72, 205, 201, 201, 215, 81, 40, 207, + 47, 202, 73, 225, 2, 4, 0, 0, 255, 255, 33, 231, 4, 147} + b := bytes.NewReader(buff) + + r, err := zlib.NewReader(b) + if err != nil { + panic(err) + } + io.Copy(os.Stdout, r) + + r.Close() +}`}, 1, gosec.NewConfig()}, {[]string{` +package main + +import ( + "archive/zip" + "io" + "os" + "strconv" +) + +func main() { + r, err := zip.OpenReader("tmp.zip") + if err != nil { + panic(err) + } + defer r.Close() + + for i, f := range r.File { + out, err := os.OpenFile("output" + strconv.Itoa(i), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode()) + if err != nil { + panic(err) + } + + rc, err := f.Open() + if err != nil { + panic(err) + } + + _, err = io.Copy(out, rc) + + out.Close() + rc.Close() + + if err != nil { + panic(err) + } + } +}`}, 1, gosec.NewConfig()}, {[]string{` +package main + +import ( + "io" + "os" +) + +func main() { + s, err := os.Open("src") + if err != nil { + panic(err) + } + defer s.Close() + + d, err := os.Create("dst") + if err != nil { + panic(err) + } + defer d.Close() + + _, err = io.Copy(d, s) + if err != nil { + panic(err) + } +}`}, 0, gosec.NewConfig()}} + // SampleCodeG201 - SQL injection via format string SampleCodeG201 = []CodeSample{ {[]string{`