Static Analyzer for the SDK

[ebuchman]

Things to check for:

• sensitive imports (unsafe, reflect, runtime, rand, etc.)
• errors are properly propagated (we don't ignore err != nil)
• integer casting/type conversions
• sources of non-determinism:
  • randomness (can be covered by checking rand imports)
  • iterating over a map
  • time.Now or equivalent
  • floats
  • go routines
• panics in Begin/EndBlock (these are allowed in tx handling, since they're handled by the SDK, but not in the Begin/EndBlock)

Other?

[odeke-em]

• missing Unlock when you have a mutex.Lock() for example

mu.Lock()
defer mu.Lock()

• an explicitly requested pass say by a comment before a function/method, that ensures that map iteration over account addresses and migrations happens deterministically to avoid such in fix: upgrade non-determinism cosmos-sdk#10188
• Uninitialized memory for maps for example:
  var m map[string]string
  ...
  m[key] = value
  @ebuchman let's please add my colleague @cuonglm as well

[ebuchman]

done.

If I'm not mistaken there should be relatively little locking in SDK modules since it's generally all single threaded

[odeke-em]

@ebuchman we've got a fix #5 for this huge problem cosmos/cosmos-sdk#10188 fixed by PR cosmos/cosmos-sdk#10189. I think we might get utility for the entire cosmos-sdk by releasing some of these.

[odeke-em]

@ebuchman I've documented our rules in #6

[tomtau]

Things to check for:

• sensitive imports (unsafe, reflect, runtime, rand, etc.)

• errors are properly propagated (we don't ignore err != nil)

• integer casting/type conversions

• sources of non-determinism:

  • randomness (can be covered by checking rand imports)
  • iterating over a map
  • time.Now or equivalent
  • floats
  • go routines

• panics in Begin/EndBlock (these are allowed in tx handling, since they're handled by the SDK, but not in the Begin/EndBlock)

Other?

One more item is using hardcoded constants instead of configuration values: this caused an issue (validators not being slashed) before cosmos/cosmos-sdk#8461 and resurfaced again on master cosmos/cosmos-sdk#9212 .

I independently started on this query collection: https://github.com/crypto-com/cosmos-sdk-codeql/tree/main/src
For most of the items, they should be easily doable in Gosec... The main items that may be tricky (or impossible) are ones requiring data flow (e.g. to reduce false positives, one can restrict the findings to values that get used in consensus-related code) or control flow (e.g. some function call that may panic reachable from Begin/Endblock): https://codeql.github.com/docs/codeql-language-guides/analyzing-control-flow-in-python/

[tomtau]

@ebuchman FYI https://github.com/crypto-com/cosmos-sdk-codeql/tree/main/src now has the Cosmos SDK-specific items discussed here.
error propagation should be captured by errcheck or the standard suite.
integer casting/type conversions should be captured by GitHub's security query suite: https://github.com/github/codeql-go/blob/a10407823ac30c9dff84e007bc155c49a9c4a0f3/ql/src/Security/CWE-681/IncorrectIntegerConversionQuery.ql