fix: all: avoid map ranging whenever possible to fix non-determinism security warnings #13554

odeke-em · 2022-10-15T04:12:53Z

Fixes errors reported by cosmos/gosec for non-deterministic map ranging, but using golang.org/x/exp/maps.(Keys, Values) where necessary.

…security warnings Fixes errors reported by cosmos/gosec for non-deterministic map ranging, but using golang.org/x/exp/maps.(Keys, Values) where necessary.

store/rootmulti/store.go

+	for key := range rs.removalMap {
+		keys = append(keys, key)
+	}


store/rootmulti/store.go

+	for key := range rs.stores {
+		keys = append(keys, key)
+	}


alexanderbez

Can we be consistent with using maps.Keys inline with the range rather than creating a new variable (unless necessary, i.e. keys used more than range) 🙏 ?

peterbourgon

Go maps are a core language primitive, used in almost every Go program. They are also defined by the language spec to have a non-deterministic iteration order. I don't think it's reasonable to assert that the entirely normal, idiomatic, and ubiquitous expression for k, v := range m is somehow a problem. All Go programs are going to do this.

If a specific bit of code needs range determinism, that's fine! But that code needs to ensure that invariant explicitly. It doesn't seem correct to refactor all map range exprs in the whole project — extract the keys, sort them, iterate them, and then look up the corresponding val in the map — just by default. Determinism is the exception, not the rule.

peterbourgon · 2022-10-17T15:59:50Z

x/authz/migrations/v046/store.go

+	keys := maps.Keys(queueItems)
+	sort.Strings(keys)
+	for _, key := range keys {


There should be no reason to sort the return value of maps.Keys.

There is an absolute reason to, maps.Keys returns them in non-deterministic order and the source code shows it https://cs.opensource.google/go/x/exp/+/32f3d567:maps/maps.go;l=10

Surprising! But if map.Keys doesn't sort the keys, then the dependency provides very little value, right? It would be better to define your own package map with a Keys that returned ordered keys, I guess?

peterbourgon · 2022-10-17T16:02:39Z

x/upgrade/plan/info.go

+	// Ensure that downloads proceed in a deterministic pattern.
+	osArches := maps.Keys(m)
+	sort.Strings(osArches)


Likewise above.

Nope, we need these deterministic.

Is that requirement documented somewhere?

peterbourgon · 2022-10-17T16:03:29Z

x/upgrade/plan/info.go

+	// Ensure that downloads proceed in a deterministic pattern.
+	osArches := maps.Keys(m)
+	sort.Strings(osArches)
+	for _, osArch := range osArches {
+		url := m[osArch]


Suggested change

// Ensure that downloads proceed in a deterministic pattern.

osArches := maps.Keys(m)

sort.Strings(osArches)

for _, osArch := range osArches {

url := m[osArch]

for _, k := range maps.Keys(m) {

url := m[k]

odeke-em · 2022-10-29T04:18:16Z

@tac0turtle how come you closed this change? @peterbourgon provided feedback but it wasn't entirely correct about maps.Keys not needing sorting as we need determinism per https://cs.opensource.google/go/x/exp/+/32f3d567:maps/maps.go;l=10

which as you can see just iterates over keys. We want those values sorted.

odeke-em · 2022-10-29T04:21:05Z

Go maps are a core language primitive, used in almost every Go program. They are also defined by the language spec to have a non-deterministic iteration order. I don't think it's reasonable to assert that the entirely normal, idiomatic, and ubiquitous expression for k, v := range m is somehow a problem. All Go programs are going to do this.

If a specific bit of code needs range determinism, that's fine! But that code needs to ensure that invariant explicitly. It doesn't seem correct to refactor all map range exprs in the whole project — extract the keys, sort them, iterate them, and then look up the corresponding val in the map — just by default. Determinism is the exception, not the rule.

Thanks @peterbourgon for the initial review! So parts of this code does need determinism, please read the PR much closer and look at the directories, there are 5 usages of sort. out of the 19 files and that code is in super sensitive order code like store/v* for which writes to databases should have a deterministic ordering. Let's reopen this @tac0turtle

julienrbrt · 2022-10-29T19:30:16Z

Re-opening as there is discussion here.

peterbourgon

If maps.Keys and maps.Values don't sort their predicates, then I guess there are some problems here? I might be misunderstanding something!

peterbourgon · 2022-10-30T02:44:52Z

baseapp/baseapp.go

@@ -230,7 +230,7 @@ func (app *BaseApp) MountStores(keys ...storetypes.StoreKey) {
 // MountKVStores mounts all IAVL or DB stores to the provided keys in the
 // BaseApp multistore.
 func (app *BaseApp) MountKVStores(keys map[string]*storetypes.KVStoreKey) {
-	for _, key := range keys {
+	for _, key := range maps.Values(keys) {


Values returns the values of the map m. The values will be in an indeterminate order.

Does this change solve a problem?

For some code here such as this one, the values can be returned non-deterministically. The critical ones, I invoke sort.* on them :-)

naive question, why is map.Values and map.Keys needed here? does it add anything the previous code didnt?

Sure: for code that's okay with being non-deterministic, they mask the fact that we are ranging over maps given that the static analyzer rule is rigid. We could add a comment that it is okay to range over them but that'll pollute lots of code so just easier to use maps.Values and maps.Key

I still don't understand sorry. it seems like this adds an extra loop of the map to grab the keys or values. It seems this fixes the linter but doesn't actually fix any logic for us. this doesn't seem to sort like the title suggests.

Id prefer not to add things like this if its not fixing an issue we are facing.

FWIW, that static analyzer is bogus and should be removed.

@peterbourgon that's quite strong language most definitely from lack of context/ignorance of the history, but the reaasoning is there was a serious chain halt that happened late last year due to non-deterministic map ranging https://twitter.com/thorchain/status/1459288646142492673 and https://twitter.com/thorchain/status/1459288646142492673 and the cosmos-sdk and other projects have been plagued by such bugs due to non-deterministic map ranging.

@tac0turtle yeah it requires a balance but sadly would require annotations or perhaps just scoped to specific packages, but we need to figure things out. Also this change is a Draft btw folks, not yet ready for review.

The bugs on chains aren't from map ranging but from non-deterministic ordering across nodes in the network, which i dont see how this fixes that issue. These functions are loops with appends of keys or values to an array, not sorting. We can stop commenting but this feels unnecessary

@peterbourgon that's quite strong language most definitely from lack of context/ignorance of the history, but the reaasoning is there was a serious chain halt that happened late last year due to non-deterministic map ranging https://twitter.com/thorchain/status/1459288646142492673 and https://twitter.com/thorchain/status/1459288646142492673 and the cosmos-sdk and other projects have been plagued by such bugs due to non-deterministic map ranging.

I'm aware of the chain halt issues you're referring to. I was directly affected by them! 😅 The problem was not that range over a map is non-deterministic. The problem was the assumption, made by the programmer who wrote the code, that ranging over a map would be — or could be! — deterministic. Any code that needs deterministic iteration order over a map-like collection can't use the builtin map type directly. It either has to extract and sort the keys manually, or use a different type that provides this property.

And it's totally fine to extract the keys from a map, sort them, and iterate the map based on that slice, on an as-needed basis. Perfect, no problem. But this is a special case, it's not the baseline. That ranging over a map is nondeterministic is simply a property of the language. If you write Go, you need to understand that property, same as anything else, like that that switch statements don't fallthrough by default, or that you can read from a zero-value map but not write to it, or etc. etc. These things aren't problems that need to be — or even can be! — corrected by linters. They're the rules of the game.

peterbourgon · 2022-10-30T02:45:03Z

baseapp/baseapp.go

@@ -244,7 +244,8 @@ func (app *BaseApp) MountKVStores(keys map[string]*storetypes.KVStoreKey) {
 // MountTransientStores mounts all transient stores to the provided keys in
 // the BaseApp multistore.
 func (app *BaseApp) MountTransientStores(keys map[string]*storetypes.TransientStoreKey) {
-	for _, key := range keys {
+	keyL := maps.Values(keys)


Values returns the values of the map m. The values will be in an indeterminate order.

Does this change solve a problem?

What problem does it solve? Is it to silence the linter warnings about nondeterinistic iteration order? This change does seem to silence the linter, but it doesn't change the behavior, right? The order is still nondeterministic.

peterbourgon · 2022-10-30T02:45:45Z

client/v2/autocli/flag/register.go

+	names := maps.Keys(commandOptions.FlagOptions)
+	for _, name := range names {


Keys returns the keys of the map m. The keys will be in an indeterminate order.

Does this change solve a problem?

peterbourgon · 2022-10-30T02:48:03Z

client/v2/autocli/query.go

+	// Ensure that building module query commands is in a deterministic pattern.
+	moduleNames := maps.Keys(moduleOptions)
+	sort.Strings(moduleNames)
+	for _, moduleName := range moduleNames {


Is it documented somewhere that BuildQueryCommand should iterate over module names deterministically? If not, is it necessary to do so? If so, is there a test which enforces this invariant? And isn't manual sorting of keys somewhat error prone? Should there rather be a package/function that ensures deterministic order without requiring explicit sorting at the callsite?

peterbourgon · 2022-10-30T02:52:40Z

x/upgrade/plan/info.go

+	// Ensure that downloads proceed in a deterministic pattern.
+	osArches := maps.Keys(m)
+	sort.Strings(osArches)


Is that requirement documented somewhere?

peterbourgon · 2022-10-30T02:57:56Z

store/rootmulti/store.go

-	for key, store := range rs.stores {
+	keys := make([]types.StoreKey, 0, len(rs.stores))
+	for key := range rs.stores {
+		keys = append(keys, key)
+	}
+	for _, key := range keys {
+		store := rs.stores[key]


This code isn't sorting the keys, so AFAICT doesn't produce behavior any different than ranging over the map directly?

peterbourgon · 2022-10-30T02:58:20Z

x/gov/keeper/tally.go

-	for _, val := range currValidators {
+	valL := maps.Values(currValidators)
+	for _, val := range valL {
 		if len(val.Vote) == 0 {
 			continue
 		}


This code isn't sorting the values, so AFAICT doesn't produce behavior any different than ranging over the map directly?

peterbourgon · 2022-10-30T02:58:35Z

x/params/types/table.go

 func (t KeyTable) maxKeyLength() (res int) {
-	for k := range t.m {
+	for _, k := range maps.Keys(t.m) {
 		l := len(k)
 		if l > res {


This code isn't sorting the keys, so AFAICT doesn't produce behavior any different than ranging over the map directly?

peterbourgon · 2022-10-30T02:58:50Z

x/staking/keeper/val_state_change.go

-	for valAddrStr := range last {
+	valAddrL := maps.Keys(last)
+	for _, valAddrStr := range valAddrL {


This code isn't sorting the keys, so AFAICT doesn't produce behavior any different than ranging over the map directly?

peterbourgon · 2022-10-30T02:59:02Z

x/upgrade/plan/info.go

-	for key, val := range m {
+	for _, key := range maps.Keys(m) {
 		if key != "any" && !osArchRx.MatchString(key) {
 			return fmt.Errorf("invalid os/arch format in key \"%s\"", key)
 		}
+
+		val := m[key]


This code isn't sorting the keys, so AFAICT doesn't produce behavior any different than ranging over the map directly?

tac0turtle · 2022-11-19T08:07:37Z

Checking in on status of this pr? Should we take it over?

tac0turtle · 2022-12-02T20:04:24Z

Hey the core sdk Team discussed closing prs which have been inactive for 30 days.

I'll close this for now. Let us know if you need help with any review in the future

github-actions bot added C:CLI C:orm C:Store C:x/authz C:x/gov C:x/params C:x/staking C:x/upgrade labels Oct 15, 2022

odeke-em force-pushed the gosec-fix-map-iteration-non-determinism-warnings branch 2 times, most recently from 5ba0e80 to 88e53e5 Compare October 15, 2022 04:14

odeke-em changed the title ~~all: avoid map ranging whenever possible to fix non-determinism security warnings~~ fix: all: avoid map ranging whenever possible to fix non-determinism security warnings Oct 15, 2022

odeke-em force-pushed the gosec-fix-map-iteration-non-determinism-warnings branch from 88e53e5 to 378a971 Compare October 15, 2022 04:15

fix: all: avoid map ranging whenever possible to fix non-determinism …

f28f5f4

…security warnings Fixes errors reported by cosmos/gosec for non-deterministic map ranging, but using golang.org/x/exp/maps.(Keys, Values) where necessary.

odeke-em force-pushed the gosec-fix-map-iteration-non-determinism-warnings branch from 378a971 to f28f5f4 Compare October 15, 2022 04:31

github-advanced-security bot found potential problems Oct 15, 2022

View reviewed changes

alexanderbez reviewed Oct 16, 2022

View reviewed changes

peterbourgon suggested changes Oct 17, 2022

View reviewed changes

tac0turtle closed this Oct 28, 2022

julienrbrt reopened this Oct 29, 2022

peterbourgon reviewed Oct 30, 2022

View reviewed changes

tac0turtle closed this Dec 2, 2022

julienrbrt mentioned this pull request Dec 13, 2022

Audit Types v0.46..v0.47 #14257

Closed

tac0turtle deleted the gosec-fix-map-iteration-non-determinism-warnings branch April 14, 2023 10:25

odeke-em mentioned this pull request Dec 7, 2023

EPIC: determinism with maps #13039

Closed

		names := maps.Keys(commandOptions.FlagOptions)
		for _, name := range names {

fix: all: avoid map ranging whenever possible to fix non-determinism security warnings #13554

fix: all: avoid map ranging whenever possible to fix non-determinism security warnings #13554

Conversation

odeke-em commented Oct 15, 2022

alexanderbez left a comment

Choose a reason for hiding this comment

peterbourgon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odeke-em commented Oct 29, 2022

odeke-em commented Oct 29, 2022

julienrbrt commented Oct 29, 2022

peterbourgon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbourgon Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tac0turtle commented Nov 19, 2022

tac0turtle commented Dec 2, 2022

peterbourgon left a comment •

edited

Loading

peterbourgon Nov 2, 2022 •

edited

Loading