From 564c28f4eba5d994aed699688d7653a5e99df605 Mon Sep 17 00:00:00 2001 From: Victor Nicolet Date: Fri, 4 Oct 2024 13:45:26 -0400 Subject: [PATCH] Adding warning for usage of unsound features when a function is being summarized. --- analysis/dataflow/builtins.go | 4 +- analysis/dataflow/inter_procedural.go | 2 +- analysis/dataflow/intra_procedural.go | 2 + analysis/dataflow/report.go | 102 ++++++++++++++++++ .../testdata/unsound-features/config.yaml | 15 +++ .../testdata/unsound-features/main.go | 98 +++++++++++++++++ analysis/dataflow/unsound_features_test.go | 66 ++++++++++++ analysis/summaries/standard_library.go | 12 ++- analysis/taint/testdata/basic/config.yaml | 3 + analysis/taint/testdata/basic/fields.go | 42 +++++++- analysis/taint/testdata/basic/main.go | 1 + cmd/argot/cli/defs.go | 5 +- 12 files changed, 346 insertions(+), 6 deletions(-) create mode 100644 analysis/dataflow/testdata/unsound-features/config.yaml create mode 100644 analysis/dataflow/testdata/unsound-features/main.go create mode 100644 analysis/dataflow/unsound_features_test.go diff --git a/analysis/dataflow/builtins.go b/analysis/dataflow/builtins.go index 51d0afef..04118ab4 100644 --- a/analysis/dataflow/builtins.go +++ b/analysis/dataflow/builtins.go @@ -121,8 +121,8 @@ func doBuiltinCall(t *IntraAnalysisState, callValue ssa.Value, callCommon *ssa.C // for recover, we will need some form of panic analysis case "recover": - t.parentAnalyzerState.Logger.Warnf("Encountered recover at %s, the analysis may be unsound.\n", - instruction.Parent().Prog.Fset.Position(instruction.Pos())) + // TODO: handler recovers. Analysis is unsound with recovers. + // NOTE: Unsoundness is reported by reportUnsoundFeatures return true default: // Special case: the call to Error() of the builtin error interface diff --git a/analysis/dataflow/inter_procedural.go b/analysis/dataflow/inter_procedural.go index 51c14e61..76e26c34 100644 --- a/analysis/dataflow/inter_procedural.go +++ b/analysis/dataflow/inter_procedural.go @@ -150,7 +150,7 @@ func (g *InterProceduralFlowGraph) BuildGraph() { externalContractSummary := g.AnalyzerState.LoadExternalContractSummary(node) if externalContractSummary != nil { logger.Debugf("Loaded %s from external contracts.\n", - formatutil.SanitizeRepr(node.CallSite().Common())) + formatutil.SanitizeRepr(node.Callee())) g.Summaries[node.Callee()] = externalContractSummary node.CalleeSummary = externalContractSummary if x := externalContractSummary.Callsites[node.CallSite()]; x == nil { diff --git a/analysis/dataflow/intra_procedural.go b/analysis/dataflow/intra_procedural.go index 342c1f89..e92bf2ea 100644 --- a/analysis/dataflow/intra_procedural.go +++ b/analysis/dataflow/intra_procedural.go @@ -106,6 +106,8 @@ func RunIntraProcedural(a *AnalyzerState, sm *SummaryGraph) (time.Duration, erro postBlockCallback: sm.postBlockCallBack, } + reportUnsoundFeatures(a, sm.Parent) + // Output warning if defer stack is unbounded if !state.deferStacks.DeferStackBounded { a.Logger.Warnf("Defer stack unbounded in %s: %s", diff --git a/analysis/dataflow/report.go b/analysis/dataflow/report.go index 4ca98022..73822e87 100644 --- a/analysis/dataflow/report.go +++ b/analysis/dataflow/report.go @@ -16,9 +16,12 @@ package dataflow import ( "fmt" + "go/token" + "strings" "github.com/awslabs/ar-go-tools/analysis/lang" "github.com/awslabs/ar-go-tools/internal/formatutil" + "golang.org/x/tools/go/ssa" ) // ReportMissingOrNotConstructedSummary prints a missing summary message to the cache's logger. @@ -106,3 +109,102 @@ func (s *AnalyzerState) ReportSummaryNotConstructed(callSite *CallNode) { } } } + +type unsoundFeaturesMap struct { + Recovers map[token.Position]bool + UnsafeUsages map[token.Position]string + ReflectUsages map[token.Position]string +} + +// reportUnsoundFeatures logs warning messages when unsound features are used in a function. Those include: +// +// - call to recover builtin +// +// - unsafe +// +// - reflect +// +// Usages of those features are logged at WARN level with the position of where the feature is used. +func reportUnsoundFeatures(state *AnalyzerState, f *ssa.Function) { + unsoundFeatures := FindUnsoundFeatures(f) + if len(unsoundFeatures.Recovers) > 0 || + len(unsoundFeatures.UnsafeUsages) > 0 || + len(unsoundFeatures.ReflectUsages) > 0 { + msg := fmt.Sprintf("Function %s is using features that may make the analysis unsound.\n", f.String()) + + if len(unsoundFeatures.Recovers) > 0 { + msg += " Using recover at position:\n" + } + for pos := range unsoundFeatures.Recovers { + msg += "\t " + pos.String() + "\n" + } + + if len(unsoundFeatures.UnsafeUsages) > 0 { + msg += " Usages of unsafe:\n" + } + + for pos, usageMsg := range unsoundFeatures.UnsafeUsages { + msg += " At " + pos.String() + " : " + usageMsg + "\n" + } + + if len(unsoundFeatures.ReflectUsages) > 0 { + msg += " Usages of reflection:\n" + } + + for pos, usageMsg := range unsoundFeatures.ReflectUsages { + msg += " At " + pos.String() + " : " + usageMsg + "\n" + } + msg += " Adding a predefined summary might help avoid soundness issues.\n" + + state.Logger.Warnf(msg) + } +} +func FindUnsoundFeatures(f *ssa.Function) unsoundFeaturesMap { + unsafeUsages := map[token.Position]string{} + recovers := map[token.Position]bool{} + reflectUsages := map[token.Position]string{} + lang.IterateInstructions(f, func(index int, instrI ssa.Instruction) { + iPos := instrI.Parent().Prog.Fset.Position(instrI.Pos()) + switch instr := instrI.(type) { + case ssa.CallInstruction: + callCommon := instr.Common() + if callCommon.Value == nil { + return + } + if callCommon.Value.Name() == "recover" { + recovers[iPos] = true + return + } + if callCommon.IsInvoke() { + // Only warn for implementations. + return + } + typStr := callCommon.Value.Type().String() + if strings.Contains(typStr, "unsafe") { + unsafeUsages[iPos] = fmt.Sprintf("Calling %s from unsafe package.", callCommon.Value.Name()) + return + } + if strings.Contains(typStr, "reflect") { + reflectUsages[iPos] = fmt.Sprintf("Calling %s from reflect package.", callCommon.Value.Name()) + return + } + + case *ssa.Alloc: + typ := instr.Type().Underlying() + if strings.HasPrefix(typ.String(), "unsafe") { + unsafeUsages[iPos] = fmt.Sprintf("Allocating object of type %s", typ.String()) + return + } + if strings.HasPrefix(typ.String(), "reflect") { + reflectUsages[iPos] = fmt.Sprintf("Allocating object of type %s", typ.String()) + return + } + case *ssa.Convert: + typStr := instr.Type().String() + if strings.Contains(typStr, "unsafe") { + unsafeUsages[iPos] = "Converting data to an unsafe pointer." + } + } + }) + return unsoundFeaturesMap{recovers, unsafeUsages, reflectUsages} +} diff --git a/analysis/dataflow/testdata/unsound-features/config.yaml b/analysis/dataflow/testdata/unsound-features/config.yaml new file mode 100644 index 00000000..7a1c19f4 --- /dev/null +++ b/analysis/dataflow/testdata/unsound-features/config.yaml @@ -0,0 +1,15 @@ +taint-tracking-problems: + - sources: + - package: main + method: Source + - package: command-line-arguments + method: Source + - package: (main|command-line-arguments) + field: Source + sinks: + - package: command-line-arguments + method: Sink + - package: main + method: Sink +options: + summarize-on-demand: true \ No newline at end of file diff --git a/analysis/dataflow/testdata/unsound-features/main.go b/analysis/dataflow/testdata/unsound-features/main.go new file mode 100644 index 00000000..bf23d242 --- /dev/null +++ b/analysis/dataflow/testdata/unsound-features/main.go @@ -0,0 +1,98 @@ +/* + * Copyright (c) + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * 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 main + +import ( + "fmt" + "math/rand" + "reflect" + "strconv" + "unsafe" +) + +func mayPanic() { + panic("a problem") +} + +func Source() string { + return "x" + strconv.Itoa(rand.Int()) +} + +func Sink(x string) { + fmt.Println(x) +} + +type Example struct { + x int + f string +} + +func (e Example) String() string { + return e.f + strconv.Itoa(e.x) +} + +func usingUnsafe() { + s := Example{ + x: 1243, + f: Source(), + } + x := []string{"a", "b", "c"} + var i uintptr + i = 3 + // equivalent to f := unsafe.Pointer(&s.f) + f := unsafe.Pointer(uintptr(unsafe.Pointer(&s)) + unsafe.Offsetof(s.f)) + // equivalent to e := unsafe.Pointer(&x[i]) + e := unsafe.Pointer(uintptr(unsafe.Pointer(&x[0])) + i*unsafe.Sizeof(x[0])) + + fmt.Println(e, f) +} + +func usingReflect() { + x := 10 + name := "Go Lang" + example := Example{1, Source()} // @Source(reflect) + fmt.Println(reflect.TypeOf(x)) + fmt.Println(reflect.TypeOf(name)) + fmt.Println(reflect.TypeOf(example)) + + method := reflect.ValueOf(example).MethodByName("String") + if !method.IsValid() { + fmt.Println("ERROR: String is not implemented") + return + } + e := method.Call(nil) // this call is elided by the pointer analysis and results in false negatives! + Sink(e[0].String()) // @Sink(reflect) +} + +func usingRecover() { + defer func() { + if r := recover(); r != nil { + + fmt.Println("Recovered. Error:\n", r) + } + }() + mayPanic() + fmt.Println("After mayPanic()") + +} + +func main() { + usingUnsafe() + usingReflect() + usingRecover() +} diff --git a/analysis/dataflow/unsound_features_test.go b/analysis/dataflow/unsound_features_test.go new file mode 100644 index 00000000..85670cd1 --- /dev/null +++ b/analysis/dataflow/unsound_features_test.go @@ -0,0 +1,66 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// +// 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 dataflow_test + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/awslabs/ar-go-tools/analysis/config" + "github.com/awslabs/ar-go-tools/analysis/dataflow" + "github.com/awslabs/ar-go-tools/internal/analysistest" +) + +func TestUnsoundFeatures(t *testing.T) { + dir := filepath.Join("testdata", "unsound-features") + lp, err := analysistest.LoadTest(testfsys, dir, []string{}, + analysistest.LoadTestOptions{ApplyRewrite: true}) + if err != nil { + t.Fatalf("failed to load test: %v", err) + } + c, err := dataflow.NewInitializedAnalyzerState(lp.Prog, lp.Pkgs, config.NewLogGroup(lp.Config), lp.Config) + if err != nil { + t.Errorf("error building state: %q", err) + } + var unsafeChecked, reflectChecked, recoverChecked bool + for f := range c.ReachableFunctions() { + if strings.Contains(f.Name(), "usingUnsafe") { + + uf := dataflow.FindUnsoundFeatures(f) + if len(uf.UnsafeUsages) <= 2 { + t.Errorf("Did not detect enough usages of unsage in usingUnsafe") + } + unsafeChecked = true + } + if strings.Contains(f.Name(), "usingReflect") { + uf := dataflow.FindUnsoundFeatures(f) + if len(uf.ReflectUsages) <= 2 { + t.Errorf("Did not detect enough usages of unsage in usingReflect") + } + reflectChecked = true + } + if strings.Contains(f.Name(), "usingRecover$1") { + uf := dataflow.FindUnsoundFeatures(f) + if len(uf.Recovers) <= 0 { + t.Errorf("Did not detect enough usages of recover in usingRecover") + } + recoverChecked = true + } + } + if !(recoverChecked && reflectChecked && unsafeChecked) { + t.Errorf("Failed to check for recover, reflect and unsafe functions") + } +} diff --git a/analysis/summaries/standard_library.go b/analysis/summaries/standard_library.go index c03e9b78..1cf11e4c 100644 --- a/analysis/summaries/standard_library.go +++ b/analysis/summaries/standard_library.go @@ -791,7 +791,12 @@ var summaryReflect = map[string]Summary{ [][]int{{0}, {1}}, [][]int{{0}, {0}}, }, - "(reflect.Value).Bool": SingleVarArgPropagation, + "(reflect.Value).Bool": SingleVarArgPropagation, + // Over-approximation for Call: it is assumed the function being called fully propagates data + "(reflect.Value).Call": { + [][]int{{0}, {1}}, + [][]int{{0}, {0}}, + }, "(reflect.Value).Float": SingleVarArgPropagation, "(reflect.Value).Int": SingleVarArgPropagation, // func (v Value) Elem() Value @@ -829,6 +834,11 @@ var summaryReflect = map[string]Summary{ "(reflect.Value).Kind": SingleVarArgPropagation, // func (v Value) Len() int "(reflect.Value).Len": SingleVarArgPropagation, + // func (v Value) MethodByName(name string) Value + "(reflect.Value).MethodByName": { + [][]int{{0}, {1}}, + [][]int{{0}, {0}}, + }, // func (v Value) NumField() int "(reflect.Value).NumField": SingleVarArgPropagation, // func (v Value) MapKeys() []Value diff --git a/analysis/taint/testdata/basic/config.yaml b/analysis/taint/testdata/basic/config.yaml index ae99296e..fec22792 100644 --- a/analysis/taint/testdata/basic/config.yaml +++ b/analysis/taint/testdata/basic/config.yaml @@ -9,6 +9,9 @@ taint-tracking-problems: - package: "(basic)|(main)|(command-line-arguments)" type: "(chan \\*_S)|(chan _S)" kind: "channel receive" + - package: "(basic)|(main)|(command-line-arguments)" + type: "\\*Sample" + field: "Secret" sinks: - package: "(basic)|(main)|(command-line-arguments)" # Similarly, sinks are sink1 sink2 sink2 ... diff --git a/analysis/taint/testdata/basic/fields.go b/analysis/taint/testdata/basic/fields.go index c605c03d..98573ba6 100644 --- a/analysis/taint/testdata/basic/fields.go +++ b/analysis/taint/testdata/basic/fields.go @@ -14,7 +14,13 @@ package main -import "fmt" +import ( + "crypto/aes" + "crypto/cipher" + "encoding/base64" + "encoding/json" + "fmt" +) type Example struct { SourceField string @@ -29,6 +35,40 @@ func testField() { sink1(s4) // tainted data reaches this @Sink(field1,field2) } +type Sample struct { + Secret string + OtherData string +} + +func testField2() { + var payload Sample + err := json.Unmarshal([]byte("{\"Secret\":\"sdfds\"}"), &payload) + if err != nil { + return + } + + decodedAESKey, err := base64.StdEncoding.DecodeString(payload.OtherData) + if err != nil { + return + } + + newCipher, err := aes.NewCipher(decodedAESKey) + if err != nil { + return + } + + gcm, err := cipher.NewGCM(newCipher) + if err != nil { + return + } + nonceSize := gcm.NonceSize() + creds, err := base64.StdEncoding.DecodeString(payload.Secret) // @Source(secret) + if err != nil { + return + } + sink2(fmt.Sprintf("%s-%d", creds, nonceSize)) // @Sink(secret) +} + type SourceStruct struct { Source1 string } diff --git a/analysis/taint/testdata/basic/main.go b/analysis/taint/testdata/basic/main.go index 91c8a457..9a669e1a 100644 --- a/analysis/taint/testdata/basic/main.go +++ b/analysis/taint/testdata/basic/main.go @@ -65,6 +65,7 @@ func main() { test5() // see example3.go testField() // see fields.go testFieldEmbedded() // see fields.go + testField2() // see fields.go runSanitizerExamples() // see sanitizers.go testAliasingTransitive() // see memory.go testChannelReadAsSource() // see channels.go diff --git a/cmd/argot/cli/defs.go b/cmd/argot/cli/defs.go index be1f2dda..57e0fb86 100644 --- a/cmd/argot/cli/defs.go +++ b/cmd/argot/cli/defs.go @@ -17,6 +17,7 @@ package cli import ( "go/token" "regexp" + "slices" "strings" "github.com/awslabs/ar-go-tools/analysis/dataflow" @@ -88,7 +89,9 @@ func funcsMatchingCommand(tt *term.Terminal, c *dataflow.AnalyzerState, command regexErr(tt, rString, err) return []*ssa.Function{} } - return findFunc(c, r) + s := findFunc(c, r) + slices.SortFunc(s, func(a, b *ssa.Function) int { return strings.Compare(a.String(), b.String()) }) + return s } func findFunc(c *dataflow.AnalyzerState, target *regexp.Regexp) []*ssa.Function {