Rule which detects a potential path traversal when extracting zip arc…

…hives (securego#208) * Add a rule which detects file path traversal when extracting zip archive * Detect if any argument is derived from zip.File * Drop support for Go version 1.8
holyspectral · Jul 18, 2018 · 1923b6d · 1923b6d
1 parent 4ae8c95
commit 1923b6d
Show file tree

Hide file tree

Showing 6 changed files with 160 additions and 1 deletion.
diff --git a/.travis.yml b/.travis.yml
@@ -1,7 +1,6 @@
 language: go
 
 go:
-  - 1.8
   - 1.9
   - "1.10"
   - tip

diff --git a/README.md b/README.md
@@ -50,6 +50,7 @@ or to specify a set of rules to explicitly exclude using the '-exclude=' flag.
   - G302: Poor file permisions used with chmod
   - G303: Creating tempfile using a predictable path
   - G304: File path provided as taint input
+  - G305: File traversal when extracting zip archive
   - 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/rules/archive.go b/rules/archive.go
@@ -0,0 +1,60 @@
+package rules
+
+import (
+	"go/ast"
+	"go/types"
+
+	"github.com/GoASTScanner/gas"
+)
+
+type archive struct {
+	gas.MetaData
+	calls   gas.CallList
+	argType string
+}
+
+func (a *archive) ID() string {
+	return a.MetaData.ID
+}
+
+// Match inspects AST nodes to determine if the filepath.Joins uses any argument derived from type zip.File
+func (a *archive) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
+	if node := a.calls.ContainsCallExpr(n, c); node != nil {
+		for _, arg := range node.Args {
+			var argType types.Type
+			if selector, ok := arg.(*ast.SelectorExpr); ok {
+				argType = c.Info.TypeOf(selector.X)
+			} else if ident, ok := arg.(*ast.Ident); ok {
+				if ident.Obj != nil && ident.Obj.Kind == ast.Var {
+					decl := ident.Obj.Decl
+					if assign, ok := decl.(*ast.AssignStmt); ok {
+						if selector, ok := assign.Rhs[0].(*ast.SelectorExpr); ok {
+							argType = c.Info.TypeOf(selector.X)
+						}
+					}
+				}
+			}
+
+			if argType != nil && argType.String() == a.argType {
+				return gas.NewIssue(c, n, a.ID(), a.What, a.Severity, a.Confidence), nil
+			}
+		}
+	}
+	return nil, nil
+}
+
+// NewArchive creates a new rule which detects the file traversal when extracting zip archives
+func NewArchive(id string, conf gas.Config) (gas.Rule, []ast.Node) {
+	calls := gas.NewCallList()
+	calls.Add("path/filepath", "Join")
+	return &archive{
+		calls:   calls,
+		argType: "*archive/zip.File",
+		MetaData: gas.MetaData{
+			ID:         id,
+			Severity:   gas.Medium,
+			Confidence: gas.High,
+			What:       "File traversal when extracting zip archive",
+		},
+	}, []ast.Node{(*ast.CallExpr)(nil)}
+}
diff --git a/rules/rulelist.go b/rules/rulelist.go
@@ -79,6 +79,7 @@ func Generate(filters ...RuleFilter) RuleList {
 		{"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},
+		{"G305", "File path traversal when extracting zip archive", NewArchive},
 
 		// crypto
 		{"G401", "Detect the usage of DES, RC4, or MD5", NewUsesWeakCryptography},

diff --git a/rules/rules_test.go b/rules/rules_test.go
@@ -103,6 +103,10 @@ var _ = Describe("gas rules", func() {
 			runner("G304", testutils.SampleCodeG304)
 		})
 
+		It("should detect file path traversal when extracting zip archive", func() {
+			runner("G305", testutils.SampleCodeG305)
+		})
+
 		It("should detect weak crypto algorithms", func() {
 			runner("G401", testutils.SampleCodeG401)
 		})

diff --git a/testutils/source.go b/testutils/source.go
@@ -501,6 +501,100 @@ func main() {
 	log.Fatal(http.ListenAndServe(":3000", nil))
 }`, 1}}
 
+	// SampleCodeG305 - File path traversal when extracting zip archives
+	SampleCodeG305 = []CodeSample{{`
+package unzip
+
+import (
+	"archive/zip"
+	"io"
+	"os"
+	"path/filepath"
+)
+
+func unzip(archive, target string) error {
+	reader, err := zip.OpenReader(archive)
+	if err != nil {
+		return err
+	}
+
+	if err := os.MkdirAll(target, 0750); err != nil {
+		return err
+	}
+
+	for _, file := range reader.File {
+		path := filepath.Join(target, file.Name)
+		if file.FileInfo().IsDir() {
+			os.MkdirAll(path, file.Mode()) // #nosec
+			continue
+		}
+
+		fileReader, err := file.Open()
+		if err != nil {
+			return err
+		}
+		defer fileReader.Close()
+
+		targetFile, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, file.Mode())
+		if err != nil {
+			return err
+		}
+		defer targetFile.Close()
+
+		if _, err := io.Copy(targetFile, fileReader); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}`, 1}, {`
+package unzip
+
+import (
+	"archive/zip"
+	"io"
+	"os"
+	"path/filepath"
+)
+
+func unzip(archive, target string) error {
+	reader, err := zip.OpenReader(archive)
+	if err != nil {
+		return err
+	}
+
+	if err := os.MkdirAll(target, 0750); err != nil {
+		return err
+	}
+
+	for _, file := range reader.File {
+                archiveFile := file.Name
+		path := filepath.Join(target, archiveFile)
+		if file.FileInfo().IsDir() {
+			os.MkdirAll(path, file.Mode()) // #nosec
+			continue
+		}
+
+		fileReader, err := file.Open()
+		if err != nil {
+			return err
+		}
+		defer fileReader.Close()
+
+		targetFile, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, file.Mode())
+		if err != nil {
+			return err
+		}
+		defer targetFile.Close()
+
+		if _, err := io.Copy(targetFile, fileReader); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}`, 1}}
+
 	// SampleCodeG401 - Use of weak crypto MD5
 	SampleCodeG401 = []CodeSample{
 		{`
-Original file line number
+Diff line change
@@ -1,7 +1,6 @@
     language: go
     go:
-      - 1.8
       - 1.9
       - "1.10"
       - tip
@@ Expand Down @@