rules/sdk: add pass to reject time.Now() #12
Conversation
odeke-em
force-pushed
the
reject-time.Now
branch
8 times, most recently
from
3245b93
to
1d54e29
Compare
odeke-em
force-pushed
the
master
branch
4 times, most recently
from
2c09f88
to
324096f
Compare
odeke-em
force-pushed
the
reject-time.Now
branch
2 times, most recently
from
ae8827b
to
9eac551
Compare
rules/rulelist.go
Outdated
| @@ -117,6 +117,7 @@ func Generate(filters ...RuleFilter) RuleList { | |||
| {"G703", "Errors that don't result in rollback", sdk.NewErrorNotPropagated}, | |||
| {"G704", "Strconv invalid bitSize and cast", sdk.NewStrconvIntBitSizeOverflow}, | |||
| {"G705", "Iterating over maps undeterministically", sdk.NewMapRangingCheck}, | |||
| {"G706", "Non-consensus aware time.Now() usage", sdk.NewTimeNowRefusal}, | |||
There was a problem hiding this comment.
I think this would be better written as "Non consensus-aware time.Now() usage" or perhaps more helpfully something like "Use of time.Now() in consensus code could lead to chain halt"
There was a problem hiding this comment.
Thank you @johnsaigle! Done, please take a look!
rules/rules_test.go
Outdated
| @@ -95,6 +95,10 @@ var _ = Describe("gosec rules", func() { | |||
| runner("G705", testutils.SampleCodeMapRangingNonDeterministic) | |||
| }) | |||
|
|
|||
| It("should detect non-consensus aware time.Now() usage", func() { | |||
There was a problem hiding this comment.
Similar to the above, I think this could be worded in a way that's more clear
There was a problem hiding this comment.
Thank you @johnsaigle! Done.
testutils/source.go
Outdated
| @@ -2528,4 +2528,23 @@ func noop(keys []string) []string {return keys} | |||
| `}, 13, gosec.NewConfig(), | |||
| }, | |||
| } | |||
|
|
|||
| SampleCodeTimeNowNonCensusAware = []CodeSample{ | |||
There was a problem hiding this comment.
| SampleCodeTimeNowNonCensusAware = []CodeSample{ | |
| SampleCodeTimeNowNonConsensusAware = []CodeSample{ |
There was a problem hiding this comment.
Thank you @johnsaigle, done and please take a look again!
odeke-em
force-pushed
the
reject-time.Now
branch
2 times, most recently
from
225257f
to
1b5bf8b
Compare
| @@ -95,6 +95,10 @@ var _ = Describe("gosec rules", func() { | |||
| runner("G705", testutils.SampleCodeMapRangingNonDeterministic) | |||
| }) | |||
|
|
|||
| It("should detect use of time.Now() in consensus code which could lead to chain halt", func() { | |||
There was a problem hiding this comment.
this may fire false positives in tests
There was a problem hiding this comment.
gosec skips *_test.go unless explicitly enabled. Do you mean we should avoid this in /testutil/? One thing we can do is scope it to x/* code perhaps
There was a problem hiding this comment.
ah okay didnt know this. I think should be fine then
time.Now() uses local clocks that unfortunately when used in distributed consensus introduce lots of skew and can be exploited in vulnerabilities. Instead there is consensus aware clock whose timestamp is embedded in the current Block. This pass rejects usages as per https://forum.cosmos.network/t/cosmos-sdk-vulnerability-retrospective-security-advisory-jackfruit-october-12-2021/5349 Updates #1
odeke-em
force-pushed
the
reject-time.Now
branch
from
1b5bf8b
to
308efee
Compare
|
Hey @odeke-em 👋 - any reason this was not merged? |
time.Now() uses local clocks that unfortunately when used
in distributed consensus introduce lots of skew and
can be exploited in vulnerabilities. Instead there is
consensus aware clock whose timestamp is embedded in
the current Block. This pass rejects usages as per
https://forum.cosmos.network/t/cosmos-sdk-vulnerability-retrospective-security-advisory-jackfruit-october-12-2021/5349
Updates #1