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: is it pedantic to reject reflect, runtime, math/rand, crypto/rand? #2

Open
odeke-em opened this issue Oct 18, 2021 · 3 comments
Assignees

Comments

@odeke-em
Copy link
Collaborator

The purpose of this issue is to ask if we are being pedantic about some of these imports:

If we look at https://github.com/informalsystems/gosec/blob/a284576b08668f2835734b359b27faea587a98b1/rules/sdk/blocklist.go#L71-L78

we can see a bunch of critical packages rejected, but I see lots of valid uses for a bunch of them like:
a) reflect: used in the SDK when interfaces are passed into say (*BaseApp).MountStores to return a correct error message indicating that an interface wasn't accepted -- this one is okay to flag as wrong per cosmos/cosmos-sdk#10392 but there are so many instances where using reflect is warranted like https://github.com/cosmos/cosmos-sdk/search?q=reflect
b) runtime is used in cosmosvisor to retrieve the operating system and architecture as well as for finding stack traces per https://github.com/cosmos/cosmos-sdk/search?q=runtime
c) math/rand: we use rand seeded to generate random numbers
d) crypto/rand: we use rand to generate private keys in crypto/keys/itnernal/ecdsa in GenPrivKey

@ebuchman
Copy link
Member

ebuchman commented Nov 9, 2021

so i think anywhere we're using this stuff we should flag it. in general there will surely be reflect and possibly runtime in the core, but I think these linters are more geared for the /x modules (tho we should probably use them on the core too).

Any use of rand is suspect and should really only exist in tests since if we generate a random number in the blockchain app we'll have non-determinism.

@ebuchman
Copy link
Member

ebuchman commented Nov 9, 2021

These tools are going to generate lots of false positives and there's really no easy way around that so maybe there's some kind of process/tooling we can add to set certain instances to ignore or something ?

I guess we could add annotations in the code but that's a bit of a pain

@tomtau
Copy link

tomtau commented Nov 16, 2021

if it integrates with Code Scanning on GH: https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning I guess this can be set with properties.precision: low and properties.problem.severity: warning, so at least on the Security tabs, it's ordered not to show up at the top and one can dismiss it as a false positive: https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/managing-code-scanning-alerts-for-your-repository#dismissing-or-deleting-alerts

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

No branches or pull requests

3 participants