Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

rules/sdk: add pass to reject time.Now() #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

odeke-em
Copy link
Collaborator

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 odeke-em force-pushed the reject-time.Now branch 8 times, most recently from 3245b93 to 1d54e29 Compare November 24, 2021 18:03
@odeke-em odeke-em force-pushed the master branch 4 times, most recently from 2c09f88 to 324096f Compare August 8, 2022 02:55
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

Is this scoped to non _test.go files?

@odeke-em odeke-em force-pushed the reject-time.Now branch 2 times, most recently from ae8827b to 9eac551 Compare September 2, 2022 07:24
@@ -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},

Choose a reason for hiding this comment

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

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"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you @johnsaigle! Done, please take a look!

@@ -95,6 +95,10 @@ var _ = Describe("gosec rules", func() {
runner("G705", testutils.SampleCodeMapRangingNonDeterministic)
})

It("should detect non-consensus aware time.Now() usage", func() {

Choose a reason for hiding this comment

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

Similar to the above, I think this could be worded in a way that's more clear

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you @johnsaigle! Done.

@@ -2528,4 +2528,23 @@ func noop(keys []string) []string {return keys}
`}, 13, gosec.NewConfig(),
},
}

SampleCodeTimeNowNonCensusAware = []CodeSample{

Choose a reason for hiding this comment

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

Suggested change
SampleCodeTimeNowNonCensusAware = []CodeSample{
SampleCodeTimeNowNonConsensusAware = []CodeSample{

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you @johnsaigle, done and please take a look again!

rules/rules_test.go Outdated Show resolved Hide resolved
@odeke-em odeke-em force-pushed the reject-time.Now branch 2 times, most recently from 225257f to 1b5bf8b Compare September 24, 2022 02:21
@@ -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() {
Copy link
Member

Choose a reason for hiding this comment

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

this may fire false positives in tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link

@johnsaigle johnsaigle left a comment

Choose a reason for hiding this comment

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

Looks good!

There seems to be unit test failures in the CI though

@okdas
Copy link

okdas commented Aug 30, 2024

Hey @odeke-em 👋 - any reason this was not merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants