From 9c19cb6501e3b118b2619496cab2397470279819 Mon Sep 17 00:00:00 2001 From: Vladimir Severov Date: Fri, 3 Jun 2022 01:19:51 +0300 Subject: [PATCH] Add check for usage of Rat.SetString in math/big with an overflow error (#819) * Add check for usage of Rat.SetString in math/big with an overflow error Rat.SetString in math/big in Go before 1.16.14 and 1.17.x before 1.17.7 has an overflow that can lead to Uncontrolled Memory Consumption. It is the CVE-2022-23772. * Use ContainsPkgCallExpr instead of manual parsing --- README.md | 1 + call_list.go | 25 +++++++++++++++-------- helpers.go | 9 ++++++++ issue.go | 3 ++- report/formatter_test.go | 7 ++++--- rules/math_big_rat.go | 44 ++++++++++++++++++++++++++++++++++++++++ rules/rulelist.go | 1 + rules/rules_test.go | 4 ++++ testutils/source.go | 20 ++++++++++++++++++ 9 files changed, 102 insertions(+), 12 deletions(-) create mode 100644 rules/math_big_rat.go diff --git a/README.md b/README.md index 337477d..d174c19 100644 --- a/README.md +++ b/README.md @@ -145,6 +145,7 @@ directory you can supply `./...` as the input argument. - G110: Potential DoS vulnerability via decompression bomb - G111: Potential directory traversal - G112: Potential slowloris attack +- G113: Usage of Rat.SetString in math/big with an overflow (CVE-2022-23772) - 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/call_list.go b/call_list.go index 4b3fcf0..4f2d6c5 100644 --- a/call_list.go +++ b/call_list.go @@ -47,7 +47,7 @@ func (c CallList) Add(selector, ident string) { } // Contains returns true if the package and function are -/// members of this call list. +// members of this call list. func (c CallList) Contains(selector, ident string) bool { if idents, ok := c[selector]; ok { _, found := idents[ident] @@ -77,17 +77,26 @@ func (c CallList) ContainsPkgCallExpr(n ast.Node, ctx *Context, stripVendor bool return nil } - // Use only explicit path (optionally strip vendor path prefix) to reduce conflicts - path, ok := GetImportPath(selector, ctx) - if !ok { - return nil + // Selector can have two forms: + // 1. A short name if a module function is called (expr.Name). + // E.g., "big" if called function from math/big. + // 2. A full name if a structure function is called (TypeOf(expr)). + // E.g., "math/big.Rat" if called function of Rat structure from math/big. + if !strings.ContainsRune(selector, '.') { + // Use only explicit path (optionally strip vendor path prefix) to reduce conflicts + path, ok := GetImportPath(selector, ctx) + if !ok { + return nil + } + selector = path } + if stripVendor { - if vendorIdx := strings.Index(path, vendorPath); vendorIdx >= 0 { - path = path[vendorIdx+len(vendorPath):] + if vendorIdx := strings.Index(selector, vendorPath); vendorIdx >= 0 { + selector = selector[vendorIdx+len(vendorPath):] } } - if !c.Contains(path, ident) { + if !c.Contains(selector, ident) { return nil } diff --git a/helpers.go b/helpers.go index e5f2218..7ac1f2c 100644 --- a/helpers.go +++ b/helpers.go @@ -449,3 +449,12 @@ func RootPath(root string) (string, error) { root = strings.TrimSuffix(root, "...") return filepath.Abs(root) } + +// GoVersion returns parsed version of Go from runtime +func GoVersion() (int, int, int) { + versionParts := strings.Split(runtime.Version(), ".") + major, _ := strconv.Atoi(versionParts[0][2:]) + minor, _ := strconv.Atoi(versionParts[1]) + build, _ := strconv.Atoi(versionParts[2]) + return major, minor, build +} diff --git a/issue.go b/issue.go index 832ab26..32b9bc0 100644 --- a/issue.go +++ b/issue.go @@ -65,6 +65,7 @@ var ruleToCWE = map[string]string{ "G110": "409", "G111": "22", "G112": "400", + "G113": "190", "G201": "89", "G202": "89", "G203": "79", @@ -182,7 +183,7 @@ func NewIssue(ctx *Context, node ast.Node, ruleID, desc string, severity Score, var code string if file, err := os.Open(fobj.Name()); err == nil { - defer file.Close() //#nosec + defer file.Close() // #nosec s := codeSnippetStartLine(node, fobj) e := codeSnippetEndLine(node, fobj) code, err = codeSnippet(file, s, e, node) diff --git a/report/formatter_test.go b/report/formatter_test.go index 66496d8..23e2775 100644 --- a/report/formatter_test.go +++ b/report/formatter_test.go @@ -277,9 +277,10 @@ var _ = Describe("Formatter", func() { Context("When using different report formats", func() { grules := []string{ "G101", "G102", "G103", "G104", "G106", "G107", "G109", - "G110", "G111", "G112", "G201", "G202", "G203", "G204", "G301", - "G302", "G303", "G304", "G305", "G401", "G402", "G403", - "G404", "G501", "G502", "G503", "G504", "G505", + "G110", "G111", "G112", "G113", "G201", "G202", "G203", + "G204", "G301", "G302", "G303", "G304", "G305", "G401", + "G402", "G403", "G404", "G501", "G502", "G503", "G504", + "G505", "G601", } It("csv formatted report should contain the CWE mapping", func() { diff --git a/rules/math_big_rat.go b/rules/math_big_rat.go new file mode 100644 index 0000000..69037e1 --- /dev/null +++ b/rules/math_big_rat.go @@ -0,0 +1,44 @@ +package rules + +import ( + "go/ast" + + "github.com/securego/gosec/v2" +) + +type usingOldMathBig struct { + gosec.MetaData + calls gosec.CallList +} + +func (r *usingOldMathBig) ID() string { + return r.MetaData.ID +} + +func (r *usingOldMathBig) Match(node ast.Node, ctx *gosec.Context) (gi *gosec.Issue, err error) { + if callExpr := r.calls.ContainsPkgCallExpr(node, ctx, false); callExpr == nil { + return nil, nil + } + + confidence := gosec.Low + major, minor, build := gosec.GoVersion() + if major == 1 && (minor == 16 && build < 14 || minor == 17 && build < 7) { + confidence = gosec.Medium + } + + return gosec.NewIssue(ctx, node, r.ID(), r.What, r.Severity, confidence), nil +} + +// NewUsingOldMathBig rule detects the use of Rat.SetString from math/big. +func NewUsingOldMathBig(id string, _ gosec.Config) (gosec.Rule, []ast.Node) { + calls := gosec.NewCallList() + calls.Add("math/big.Rat", "SetString") + return &usingOldMathBig{ + calls: calls, + MetaData: gosec.MetaData{ + ID: id, + What: "Potential uncontrolled memory consumption in Rat.SetString (CVE-2022-23772)", + Severity: gosec.High, + }, + }, []ast.Node{(*ast.CallExpr)(nil)} +} diff --git a/rules/rulelist.go b/rules/rulelist.go index 01c0e51..af8ea0d 100644 --- a/rules/rulelist.go +++ b/rules/rulelist.go @@ -75,6 +75,7 @@ func Generate(trackSuppressions bool, filters ...RuleFilter) RuleList { {"G110", "Detect io.Copy instead of io.CopyN when decompression", NewDecompressionBombCheck}, {"G111", "Detect http.Dir('/') as a potential risk", NewDirectoryTraversal}, {"G112", "Detect ReadHeaderTimeout not configured as a potential risk", NewSlowloris}, + {"G113", "Usage of Rat.SetString in math/big with an overflow", NewUsingOldMathBig}, // injection {"G201", "SQL query construction using format string", NewSQLStrFormat}, diff --git a/rules/rules_test.go b/rules/rules_test.go index d10bd3d..abca817 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -98,6 +98,10 @@ var _ = Describe("gosec rules", func() { runner("G112", testutils.SampleCodeG112) }) + It("should detect potential uncontrolled memory consumption in Rat.SetString", func() { + runner("G113", testutils.SampleCodeG113) + }) + It("should detect sql injection via format strings", func() { runner("G201", testutils.SampleCodeG201) }) diff --git a/testutils/source.go b/testutils/source.go index ebb9fd0..e6a1709 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -1052,6 +1052,26 @@ func HelloServer(w http.ResponseWriter, r *http.Request) { `}, 0, gosec.NewConfig()}, } + // SampleCodeG113 - Usage of Rat.SetString in math/big with an overflow + SampleCodeG113 = []CodeSample{ + {[]string{ + ` +package main + +import ( + "math/big" + "fmt" +) + +func main() { + r := big.Rat{} + r.SetString("13e-9223372036854775808") + + fmt.Println(r) +}`, + }, 1, gosec.NewConfig()}, + } + // SampleCodeG201 - SQL injection via format string SampleCodeG201 = []CodeSample{ {[]string{`