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

Minor SAST warnings #3419

Open
fyfyrchik opened this issue Apr 24, 2024 · 1 comment · May be fixed by #3455
Open

Minor SAST warnings #3419

fyfyrchik opened this issue Apr 24, 2024 · 1 comment · May be fixed by #3455
Assignees
Labels
bug Something isn't working I4 No visible changes S4 Routine security Affects security U2 Seriously planned
Milestone

Comments

@fyfyrchik
Copy link
Contributor

fyfyrchik commented Apr 24, 2024

Hello! I have run https://svace.pages.ispras.ru/svace-website/en/ against neo-go codebase and here are some of the issues found. I think most of them are OK (I ignored issues such as having empty set of oracles in the oracle post-persist)

After discussion of @roman-khimov, I decided to create it here, both seems minor.

  1. Here prevHeader could be nil in the else branch.
Message: After having been compared to a nil value at blockchain.go:2984, pointer 'prevHeader' is dereferenced at blockchain.go:2987.

if prevHeader == nil && currHeader.PrevHash.Equals(util.Uint256{}) {

  1. Here an attacker could use NeoFS protocol scheme to cause panic in production node without neofs support
Variable len(t67.MainCfg.NeoFS.Nodes), whose possible value set allows a zero value at request.go:162 by calling function 'builtin.len', is used as a denominator at request.go:162.

rc, err := neofs.Get(ctx, priv, u, o.MainCfg.NeoFS.Nodes[index])

@fyfyrchik fyfyrchik added bug Something isn't working U2 Seriously planned labels Apr 24, 2024
@roman-khimov roman-khimov added S4 Routine I4 No visible changes labels Apr 24, 2024
@roman-khimov roman-khimov added this to the v0.106.0 milestone Apr 24, 2024
@roman-khimov roman-khimov added the security Affects security label Apr 24, 2024
@roman-khimov
Copy link
Member

  1. prevHeader is never a nil in reality, but the code is bogus, I agree (since a6610ba).
  2. True, but requires a very specific (mis)configuration --- oracle service enabled, but no NeoFS nodes configured.

@roman-khimov roman-khimov modified the milestones: v0.106.0, v0.107.0 May 13, 2024
@AnnaShaleva AnnaShaleva modified the milestones: v0.107.0, v0.106.1 May 21, 2024
AliceInHunterland added a commit that referenced this issue May 21, 2024
Prevents prevHeader to be nil in the else branch.

Refs #3419

Signed-off-by: Ekaterina Pavlova <[email protected]>
AliceInHunterland added a commit that referenced this issue May 21, 2024
Prevent the risk of a division by zero error when accessing the
`o.MainCfg.NeoFS.Nodes[index]` array

Close #3419

Signed-off-by: Ekaterina Pavlova <[email protected]>
@AliceInHunterland AliceInHunterland linked a pull request May 21, 2024 that will close this issue
AliceInHunterland added a commit that referenced this issue May 21, 2024
Prevent the risk of a division by zero error when accessing the
`o.MainCfg.NeoFS.Nodes[index]` array.

Close #3419

Signed-off-by: Ekaterina Pavlova <[email protected]>
AliceInHunterland added a commit that referenced this issue May 21, 2024
prevHeader is never nil.

Refs #3419

Signed-off-by: Ekaterina Pavlova <[email protected]>
AliceInHunterland added a commit that referenced this issue May 21, 2024
Prevent the risk of a division by zero error when accessing the
`o.MainCfg.NeoFS.Nodes[index]` array.

Close #3419

Signed-off-by: Ekaterina Pavlova <[email protected]>
@roman-khimov roman-khimov modified the milestones: v0.106.1, v0.106.2 Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working I4 No visible changes S4 Routine security Affects security U2 Seriously planned
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants