Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Go: Do not track taint into a sync.Map via the key of a key-value pair #18920

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented Mar 4, 2025

There is no way of reading the keys stored in the sync.Map back.

owen-mc added 2 commits March 4, 2025 12:11
There is no way to get the value of a key out of a `sync.Map`.
@Copilot Copilot bot review requested due to automatic review settings March 4, 2025 12:15
@owen-mc owen-mc requested a review from a team as a code owner March 4, 2025 12:15

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This pull request updates the taint tracking rules for Go’s sync.Map by no longer tracking taint through the key of a key-value pair, and it removes tests that validate key-based taint propagation.

  • Updated change note documenting the new behavior.
  • Modified sync.Map model configuration to track only the value argument for LoadOrStore, Store, and Swap.
  • Removed tests that previously witnessed taint flow from sync.Map keys.

Reviewed Changes

File Description
go/ql/lib/change-notes/2025-03-04-improve-models-for-sync-map.md Added change note explaining the updated taint behavior for sync.Map.
go/ql/lib/ext/sync.model.yml Adjusted taint tracking configuration to ignore the key by switching from tracking both key and receiver to tracking only the value argument.
go/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/Sync.go Removed tests that expected key-based taint flow for sync.Map.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

go/ql/lib/ext/sync.model.yml:9

  • Verify that switching the taint propagation from "Argument[0..1]" to "Argument[1]" for LoadOrStore aligns with the intended behavior, ensuring that no necessary taint information is lost.
      - ["sync", "Map", True, "LoadOrStore", "", "", "Argument[1]", "Argument[receiver]", "taint", "manual"]

go/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/Sync.go:16

  • [nitpick] Since tests related to key-based taint propagation have been removed, consider adding or updating tests to validate that taint only flows through the value side of sync.Map operations.
func TaintStepTest_SyncMapLoadOrStore_B1I0O0(sourceCQL interface{}) interface{} {

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

@mbg
Copy link
Member

mbg commented Mar 4, 2025

There is no way of reading the keys stored in the sync.Map back.

https://pkg.go.dev/sync#Map.Range seems to allow reading all the keys in a sync.Map, so I think we should probably continue tracking which keys are tainted.

@owen-mc
Copy link
Contributor Author

owen-mc commented Mar 4, 2025

Hmm. But we don't model Range in any way, so we wouldn't track taint flow through it anyway. In the future we might, but we can always switch to synthetic fields for key and value when that happens.

Co-authored-by: Michael B. Gale <mbg@github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants