Skip to content

Commit

Permalink
Fix field sensitivity soundness with predefined summaries + reachabil…
Browse files Browse the repository at this point in the history
…ity info from dataflow state.
  • Loading branch information
victornicolet committed Dec 16, 2024
1 parent 9dd1ee1 commit 0d71bdc
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 9 deletions.
10 changes: 6 additions & 4 deletions analysis/dataflow/function_summary_graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ import (
"golang.org/x/tools/go/ssa"
)

var allAccessPathFlow = map[string]map[string]bool{"*": {"": true}}

// SummaryGraph is the function dataflow summary graph.
type SummaryGraph struct {
// the unique ID of the summary
Expand Down Expand Up @@ -932,12 +934,12 @@ func (g *SummaryGraph) addParamEdgeByPos(src int, dest int) bool {
if outEdges == nil {
outEdges = make([]EdgeInfo, 0, 1)
}
srcArg.out[destArg] = append(outEdges, EdgeInfo{map[string]map[string]bool{}, 0, nil})
srcArg.out[destArg] = append(outEdges, EdgeInfo{allAccessPathFlow, 0, nil})

if destArg.in == nil {
destArg.in = make(map[GraphNode]EdgeInfo)
}
destArg.in[srcArg] = EdgeInfo{map[string]map[string]bool{}, 0, nil}
destArg.in[srcArg] = EdgeInfo{allAccessPathFlow, 0, nil}
return true
}
}
Expand Down Expand Up @@ -965,12 +967,12 @@ func (g *SummaryGraph) addReturnEdgeByPos(src int, pos int) bool {
if outEdges == nil {
outEdges = make([]EdgeInfo, 0, 1)
}
srcArg.out[retNode[pos]] = append(outEdges, EdgeInfo{map[string]map[string]bool{}, pos, nil})
srcArg.out[retNode[pos]] = append(outEdges, EdgeInfo{allAccessPathFlow, pos, nil})

if retNode[pos].in == nil {
retNode[pos].in = make(map[GraphNode]EdgeInfo)
}
retNode[pos].in[srcArg] = EdgeInfo{map[string]map[string]bool{}, pos, nil}
retNode[pos].in[srcArg] = EdgeInfo{allAccessPathFlow, pos, nil}
return true
}
}
Expand Down
6 changes: 2 additions & 4 deletions analysis/dataflow/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ type State struct {
// BoundingInfo is a map from pointer labels to the closures that bind them. The bounding analysis produces such
// a map
BoundingInfo BoundingMap

reachableFunctions map[*ssa.Function]bool
}

// NewState generates a State from a PointerState
Expand Down Expand Up @@ -237,8 +235,8 @@ func (s *State) PopulateBoundingInformation(verbose bool) error {
// IsReachableFunction returns true if f is reachable according to the pointer analysis, or if the pointer analysis
// and ReachableFunctions has never been called.
func (s *State) IsReachableFunction(f *ssa.Function) bool {
if s != nil && s.reachableFunctions != nil {
return s.reachableFunctions[f]
if s != nil && s.ReachableFunctions() != nil {
return s.ReachableFunctions()[f]
}
// If no reachability information has been computed, assume every function is reachable
s.Logger.Debugf("No reachability information has been computed")
Expand Down
1 change: 1 addition & 0 deletions analysis/summaries/standard_library.go
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ var summaryCrypto = map[string]Summary{
"crypto/x509.MarshalPKIXPublicKey": SingleVarArgPropagation,
"crypto/x509.ParsePKCS1PrivateKey": SingleVarArgPropagation,
"crypto/x509.SystemCertPool": NoDataFlowPropagation,
"crypto/sha256.blockGeneric": NoDataFlowPropagation,
"(crypto.Hash).New": SingleVarArgPropagation,
"(*crypto/tls.Config).Clone": SingleVarArgPropagation,
"(*crypto/x509.CertPool).AppendCertsFromPEM": {
Expand Down
5 changes: 4 additions & 1 deletion analysis/taint/dataflow_visitor.go
Original file line number Diff line number Diff line change
Expand Up @@ -767,9 +767,12 @@ func (v *Visitor) addNext(s *df.State,
}
}
}
if len(edgeInfo.RelPath) == 0 || (len(edgeInfo.RelPath) == 1 && edgeInfo.RelPath[""][""]) {
if len(edgeInfo.RelPath) == 0 || len(edgeInfo.RelPath) == 1 && edgeInfo.RelPath[""][""] {
nextNodeAccessPaths = cur.AccessPaths
}
if len(edgeInfo.RelPath) == 1 && edgeInfo.RelPath["*"][""] {
nextNodeAccessPaths = []string{""}
}
// No matching access paths for this edge
if len(nextNodeAccessPaths) == 0 {
return que
Expand Down
10 changes: 10 additions & 0 deletions analysis/taint/testdata/fields/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,15 @@ func testGeneratingPayloadWithTaintedData() {
sink(fmt.Sprintf("%s-%s", payload.MessageId, strconv.Itoa(rand.Int()))) // @Sink(testGenPayload)
}

func testGeneratingPayloadWithTaintedDataThroughEncoding() {
taintedId := source() // @Source(testGenPayloadThroughEnc)
payload, err := generatePayload(log.Default(), "m-1423", taintedId, "hello")
if err != nil {
panic("error")
}
sink(fmt.Sprintf("%s", string(payload.Payload))) //@Sink(testGenPayloadThroughEnc)
}

func main() {
testSimpleField1()
testSimpleField2()
Expand All @@ -499,4 +508,5 @@ func main() {
testTaintStructAsValue()
testStructWithFuncs()
testGeneratingPayloadWithTaintedData()
testGeneratingPayloadWithTaintedDataThroughEncoding()
}

0 comments on commit 0d71bdc

Please sign in to comment.