From 23c8641855689198daee179475f149757af64472 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Tue, 1 Aug 2023 23:43:01 +0100 Subject: [PATCH] propagate "uses reflection" through SSA stores Up until now, the new SSA reflection detection relied on call sites to propagate which objects (named types, struct fields) used reflection. For example, given the code: json.Marshal(new(T)) we would first record that json.Marshal calls reflect.TypeOf, and then that the user's code called json.Marshal with the type *T. However, this would not catch a slight variation on the above: var t T reflect.TypeOf(t) t.foo = struct{bar int}{} Here, type T's fields such as "foo" and "bar" are not obfuscated, since our logic sees the call site and marks the type T recursively. However, the unnamed `struct{bar int}` type was still obfuscated, causing errors such as: cannot use struct{uKGvcJvD24 int}{} (value of type struct{uKGvcJvD24 int}) as struct{bar int} value in assignment The solution is to teach the analysis about *ssa.Store instructions in a similar way to how it already knows about *ssa.Call instructions. If we see a store where the destination type is marked for reflection, then we mark the source type as well, fixing the bug above. This fixes obfuscating github.com/gogo/protobuf/proto. A number of other Go modules fail with similar errors, and they look like very similar bugs, but this particular fix doesn't apply to them. Future incremental fixes will try to deal with those extra cases. Fixes #685. --- main.go | 7 ++- reflect.go | 109 ++++++++++++++++++++-------------- testdata/script/reflect.txtar | 22 +++++++ 3 files changed, 92 insertions(+), 46 deletions(-) diff --git a/main.go b/main.go index 46c870e..1dd2fb5 100644 --- a/main.go +++ b/main.go @@ -1539,9 +1539,10 @@ func computePkgCache(fsCache *cache.Cache, lpkg *listedPackage, pkg *types.Packa // Fill the reflect info from SSA, which builds on top of the syntax tree and type info. inspector := reflectInspector{ - pkg: pkg, - checkedAPIs: make(map[string]bool), - result: computed, // append the results + pkg: pkg, + checkedAPIs: make(map[string]bool), + propagatedStores: map[*ssa.Store]bool{}, + result: computed, // append the results } if ssaPkg == nil { ssaPkg = ssaBuildPkg(pkg, files, info) diff --git a/reflect.go b/reflect.go index 9289dc6..febf5f7 100644 --- a/reflect.go +++ b/reflect.go @@ -15,6 +15,8 @@ type reflectInspector struct { checkedAPIs map[string]bool + propagatedStores map[*ssa.Store]bool + result pkgCache } @@ -24,7 +26,7 @@ func (ri *reflectInspector) recordReflection(ssaPkg *ssa.Package) { return } - lenPrevReflectAPIs := len(ri.result.ReflectAPIs) + prevDone := len(ri.result.ReflectAPIs) + len(ri.propagatedStores) // find all unchecked APIs to add them to checkedAPIs after the pass notCheckedAPIs := make(map[string]bool) @@ -41,8 +43,9 @@ func (ri *reflectInspector) recordReflection(ssaPkg *ssa.Package) { maps.Copy(ri.checkedAPIs, notCheckedAPIs) // if a new reflectAPI is found we need to Re-evaluate all functions which might be using that API - if len(ri.result.ReflectAPIs) > lenPrevReflectAPIs { - ri.recordReflection(ssaPkg) + newDone := len(ri.result.ReflectAPIs) + len(ri.propagatedStores) + if newDone > prevDone { + ri.recordReflection(ssaPkg) // TODO: avoid recursing } } @@ -157,10 +160,10 @@ func (ri *reflectInspector) checkInterfaceMethod(m *types.Func) { // Checks all callsites in a function declaration for use of reflection. func (ri *reflectInspector) checkFunction(fun *ssa.Function) { - /* if fun != nil && fun.Synthetic != "loaded from gc object file" { - // fun.WriteTo crashes otherwise - fun.WriteTo(os.Stdout) - } */ + // if fun != nil && fun.Synthetic != "loaded from gc object file" { + // // fun.WriteTo crashes otherwise + // fun.WriteTo(os.Stdout) + // } f, _ := fun.Object().(*types.Func) @@ -173,54 +176,59 @@ func (ri *reflectInspector) checkFunction(fun *ssa.Function) { } } - /* fmt.Printf("f: %v\n", f) - fmt.Printf("fun: %v\n", fun) */ + // fmt.Printf("f: %v\n", f) + // fmt.Printf("fun: %v\n", fun) for _, block := range fun.Blocks { for _, inst := range block.Instrs { - /* fmt.Printf("inst: %v, t: %T\n", inst, inst) */ - call, ok := inst.(*ssa.Call) - if !ok { - continue - } - - callName := call.Call.Value.String() - if m := call.Call.Method; m != nil { - callName = call.Call.Method.FullName() - } - - if ri.checkedAPIs[callName] { - // only check apis which were not already checked - continue - } - - /* fmt.Printf("callName: %v\n", callName) */ + // fmt.Printf("inst: %v, t: %T\n", inst, inst) + switch inst := inst.(type) { + case *ssa.Store: + if ri.propagatedStores[inst] { + break // already done + } + if storeAddrUsedForReflect(ri.result, inst.Addr.Type()) { + ri.recordArgReflected(inst.Val, make(map[ssa.Value]bool)) + ri.propagatedStores[inst] = true + } + case *ssa.Call: + callName := inst.Call.Value.String() + if m := inst.Call.Method; m != nil { + callName = inst.Call.Method.FullName() + } - // record each call argument passed to a function parameter which is used in reflection - knownParams := ri.result.ReflectAPIs[callName] - for knownParam := range knownParams { - if len(call.Call.Args) <= knownParam { + if ri.checkedAPIs[callName] { + // only check apis which were not already checked continue } - arg := call.Call.Args[knownParam] + /* fmt.Printf("callName: %v\n", callName) */ - /* fmt.Printf("flagging arg: %v\n", arg) */ + // record each call argument passed to a function parameter which is used in reflection + knownParams := ri.result.ReflectAPIs[callName] + for knownParam := range knownParams { + if len(inst.Call.Args) <= knownParam { + continue + } - visited := make(map[ssa.Value]bool) - reflectedParam := ri.recordArgReflected(arg, visited) - if reflectedParam == nil { - continue - } + arg := inst.Call.Args[knownParam] - pos := slices.Index(fun.Params, reflectedParam) - if pos < 0 { - continue - } + /* fmt.Printf("flagging arg: %v\n", arg) */ - /* fmt.Printf("recorded param: %v func: %v\n", pos, fun) */ + reflectedParam := ri.recordArgReflected(arg, make(map[ssa.Value]bool)) + if reflectedParam == nil { + continue + } - reflectParams[pos] = true + pos := slices.Index(fun.Params, reflectedParam) + if pos < 0 { + continue + } + + /* fmt.Printf("recorded param: %v func: %v\n", pos, fun) */ + + reflectParams[pos] = true + } } } } @@ -459,3 +467,18 @@ func usedForReflect(cache pkgCache, obj types.Object) bool { _, ok := cache.ReflectObjects[objStr] return ok } + +// storeAddrUsedForReflect is only used in reflectInspector +// to see if a [ssa.Store.Addr] has been marked as used by reflection. +// We only mark named objects, so this function looks for a type's first struct field. +func storeAddrUsedForReflect(cache pkgCache, typ types.Type) bool { + switch typ := typ.(type) { + case *types.Struct: + if typ.NumFields() > 0 { + return usedForReflect(cache, typ.Field(0)) + } + case interface{ Elem() types.Type }: + return storeAddrUsedForReflect(cache, typ.Elem()) + } + return false +} diff --git a/testdata/script/reflect.txtar b/testdata/script/reflect.txtar index f9119ef..aa932e4 100644 --- a/testdata/script/reflect.txtar +++ b/testdata/script/reflect.txtar @@ -31,6 +31,7 @@ import ( "reflect" "unsafe" "strings" + "sync" "text/template" "test/main/importedpkg" @@ -164,6 +165,12 @@ func main() { embedFieldField.Name, ) } + + y := UnnamedStructFields{} + y.unexportedGoGoProto = new(struct { + mu sync.Mutex + extensionMap map[int32]EncodingT + }) } type EmbeddingIndirect struct { @@ -334,6 +341,21 @@ type UnnamedStructInterface interface { UnnamedStructMethod(struct{ UnnamedStructField string }) } +// Some projects declare types with unnamed struct fields, +// and the entire type is used via reflection and cannot be obfuscated. +// However, when assigning to these fields, the use of inline anonymous struct types +// confused garble, and it did not obfuscate those inline structs as well. +// That resulted in "cannot use X as Y value in assignment" build errors. + +var _ = reflect.TypeOf(UnnamedStructFields{}) + +type UnnamedStructFields struct { + // As seen in github.com/gogo/protobuf/proto. + unexportedGoGoProto *struct { + mu sync.Mutex + extensionMap map[int32]EncodingT + } +} -- importedpkg/imported.go -- package importedpkg