diff --git a/secmem-lint/analyzer.go b/secmem-lint/analyzer.go index f660359..2e7527b 100644 --- a/secmem-lint/analyzer.go +++ b/secmem-lint/analyzer.go @@ -54,10 +54,15 @@ func run(pass *analysis.Pass) (any, error) { // accessor is a recognized secmem borrowing-closure call: // recv.Method(func(p []byte) { ... }). +// +// Both fields identify things by types.Object rather than by name or AST shape. +// Names are the wrong key twice over: a receiver written as a field selector +// (s.buf) is not an identifier at all, and a borrowed parameter's name can be +// shadowed by an unrelated variable that happens to match. type accessor struct { - recv *ast.Ident // receiver identifier, or nil if not a plain identifier - fn *ast.FuncLit // the borrowing closure - params map[string]bool // names of its []byte parameters (the borrowed slices) + recv []types.Object // receiver identity chain, nil if undecidable + fn *ast.FuncLit // the borrowing closure + params map[types.Object]bool // its []byte parameters (the borrowed slices) } // borrowAccessor reports whether call is a secmem borrowing accessor and, if so, @@ -87,14 +92,68 @@ func borrowAccessor(pass *analysis.Pass, call *ast.CallExpr) (accessor, bool) { if lit == nil { return accessor{}, false } - params := byteSliceParams(lit) + params := byteSliceParams(pass, lit) if len(params) == 0 { return accessor{}, false } - recv, _ := sel.X.(*ast.Ident) + recv, _ := receiverKey(pass, sel.X) return accessor{recv: recv, fn: lit, params: params}, true } +// receiverKey builds a comparison key for a receiver expression: the chain of +// objects from the root identifier through any field selections, so buf and +// s.buf and s.inner.buf each get a key that can be compared for identity. +// +// It reports false for shapes whose identity cannot be decided statically — +// index expressions, calls, type assertions. bufs[i] and bufs[j] are written +// alike and need not be the same buffer, and getBuf() twice need not return the +// same one, so treating them as equal would be a false positive on a linter +// whose findings block a build. +func receiverKey(pass *analysis.Pass, expr ast.Expr) ([]types.Object, bool) { + switch e := expr.(type) { + case *ast.Ident: + obj := pass.TypesInfo.ObjectOf(e) + if obj == nil { + return nil, false + } + return []types.Object{obj}, true + + case *ast.ParenExpr: + return receiverKey(pass, e.X) + + case *ast.StarExpr: + // (*p).WithBytes(...) names the same buffer as p.WithBytes(...). + return receiverKey(pass, e.X) + + case *ast.SelectorExpr: + base, ok := receiverKey(pass, e.X) + if !ok { + return nil, false + } + field := pass.TypesInfo.ObjectOf(e.Sel) + if field == nil { + return nil, false + } + key := make([]types.Object, 0, len(base)+1) + key = append(key, base...) + return append(key, field), true + } + return nil, false +} + +// sameReceiver reports whether two receiver keys name the same buffer. +func sameReceiver(a, b []types.Object) bool { + if len(a) == 0 || len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + func isBorrowMethod(pkgPath, method string) bool { switch pkgPath { case secmemPkg: @@ -105,24 +164,27 @@ func isBorrowMethod(pkgPath, method string) bool { return false } -// byteSliceParams returns the names of the closure's []byte parameters — the -// borrowed slices whose escape the checks track. -func byteSliceParams(fn *ast.FuncLit) map[string]bool { - names := make(map[string]bool) +// byteSliceParams returns the closure's []byte parameters — the borrowed slices +// whose escape the checks track. +func byteSliceParams(pass *analysis.Pass, fn *ast.FuncLit) map[types.Object]bool { + objs := make(map[types.Object]bool) if fn.Type == nil || fn.Type.Params == nil { - return names + return objs } for _, field := range fn.Type.Params.List { if !isByteSlice(field.Type) { continue } for _, n := range field.Names { - if n.Name != "_" { - names[n.Name] = true + if n.Name == "_" { + continue + } + if obj := pass.TypesInfo.ObjectOf(n); obj != nil { + objs[obj] = true } } } - return names + return objs } func isByteSlice(expr ast.Expr) bool { @@ -196,26 +258,31 @@ func nolintApplies(comment string) bool { // refersToParam reports whether expr is (a paren/slice around) a borrowed param. // It deliberately does not recurse into calls, so len(p) or f(p) is not itself a // direct reference to the borrowed slice. -func refersToParam(expr ast.Expr, params map[string]bool) bool { +func refersToParam(pass *analysis.Pass, expr ast.Expr, params map[types.Object]bool) bool { switch e := expr.(type) { case *ast.Ident: - return params[e.Name] + return params[pass.TypesInfo.ObjectOf(e)] case *ast.ParenExpr: - return refersToParam(e.X, params) + return refersToParam(pass, e.X, params) case *ast.SliceExpr: - return refersToParam(e.X, params) + return refersToParam(pass, e.X, params) } return false } // goStmtLeaksParam reports whether a go statement hands a borrowed param to the // new goroutine — captured by a closure body or passed as an argument. -func goStmtLeaksParam(call *ast.CallExpr, params map[string]bool) bool { +// +// The capture scan resolves each identifier to its object rather than comparing +// names. A goroutine that declares its own b, or ranges over one, is not +// touching the borrowed slice at all, and matching on the name alone reported it +// as a leak. +func goStmtLeaksParam(pass *analysis.Pass, call *ast.CallExpr, params map[types.Object]bool) bool { if call == nil { return false } for _, arg := range call.Args { - if refersToParam(arg, params) { + if refersToParam(pass, arg, params) { return true } } @@ -228,7 +295,7 @@ func goStmtLeaksParam(call *ast.CallExpr, params map[string]bool) bool { if leaked { return false } - if id, ok := n.(*ast.Ident); ok && params[id.Name] { + if id, ok := n.(*ast.Ident); ok && params[pass.TypesInfo.ObjectOf(id)] { leaked = true } return !leaked diff --git a/secmem-lint/escape.go b/secmem-lint/escape.go index 8b7aaa1..1973dec 100644 --- a/secmem-lint/escape.go +++ b/secmem-lint/escape.go @@ -17,11 +17,11 @@ func checkCallbackEscapes(pass *analysis.Pass, acc accessor, sup *suppressor) { case *ast.CallExpr: checkCallEscape(pass, acc, node, sup) case *ast.SendStmt: - if refersToParam(node.Value, acc.params) && !sup.suppressed(pass, node.Pos()) { + if refersToParam(pass, node.Value, acc.params) && !sup.suppressed(pass, node.Pos()) { report(pass, node.Pos(), "borrowed secret bytes sent to a channel; they can outlive the closure") } case *ast.GoStmt: - if goStmtLeaksParam(node.Call, acc.params) && !sup.suppressed(pass, node.Pos()) { + if goStmtLeaksParam(pass, node.Call, acc.params) && !sup.suppressed(pass, node.Pos()) { report(pass, node.Pos(), "borrowed secret bytes handed to a goroutine; they can outlive the closure") } case *ast.AssignStmt: @@ -37,7 +37,7 @@ func checkCallEscape(pass *analysis.Pass, acc accessor, call *ast.CallExpr, sup if id, ok := call.Fun.(*ast.Ident); ok { switch id.Name { case "string": - if len(call.Args) == 1 && refersToParam(call.Args[0], acc.params) && !sup.suppressed(pass, call.Pos()) { + if len(call.Args) == 1 && refersToParam(pass, call.Args[0], acc.params) && !sup.suppressed(pass, call.Pos()) { report(pass, call.Pos(), "string() copies borrowed secret bytes into a heap string") } return @@ -46,7 +46,7 @@ func checkCallEscape(pass *analysis.Pass, acc accessor, call *ast.CallExpr, sup // the same escape written with a slice expression, and matching // only the identifier let it through. if call.Ellipsis.IsValid() && len(call.Args) >= 2 { - if refersToParam(call.Args[len(call.Args)-1], acc.params) && !sup.suppressed(pass, call.Pos()) { + if refersToParam(pass, call.Args[len(call.Args)-1], acc.params) && !sup.suppressed(pass, call.Pos()) { report(pass, call.Pos(), "append(dst, borrowed...) copies borrowed secret bytes into an escaping slice") } } @@ -54,12 +54,12 @@ func checkCallEscape(pass *analysis.Pass, acc accessor, call *ast.CallExpr, sup case "panic": // The value is formatted into the runtime traceback and handed to // any recover() up the stack, both well outside the lease. - if len(call.Args) == 1 && refersToParam(call.Args[0], acc.params) && !sup.suppressed(pass, call.Pos()) { + if len(call.Args) == 1 && refersToParam(pass, call.Args[0], acc.params) && !sup.suppressed(pass, call.Pos()) { report(pass, call.Pos(), "panic() puts borrowed secret bytes in the traceback and in any recover()") } return case "copy": - if len(call.Args) == 2 && refersToParam(call.Args[1], acc.params) && !sup.suppressed(pass, call.Pos()) { + if len(call.Args) == 2 && refersToParam(pass, call.Args[1], acc.params) && !sup.suppressed(pass, call.Pos()) { report(pass, call.Pos(), "copy() moves borrowed secret bytes out of the closure") } return @@ -76,7 +76,7 @@ func checkAssignEscape(pass *analysis.Pass, acc accessor, stmt *ast.AssignStmt, return } for i, rhs := range stmt.Rhs { - if i >= len(stmt.Lhs) || !refersToParam(rhs, acc.params) { + if i >= len(stmt.Lhs) || !refersToParam(pass, rhs, acc.params) { continue } where, escapes := assignTargetEscapes(pass, stmt.Lhs[i], acc) @@ -183,7 +183,7 @@ func checkSink(pass *analysis.Pass, acc accessor, call *ast.CallExpr, sup *suppr return } for _, arg := range call.Args { - if refersToParam(arg, acc.params) { + if refersToParam(pass, arg, acc.params) { if !sup.suppressed(pass, call.Pos()) { report(pass, call.Pos(), fmt.Sprintf("borrowed secret bytes passed to %s; %s", name, reason)) } diff --git a/secmem-lint/reentrancy.go b/secmem-lint/reentrancy.go index 6fde7c0..f315f22 100644 --- a/secmem-lint/reentrancy.go +++ b/secmem-lint/reentrancy.go @@ -34,12 +34,13 @@ var reentrantUnsafe = map[string]bool{ //nolint:gochecknoglobals // immutable lo // checkReentrancy flags an access method called on the SAME buffer inside its own // borrowing closure. A DIFFERENT buffer (the documented decrypt-into pattern) is // resolved by object identity and is not flagged. +// +// Receivers are compared as identity chains, so a buffer held in a struct field +// is covered. Requiring a plain identifier on both ends — which is all this +// check used to do — silently disabled it for s.buf.WithBytes(...), and holding +// the buffer in a struct is how most programs of any size hold it. func checkReentrancy(pass *analysis.Pass, acc accessor, sup *suppressor) { - if acc.recv == nil { - return - } - recvObj := pass.TypesInfo.ObjectOf(acc.recv) - if recvObj == nil { + if len(acc.recv) == 0 { return } ast.Inspect(acc.fn.Body, func(n ast.Node) bool { @@ -51,8 +52,8 @@ func checkReentrancy(pass *analysis.Pass, acc accessor, sup *suppressor) { if !ok || !reentrantUnsafe[sel.Sel.Name] { return true } - inner, ok := sel.X.(*ast.Ident) - if !ok || pass.TypesInfo.ObjectOf(inner) != recvObj { + inner, ok := receiverKey(pass, sel.X) + if !ok || !sameReceiver(acc.recv, inner) { return true } if !sup.suppressed(pass, call.Pos()) { diff --git a/secmem-lint/testdata/src/escape/escape.go b/secmem-lint/testdata/src/escape/escape.go index cdfb95c..d161cd5 100644 --- a/secmem-lint/testdata/src/escape/escape.go +++ b/secmem-lint/testdata/src/escape/escape.go @@ -83,3 +83,17 @@ func cleanInnerAggregate(buf *secmem.SecureBuffer) { _ = local }) } + +// shadowedNameInGoroutineOK: the goroutine declares its own b. The capture scan +// used to match on the identifier's NAME, so this unrelated variable was +// reported as a leak of the borrowed slice. +func shadowedNameInGoroutineOK(buf *secmem.SecureBuffer, other [][]byte) { + _ = buf.WithBytes(func(b []byte) { + _ = b + go func() { + for _, b := range other { + _ = len(b) + } + }() + }) +} diff --git a/secmem-lint/testdata/src/reentrancy/reentrancy.go b/secmem-lint/testdata/src/reentrancy/reentrancy.go index 76c3184..095b783 100644 --- a/secmem-lint/testdata/src/reentrancy/reentrancy.go +++ b/secmem-lint/testdata/src/reentrancy/reentrancy.go @@ -44,3 +44,44 @@ func sameBufferLockingInspectors(buf *secmem.SecureBuffer) { _ = buf.IsDestroyed() // want `secmem-lint: IsDestroyed called on the same buffer` }) } + +// vault holds its buffer in a struct field, which is how any program larger +// than an example holds it. The check used to require a plain identifier on +// both ends, so every one of these was silently clean. +type vault struct { + buf *secmem.SecureBuffer + inner struct{ buf *secmem.SecureBuffer } +} + +func (v *vault) fieldReceiver() { + _ = v.buf.WithBytes(func(b []byte) { + _ = b + _ = v.buf.Len() // want `secmem-lint: Len called on the same buffer` + }) +} + +func (v *vault) nestedFieldReceiver() { + _ = v.inner.buf.WithBytes(func(b []byte) { + _ = b + _ = v.inner.buf.IsSealed() // want `secmem-lint: IsSealed called on the same buffer` + }) +} + +// differentFieldOK: a different field is a different buffer, so the +// decrypt-into pattern still has to survive the wider receiver matching. +func (v *vault) differentFieldOK() { + _ = v.buf.WithBytes(func(b []byte) { + _ = b + _ = v.inner.buf.Len() + }) +} + +// indexedReceiverNotDecidable: bufs[i] and bufs[j] are written alike and need +// not be the same buffer, so receivers that are index expressions are left +// alone rather than guessed at. +func indexedReceiverNotDecidable(bufs []*secmem.SecureBuffer) { + _ = bufs[0].WithBytes(func(b []byte) { + _ = b + _ = bufs[1].Len() + }) +}