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.

Sign up for GitHub

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

Fix static analysis issues #480

Merged
merged 6 commits into from
Aug 20, 2024
Merged

Fix static analysis issues #480

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d7b1626
Solve potential file inclusion via variable
MatheusFranco99 Aug 14, 2024
e1fd5a8
Fix file permission (0644 to 0600)
MatheusFranco99 Aug 14, 2024
e41fc27
Add nosec comment for PRNG (pseudo-random number generator) used for …
MatheusFranco99 Aug 14, 2024
38e4009
Fix lint issue on nil check in []byte type
MatheusFranco99 Aug 14, 2024
5fe77f7
Update permission from 0444 to 0600
MatheusFranco99 Aug 15, 2024
07f8940
Update 0444 to 0400
MatheusFranco99 Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions qbft/spectest/generate/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,17 @@ package main
import (
"encoding/json"
"fmt"
"github.com/pkg/errors"
"github.com/ssvlabs/ssv-spec/qbft/spectest/tests"
comparable2 "github.com/ssvlabs/ssv-spec/types/testingutils/comparable"
"log"
"os"
"path/filepath"
"reflect"
"runtime"
"strings"

"github.com/pkg/errors"
"github.com/ssvlabs/ssv-spec/qbft/spectest/tests"
comparable2 "github.com/ssvlabs/ssv-spec/types/testingutils/comparable"

"github.com/ssvlabs/ssv-spec/qbft/spectest"
)

Expand Down Expand Up @@ -92,7 +93,7 @@ func writeJsonStateComparison(name, testType string, post interface{}) {

file := filepath.Join(scDir, fmt.Sprintf("%s.json", name))
log.Printf("writing state comparison json: %s\n", file)
if err := os.WriteFile(file, byts, 0644); err != nil {
if err := os.WriteFile(file, byts, 0600); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would update to 0444

The reason is that we don't want anyone writing to the file, to make tests pass/fail.
There's nothing confidential here so might as well let everyone read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I agree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, gosec still complains about 0444:

G306 (CWE-276): Expect WriteFile permissions to be 0600 or less (Confidence: HIGH, Severity: MEDIUM)

Can't it be 0400? Why do we need to allow "read" for group and others?

Copy link
Contributor

@GalRogozinski GalRogozinski Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

hmm
if CI passes then it can be 0400 honestly
I just think that there is no confidential information and the SSV repo generate jsons itself.

I just saw no sec concern so you can choose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also don't see security concerns, but gosec complains and I also don't see a reason for the group and others read permission.
Updating it to 0400

panic(err.Error())
}
}
Expand All @@ -119,7 +120,7 @@ func writeJson(data []byte) {

file := filepath.Join(basedir, "tests.json")
log.Printf("writing spec tests json to: %s\n", file)
if err := os.WriteFile(file, data, 0644); err != nil {
if err := os.WriteFile(file, data, 0600); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

panic(err.Error())
}
}
2 changes: 1 addition & 1 deletion qbft/spectest/tests/controller_spectest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ func (test *ControllerSpecTest) overrideStateComparison(t *testing.T) {
basedir = filepath.Join(basedir, "generate")
dir := typescomparable.GetSCDir(basedir, reflect.TypeOf(test).String())
path := filepath.Join(dir, fmt.Sprintf("%s.json", test.TestName()))
byteValue, err := os.ReadFile(path)
byteValue, err := os.ReadFile(filepath.Clean(path))
require.NoError(t, err)
sc := make([]*qbft.Controller, len(test.RunInstanceData))
require.NoError(t, json.Unmarshal(byteValue, &sc))
Expand Down
2 changes: 1 addition & 1 deletion ssv/runner_validations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func (b *BaseRunner) ValidatePostConsensusMsg(runner Runner, psigMsgs *types.Par
}

// TODO https://github.com/ssvlabs/ssv-spec/issues/142 need to fix with this issue solution instead.
if b.State.DecidedValue == nil || len(b.State.DecidedValue) == 0 {
if len(b.State.DecidedValue) == 0 {
return errors.New("no decided value")
}

Expand Down
13 changes: 7 additions & 6 deletions ssv/spectest/generate/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,18 @@ import (
"compress/gzip"
"encoding/json"
"fmt"
"github.com/pkg/errors"
"github.com/ssvlabs/ssv-spec/ssv/spectest/tests"
"github.com/ssvlabs/ssv-spec/types"
comparable2 "github.com/ssvlabs/ssv-spec/types/testingutils/comparable"
"log"
"os"
"path/filepath"
"reflect"
"runtime"
"strings"

"github.com/pkg/errors"
"github.com/ssvlabs/ssv-spec/ssv/spectest/tests"
"github.com/ssvlabs/ssv-spec/types"
comparable2 "github.com/ssvlabs/ssv-spec/types/testingutils/comparable"

"github.com/ssvlabs/ssv-spec/ssv/spectest"
)

Expand Down Expand Up @@ -112,7 +113,7 @@ func writeSingleSCJson(path string, testType string, post interface{}) {
}

log.Printf("writing state comparison json: %s\n", file)
if err := os.WriteFile(file, byts, 0644); err != nil {
if err := os.WriteFile(file, byts, 0600); err != nil {
panic(err.Error())
}
}
Expand Down Expand Up @@ -156,7 +157,7 @@ func writeJson(data []byte) {
}

// Write the gzipped data to a file
if err := os.WriteFile(file, buf.Bytes(), 0644); err != nil {
if err := os.WriteFile(file, buf.Bytes(), 0600); err != nil {
panic(err.Error())
}
}
9 changes: 5 additions & 4 deletions types/spectest/generate/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ package main
import (
"encoding/json"
"fmt"
"github.com/ssvlabs/ssv-spec/types/spectest"
comparable2 "github.com/ssvlabs/ssv-spec/types/testingutils/comparable"
"log"
"os"
"path/filepath"
"reflect"
"runtime"
"strings"

"github.com/ssvlabs/ssv-spec/types/spectest"
comparable2 "github.com/ssvlabs/ssv-spec/types/testingutils/comparable"
)

//go:generate go run main.go
Expand Down Expand Up @@ -81,7 +82,7 @@ func writeJsonStateComparison(name, testType string, post interface{}) {

file := filepath.Join(scDir, fmt.Sprintf("%s.json", name))
log.Printf("writing state comparison json: %s\n", file)
if err := os.WriteFile(file, byts, 0644); err != nil {
if err := os.WriteFile(file, byts, 0600); err != nil {
panic(err.Error())
}
}
Expand All @@ -108,7 +109,7 @@ func writeJson(data []byte) {
file := filepath.Join(basedir, "tests.json")

fmt.Printf("writing spec tests json to: %s\n", file)
if err := os.WriteFile(file, data, 0644); err != nil {
if err := os.WriteFile(file, data, 0600); err != nil {
panic(err.Error())
}
}
14 changes: 8 additions & 6 deletions types/testingutils/comparable/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,16 @@ package comparable
import (
"encoding/json"
"fmt"
"os"
"path/filepath"
"strings"
"testing"

spec2 "github.com/attestantio/go-eth2-client/spec"
ssz "github.com/ferranbt/fastssz"
"github.com/google/go-cmp/cmp"
"github.com/ssvlabs/ssv-spec/types"
"github.com/stretchr/testify/require"
"os"
"path/filepath"
"strings"
"testing"
)

func NoErrorEncoding(obj ssz.Marshaler) []byte {
Expand Down Expand Up @@ -46,7 +47,8 @@ func UnmarshalStateComparison[T types.Root](basedir string, testName string, tes
basedir = filepath.Join(basedir, "generate")
scDir := GetSCDir(basedir, testType)
path := filepath.Join(scDir, fmt.Sprintf("%s.json", testName))
byteValue, err := os.ReadFile(path)

byteValue, err := os.ReadFile(filepath.Clean(path))
if err != nil {
return nilT, err
}
Expand All @@ -64,7 +66,7 @@ func readStateComparison(basedir string, testName string, testType string) (map[
basedir = filepath.Join(basedir, "generate")
scDir := GetSCDir(basedir, testType)
path := filepath.Join(scDir, fmt.Sprintf("%s.json", testName))
byteValue, err := os.ReadFile(path)
byteValue, err := os.ReadFile(filepath.Clean(path))
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion types/testingutils/shuffle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func getNonEmptyListsIndices(lists [][]interface{}) []int {
func MergeListsWithRandomPick(lists [][]interface{}) []interface{} {

seed := int64(42)
rand := rand.New(rand.NewSource(seed))
rand := rand.New(rand.NewSource(seed)) // #nosec G404 --This PRNG is only used for testing. No cryptographically secure one allows seeding, which is useful so tests won't change
result := make([]interface{}, 0)

// Continue until all lists are empty
Expand Down