Go: False positive when use sync.Map

[sud0why]

Description

I think the sync.Map.Store operation should not propagate the key to the sync.Map, and the same applies to sync.Map.LoadOrStore and sync.Map.Swap. This could lead to situations where keys might propagate into values through the sync.Map.

Modifications may be needed in the ext/sync.model.yml model definition file within the go-all library.

Example Code

package mytest

import (
	"fmt"
	"net/http"
	"sync"

	"gorm.io/gorm"
)

var myMap sync.Map
var db gorm.DB

func handler(req *http.Request) {
	input := req.URL.Query().Get("input")
	value := getData(input)
	fmt.Println(value)
}

func process() {
	key := "hello"
	value := getData(key)
	db.Exec(value)
}

func getData(key string) string {
	if value, ok := myMap.Load(key); ok {
		return value.(string)
	}
	value := "hello"
	myMap.Store(key, value)
	return value
}

Result

[mbg]

Hi @sud0why 👋🏻

Thanks for reporting this! That does look like a FP to me. I am guessing that we don't currently have enough precision here to distinguish between the keys and values in the map during our taint flow analysis, so conservatively mark the whole map as tainted if either a tainted key or value is stored in it. We will track this issue internally, and will fix it when possible. However, fixing FPs isn't currently a product priority, so I can't say how quickly this may get fixed.

[sud0why]

Hi @sud0why 👋🏻

Thanks for reporting this! That does look like a FP to me. I am guessing that we don't currently have enough precision here to distinguish between the keys and values in the map during our taint flow analysis, so conservatively mark the whole map as tainted if either a tainted key or value is stored in it. We will track this issue internally, and will fix it when possible. However, fixing FPs isn't currently a product priority, so I can't say how quickly this may get fixed.

I believe keys should not propagate through the map. The only way to achieve key propagation via sync.Map would be through traversing the entire map using sync.Map.Range. However, our current implementation in ext/sync.model.yml does not yet support sync.Map.Range, which means all existing propagations are limited to values.

Perhaps modifying the taint source parameters for sync.Map methods like LoadOrStore, Store, and Swap—specifically changing from Argument[0..1] to Argument[1]—would be sufficient to address this issue?

[mbg]

Yes, that's right. We do not currently model Range so there's no current way to extract keys from a sync.Map. My colleague has put together a PR to change the models so we don't consider taint flow for keys for the moment, which we have merged. You should see this FP disappear once that change is released.