From 1923b6d18e761dd94f14defea76163698b1f35af Mon Sep 17 00:00:00 2001 From: Cosmin Cojocar Date: Wed, 18 Jul 2018 14:31:07 +0200 Subject: [PATCH] Rule which detects a potential path traversal when extracting zip archives (#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 --- .travis.yml | 1 - README.md | 1 + rules/archive.go | 60 +++++++++++++++++++++++++++++ rules/rulelist.go | 1 + rules/rules_test.go | 4 ++ testutils/source.go | 94 +++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 rules/archive.go diff --git a/.travis.yml b/.travis.yml index 99640c0..7d7958f 100644 --- 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 index 5646d9b..dff0b0e 100644 --- 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 new file mode 100644 index 0000000..1b388f5 --- /dev/null +++ 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 index f6f21af..e06b7cc 100644 --- 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 index 78a2619..4a9c910 100644 --- 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 index a0ba5bf..00a2a93 100644 --- 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{ {`