From 70998b75b79447ca6a9f6c3b2cc8ab1092f860c2 Mon Sep 17 00:00:00 2001 From: Kent 'picat' Gruber Date: Fri, 4 Aug 2023 18:19:04 -0400 Subject: [PATCH] Add log injection analysis (#5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add log injection analysis * Improve callgraph construction * Add `(callgraph.Path).String` method * Use `→` for `(callgraph.Edge).String` * Add `(*callgraph.Graph).String` method * Do NOT assume so much about anonymous functions --- callgraph/callgraph.go | 76 ++++++++++++++--- cmd/logi/main.go | 10 +++ log/injection/injection.go | 122 +++++++++++++++++++++++++++ log/injection/injection_test.go | 21 +++++ log/injection/testdata/src/a/main.go | 14 +++ log/injection/testdata/src/b/main.go | 25 ++++++ log/injection/testdata/src/c/main.go | 28 ++++++ 7 files changed, 283 insertions(+), 13 deletions(-) create mode 100644 cmd/logi/main.go create mode 100644 log/injection/injection.go create mode 100644 log/injection/injection_test.go create mode 100644 log/injection/testdata/src/a/main.go create mode 100644 log/injection/testdata/src/b/main.go create mode 100644 log/injection/testdata/src/c/main.go diff --git a/callgraph/callgraph.go b/callgraph/callgraph.go index c4b7b95..10dc929 100644 --- a/callgraph/callgraph.go +++ b/callgraph/callgraph.go @@ -1,6 +1,7 @@ package callgraph import ( + "bytes" "context" "fmt" "go/token" @@ -85,30 +86,48 @@ func New(root *ssa.Function, srcFns ...*ssa.Function) (*Graph, error) { switch instrt := instr.(type) { case *ssa.Call: // debugf("found call instr") - fn, ok := instrt.Call.Value.(*ssa.Function) - if ok { - err := g.AddFunction(fn, allFns) - if err != nil { - return fmt.Errorf("failed to add src function %v from block instr: %w", fn, err) + var instrCall *ssa.Function + + // Handle the case where the function calls a + // named function (*ssa.Function), and the case + // where the function calls an anonymous + // function (*ssa.MakeClosure). + switch callt := instrt.Call.Value.(type) { + case *ssa.Function: + // debugf("found call instr to function") + instrCall = callt + case *ssa.MakeClosure: + // debugf("found call instr to closure") + switch calltFn := callt.Fn.(type) { + case *ssa.Function: + instrCall = calltFn } } + // If we could not determine the function being + // called, skip this instruction. + if instrCall == nil { + continue + } + + AddEdge(g.CreateNode(fn), instrt, g.CreateNode(instrCall)) + + err := g.AddFunction(instrCall, allFns) + if err != nil { + return fmt.Errorf("failed to add function %v from block instr: %w", instrCall, err) + } + // attempt to link function arguments that are functions for a := 0; a < len(instrt.Call.Args); a++ { arg := instrt.Call.Args[a] switch argt := arg.(type) { case *ssa.Function: // TODO: check if edge already exists? - AddEdge(g.CreateNode(fn), instrt, g.CreateNode(argt)) + AddEdge(g.CreateNode(instrCall), instrt, g.CreateNode(argt)) case *ssa.MakeClosure: switch argtFn := argt.Fn.(type) { case *ssa.Function: - AddEdge(g.CreateNode(fn), instrt, g.CreateNode(argtFn)) - - // Assumes the anonymous functions are called. - for _, anFn := range argtFn.AnonFuncs { - AddEdge(g.CreateNode(fn), instrt, g.CreateNode(anFn)) - } + AddEdge(g.CreateNode(instrCall), instrt, g.CreateNode(argtFn)) } } } @@ -208,6 +227,19 @@ func (g *Graph) CreateNode(fn *ssa.Function) *Node { return n } +func (g *Graph) String() string { + var buf bytes.Buffer + + for _, n := range g.Nodes { + fmt.Fprintf(&buf, "%s\n", n) + for _, e := range n.Out { + fmt.Fprintf(&buf, "\t→ %s\n", e.Callee) + } + fmt.Fprintf(&buf, "\n") + } + return buf.String() +} + // A Node represents a node in a call graph. type Node struct { sync.RWMutex @@ -232,7 +264,7 @@ type Edge struct { } func (e Edge) String() string { - return fmt.Sprintf("%s --> %s", e.Caller, e.Callee) + return fmt.Sprintf("%s → %s", e.Caller, e.Callee) } func (e Edge) Description() string { @@ -343,6 +375,24 @@ func (p Path) Last() *Edge { return p[len(p)-1] } +// String returns a string representation of the path which +// is a sequence of edges separated by " → ". +// +// Intended to be used while debugging. +func (p Path) String() string { + var buf bytes.Buffer + for i, e := range p { + if i == 0 { + buf.WriteString(e.Caller.String()) + } + + buf.WriteString(" → ") + + buf.WriteString(e.Callee.String()) + } + return buf.String() +} + type Paths []Path func PathSearch(start *Node, isEnd func(*Node) bool) Path { diff --git a/cmd/logi/main.go b/cmd/logi/main.go new file mode 100644 index 0000000..4d0f677 --- /dev/null +++ b/cmd/logi/main.go @@ -0,0 +1,10 @@ +package main + +import ( + "github.com/picatz/taint/log/injection" + "golang.org/x/tools/go/analysis/singlechecker" +) + +func main() { + singlechecker.Main(injection.Analyzer) +} diff --git a/log/injection/injection.go b/log/injection/injection.go new file mode 100644 index 0000000..feea5f8 --- /dev/null +++ b/log/injection/injection.go @@ -0,0 +1,122 @@ +package injection + +import ( + "fmt" + "strings" + + "github.com/picatz/taint" + "github.com/picatz/taint/callgraph" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/buildssa" +) + +var userControlledValues = taint.NewSources( + "*net/http.Request", +) + +var injectableLogFunctions = taint.NewSinks( + // Note: at this time, they *must* be a function or method. + "log.Fatal", + "log.Fatalf", + "log.Fatalln", + "log.Panic", + "log.Panicf", + "log.Panicln", + "log.Print", + "log.Printf", + "log.Println", + "log.Output", + "log.SetOutput", + "log.SetPrefix", + "log.Writer", + "(*log.Logger).Fatal", + "(*log.Logger).Fatalf", + "(*log.Logger).Fatalln", + "(*log.Logger).Panic", + "(*log.Logger).Panicf", + "(*log.Logger).Panicln", + "(*log.Logger).Print", + "(*log.Logger).Printf", + "(*log.Logger).Println", + "(*log.Logger).Output", + "(*log.Logger).SetOutput", + "(*log.Logger).SetPrefix", + "(*log.Logger).Writer", + // TODO: consider adding the following logger packages, + // and the ability to configure this list generically. + // + // https://pkg.go.dev/golang.org/x/exp/slog + // https://pkg.go.dev/github.com/golang/glog + // https://pkg.go.dev/github.com/hashicorp/go-hclog + // https://pkg.go.dev/github.com/sirupsen/logrus + // https://pkg.go.dev/go.uber.org/zap + // ... +) + +// Analyzer finds potential log injection issues to demonstrate +// the github.com/picatz/taint package. +var Analyzer = &analysis.Analyzer{ + Name: "logi", + Doc: "finds potential log injection issues", + Run: run, + Requires: []*analysis.Analyzer{buildssa.Analyzer}, +} + +// imports returns true if the package imports any of the given packages. +func imports(pass *analysis.Pass, pkgs ...string) bool { + var imported bool + for _, imp := range pass.Pkg.Imports() { + for _, pkg := range pkgs { + if strings.HasSuffix(imp.Path(), pkg) { + imported = true + break + } + } + if imported { + break + } + } + return imported +} + +func run(pass *analysis.Pass) (interface{}, error) { + // Require the log package is imported in the + // program being analyzed before running the analysis. + // + // This prevents wasting time analyzing programs that don't log. + if !imports(pass, "log") { + return nil, nil + } + + // Get the built SSA IR. + buildSSA := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA) + + // Identify the main function from the package's SSA IR. + mainFn := buildSSA.Pkg.Func("main") + if mainFn == nil { + return nil, nil + } + + // Construct a callgraph, using the main function as the root, + // constructed of all other functions. This returns a callgraph + // we can use to identify directed paths to logging functions. + cg, err := callgraph.New(mainFn, buildSSA.SrcFuncs...) + if err != nil { + return nil, fmt.Errorf("failed to create new callgraph: %w", err) + } + + // Run taint check for user controlled values (sources) ending + // up in injectable log functions (sinks). + results := taint.Check(cg, userControlledValues, injectableLogFunctions) + + // For each result, check if a prepared statement is providing + // a mitigation for the user controlled value. + // + // TODO: ensure this makes sense for all the GORM usage? + for _, result := range results { + pass.Reportf(result.SinkValue.Pos(), "potential log injection") + } + + return nil, nil +} diff --git a/log/injection/injection_test.go b/log/injection/injection_test.go new file mode 100644 index 0000000..45078cf --- /dev/null +++ b/log/injection/injection_test.go @@ -0,0 +1,21 @@ +package injection + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +var testdata = analysistest.TestData() + +func TestA(t *testing.T) { + analysistest.Run(t, testdata, Analyzer, "a") +} + +func TestB(t *testing.T) { + analysistest.Run(t, testdata, Analyzer, "b") +} + +func TestC(t *testing.T) { + analysistest.Run(t, testdata, Analyzer, "c") +} diff --git a/log/injection/testdata/src/a/main.go b/log/injection/testdata/src/a/main.go new file mode 100644 index 0000000..e0909bb --- /dev/null +++ b/log/injection/testdata/src/a/main.go @@ -0,0 +1,14 @@ +package main + +import ( + "log" + "net/http" +) + +func main() { + http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + log.Println(r.URL.Query().Get("input")) // want "potential log injection" + }) + + http.ListenAndServe(":8080", nil) +} diff --git a/log/injection/testdata/src/b/main.go b/log/injection/testdata/src/b/main.go new file mode 100644 index 0000000..2635963 --- /dev/null +++ b/log/injection/testdata/src/b/main.go @@ -0,0 +1,25 @@ +package main + +import ( + "log" + "net/http" +) + +func l(input string) { + l := log.New(nil, "", 0) + l.Println(input) // want "potential log injection" +} + +func buisness(input string) { + l(input) +} + +func main() { + http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + input := r.URL.Query().Get("input") + + buisness(input) + }) + + http.ListenAndServe(":8080", nil) +} diff --git a/log/injection/testdata/src/c/main.go b/log/injection/testdata/src/c/main.go new file mode 100644 index 0000000..ffd972d --- /dev/null +++ b/log/injection/testdata/src/c/main.go @@ -0,0 +1,28 @@ +package main + +import ( + "log" + "net/http" +) + +func l(input string) { + log.Println(input) // want "potential log injection" +} + +func buisness(input string) { + l(input) +} + +func main() { + http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + input := r.URL.Query().Get("input") + + f := func() { + buisness(input) + } + + f() + }) + + http.ListenAndServe(":8080", nil) +}