From e76b2584560843c37643928736c82c3b0413a22a Mon Sep 17 00:00:00 2001 From: coredefend Date: Thu, 8 Mar 2018 18:23:27 -0500 Subject: [PATCH] New Rule Tainted file (#183) * Add a tool to generate the TLS configuration form Mozilla's ciphers recommendation (#178) * Add a tool which generates the TLS rule configuration from Mozilla server side TLS configuration * Update README * Remove trailing space in README * Update dependencies * Fix the commends of the generated functions * Add nil pointer check to rule. (#181) TypeOf returns the type of expression e, or nil if not found. We are calling .String() on a value that may be nil in this clause. Relates to #174 * Add support for YAML output format (#177) * Add YAML output format * Update README * added rule to check for tainted file path * added #nosec to main/issue.go * updated test case import --- README.md | 1 + cmd/gas/main.go | 1 + issue.go | 1 + rules/readfile.go | 49 +++++++++++++++++++++++++++++++++++++++++++++ rules/rulelist.go | 1 + rules/rules_test.go | 4 ++++ testutils/source.go | 43 +++++++++++++++++++++++++++++++++++++++ 7 files changed, 100 insertions(+) create mode 100644 rules/readfile.go diff --git a/README.md b/README.md index d2c3bbc..59cec7a 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,7 @@ or to specify a set of rules to explicitly exclude using the '-exclude=' flag. - G301: Poor file permissions used when creating a directory - G302: Poor file permisions used with chmod - G303: Creating tempfile using a predictable path + - G304: File path provided as taint input - G401: Detect the usage of DES, RC4, or MD5 - G402: Look for bad TLS connection settings - G403: Ensure minimum RSA key length of 2048 bits diff --git a/cmd/gas/main.go b/cmd/gas/main.go index a04c151..4e560ca 100644 --- a/cmd/gas/main.go +++ b/cmd/gas/main.go @@ -110,6 +110,7 @@ func usage() { func loadConfig(configFile string) (gas.Config, error) { config := gas.NewConfig() if configFile != "" { + // #nosec file, err := os.Open(configFile) if err != nil { return nil, err diff --git a/issue.go b/issue.go index 2113529..75bb157 100644 --- a/issue.go +++ b/issue.go @@ -97,6 +97,7 @@ func NewIssue(ctx *Context, node ast.Node, desc string, severity Score, confiden line = fmt.Sprintf("%d-%d", start, end) } + // #nosec if file, err := os.Open(fobj.Name()); err == nil { defer file.Close() s := (int64)(fobj.Position(node.Pos()).Offset) // Go bug, should be int64 diff --git a/rules/readfile.go b/rules/readfile.go new file mode 100644 index 0000000..7c7117f --- /dev/null +++ b/rules/readfile.go @@ -0,0 +1,49 @@ +// (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 ( + "go/ast" + "go/types" + + "github.com/GoASTScanner/gas" +) + +type readfile struct { + gas.CallList +} + +// Match inspects AST nodes to determine if the match the methods `os.Open` or `ioutil.ReadFile` +func (r *readfile) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) { + if node := r.ContainsCallExpr(n, c); node != nil { + for _, arg := range node.Args { + if ident, ok := arg.(*ast.Ident); ok { + obj := c.Info.ObjectOf(ident) + if _, ok := obj.(*types.Var); ok && !gas.TryResolve(ident, c) { + return gas.NewIssue(c, n, "File inclusion launched with variable", gas.Medium, gas.High), nil + } + } + } + } + return nil, nil +} + +// NewReadFile detects cases where we read files +func NewReadFile(conf gas.Config) (gas.Rule, []ast.Node) { + rule := &readfile{gas.NewCallList()} + rule.Add("io/ioutil", "ReadFile") + rule.Add("os", "Open") + return rule, []ast.Node{(*ast.CallExpr)(nil)} +} diff --git a/rules/rulelist.go b/rules/rulelist.go index 2846368..01d567f 100644 --- a/rules/rulelist.go +++ b/rules/rulelist.go @@ -77,6 +77,7 @@ func Generate(filters ...RuleFilter) RuleList { "G301": {"Poor file permissions used when creating a directory", NewMkdirPerms}, "G302": {"Poor file permisions used when creation file or using chmod", NewFilePerms}, "G303": {"Creating tempfile using a predictable path", NewBadTempFile}, + "G304": {"File path provided as taint input", NewReadFile}, // crypto "G401": {"Detect the usage of DES, RC4, or MD5", NewUsesWeakCryptography}, diff --git a/rules/rules_test.go b/rules/rules_test.go index a7dca95..cde47e6 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -97,6 +97,10 @@ var _ = Describe("gas rules", func() { runner("G303", testutils.SampleCodeG303) }) + It("should detect file path provided as taint input", func() { + runner("G304", testutils.SampleCodeG304) + }) + It("should detect weak crypto algorithms", func() { runner("G401", testutils.SampleCodeG401) }) diff --git a/testutils/source.go b/testutils/source.go index eb4a826..2fc9bb0 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -459,6 +459,49 @@ func main() { ioutil.WriteFile("/tmp/demo2", []byte("This is some data"), 0644) }`, 2}} + // SampleCodeG304 - potential file inclusion vulnerability + SampleCodeG304 = []CodeSample{{` +package main +import ( +"os" +"io/ioutil" +"log" +) +func main() { +f := os.Getenv("tainted_file") +body, err := ioutil.ReadFile(f) +if err != nil { + log.Printf("Error: %v\n", err) +} +log.Print(f) + +}`, 1}, {` +package main + +import ( + "fmt" + "log" + "net/http" + "os" +) + +func main() { + http.HandleFunc("/bar", func(w http.ResponseWriter, r *http.Request) { + title := r.URL.Query().Get("title") + f, err := os.Open(title) + if err != nil { + fmt.Printf("Error: %v\n", err) + } + body := make([]byte, 5) + n1, err := f.Read(body) + if err != nil { + fmt.Printf("Error: %v\n", err) + } + fmt.Fprintf(w, "%s", body) + }) + log.Fatal(http.ListenAndServe(":3000", nil)) +}`, 1}} + // SampleCodeG401 - Use of weak crypto MD5 SampleCodeG401 = []CodeSample{ {`