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
securego · Mar 8, 2018 · e76b258 · e76b258
1 parent 57dd25a
commit e76b258
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 0 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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{
 		{`