Fix static analysis issues #480

MatheusFranco99 · 2024-08-14T10:25:29Z

Overview

This PR adds fixes for static analysis issues including:

file permission issues
file inclusion via variable issues

It also adds a nosec comment for a cryptographically insecure PRNG (pseudo-random number generator) used in tests. We use this PRNG (math/rand) since the secure crypto/rand doesn't allow seeding (which is important for consistent tests).

An existing issue with implicit memory aliasing in a for loop will be automatically fixed by the go update to 1.22.

…testing

GalRogozinski · 2024-08-15T07:29:52Z

qbft/spectest/generate/main.go

@@ -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 {


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

Yup, I agree

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?

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

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

GalRogozinski · 2024-08-15T07:30:03Z

qbft/spectest/generate/main.go

@@ -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 {


GalRogozinski · 2024-08-15T07:44:15Z

ssv/spectest/generate/main.go

@@ -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, 0444); err != nil {


GalRogozinski · 2024-08-15T07:44:26Z

ssv/spectest/generate/main.go

@@ -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(), 0444); err != nil {


GalRogozinski · 2024-08-15T07:48:28Z

types/spectest/generate/main.go

@@ -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, 0444); err != nil {


GalRogozinski

just the permissions

* Solve potential file inclusion via variable * Fix file permission (0644 to 0600) * Add nosec comment for PRNG (pseudo-random number generator) used for testing * Fix lint issue on nil check in []byte type * Update permission from 0444 to 0600 * Update 0444 to 0400

* remove domaintype from committeeMember * remove domaintype from share * add config for ssv containing domainType * add custom unmarshal for baserunner to avoid error in unmarshalling config * move domainType to Network * add testing domain in NewTestingNetwork function * Spec - filter committee duties per slot validator duties (#467) * Filter shares for slot `CommitteeRunner` based on validators that have duty for that slot. * Filter duty and create share map according to owned validators * Add test: start duty with no shares for duty's validators * Add test: happy flow for committee with fraction of duty's validators * Generate JSON tests * Apply suggestions --------- Co-authored-by: MatheusFranco99 <48058141+MatheusFranco99@users.noreply.github.com> * Spec - check att and sync duties exist before submitting (#468) * Meta - Update to go1.22 (#474) * Update go1.20 to go1.22 * Update go.sum with mod tidy * Meta - Update dependencies (#483) * Update dependencies * Fix lint issue * Generate JSON tests to trigger actions * Update fastssz * Generate JSON tests and align ssz error * Revert go-eth2-client version change * Revert fastssz upgrade * Generate SSZ and JSON tests * Meta - Fix static analysis issues (#480) * Solve potential file inclusion via variable * Fix file permission (0644 to 0600) * Add nosec comment for PRNG (pseudo-random number generator) used for testing * Fix lint issue on nil check in []byte type * Update permission from 0444 to 0600 * Update 0444 to 0400 * Meta - Drop unnecessary nolint comments (#477) * Remove nolint comment and export timeout variables * Drop unnecessary nolint * Add comment * Fix lint issue * Spec - Share length validation (#478) * Add share length validation in runner construction * Align to error handling in runners constructions * Add validation to committee runner * Add runner construction tests * Refactor runner construction in testingutil to deal with creation errors * Generate JSON tests * Fix lint issue * Fix comments * Meta - Drop redundant error (#475) * Spec - Drop redundant validation for decided messages (#476) * Remove redundant validation * Align error string * Spec - Sort signers in decided message (#484) * Sort signers in decided message * Add test for sorted signers in decided msg * Generate JSON tests * Fix lint issue * Spec - Stop processing after decided (#487) * Stop processing consensus messages after instance is decided * Align error in qbft tests * Align errors in ssv tests * Generate JSON tests * Fix lint issue * Spec - Drop leftover error check (#469) * Remove leftover err check * Align argument variable name to type * Spec - Secure key storage (#481) * Implement secure key storage using PKCS8 * Fix lint issue * Switch back PKCS8 to PKCS1 * Meta - Remove residual DKG (#502) * Remove DKG signature type * Remove DKG msg type * Remove DKGOperators field from TestKeySet * Remove unused ecdsaKeys field from TestingKeyStorage * Remove unused "ecdsaSKFromHex" function * Generate JSON tests * Spec - Add GoSec action and fix issues (#505) * Add github action and makefile command * Fix issues in round robin proposer function * Fix bad PutUint32 in GetCommitteeID * Fix issue with HasQuorum and HasPartialQuorum * Add role sanitization in GetRoleType and NewMessageType * Add sanitization to BeaconNetwork methods * Add sanitization in testingutils * Add sanitization to height usage in test files * Fix uint64 conversion in runner/postconsensus/valid_msg test * Sanitize ValidatorIndex conversion * Update action name * Fix tests to use valid RunnerRoles * Generate SSZ * Generate JSON tests * Revert the change on GetCommitteeID * Add nosec G115 to GetCommitteeID * revert the merge because it was merged with origin main by accident --------- Co-authored-by: rehs0y <lyosher@gmail.com> Co-authored-by: MatheusFranco99 <48058141+MatheusFranco99@users.noreply.github.com>

MatheusFranco99 added 3 commits August 14, 2024 11:17

Solve potential file inclusion via variable

d7b1626

Fix file permission (0644 to 0600)

e1fd5a8

Add nosec comment for PRNG (pseudo-random number generator) used for …

e41fc27

…testing

MatheusFranco99 requested a review from GalRogozinski August 14, 2024 10:25

MatheusFranco99 self-assigned this Aug 14, 2024

Fix lint issue on nil check in []byte type

38e4009

GalRogozinski reviewed Aug 15, 2024

View reviewed changes

Update permission from 0444 to 0600

5fe77f7

GalRogozinski reviewed Aug 15, 2024

View reviewed changes

GalRogozinski requested changes Aug 15, 2024

View reviewed changes

Update 0444 to 0400

07f8940

GalRogozinski approved these changes Aug 15, 2024

View reviewed changes

GalRogozinski added the meta label Aug 15, 2024

MatheusFranco99 requested a review from alan-ssvlabs August 16, 2024 08:21

alan-ssvlabs approved these changes Aug 19, 2024

View reviewed changes

MatheusFranco99 merged commit 9bf4379 into ssvlabs:dev Aug 20, 2024
2 checks passed

MatheusFranco99 deleted the static-analysis-issues branch August 20, 2024 10:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix static analysis issues #480

Fix static analysis issues #480

MatheusFranco99 commented Aug 14, 2024

GalRogozinski Aug 15, 2024

MatheusFranco99 Aug 15, 2024

MatheusFranco99 Aug 15, 2024

GalRogozinski Aug 15, 2024 •

edited

Loading

MatheusFranco99 Aug 15, 2024

GalRogozinski Aug 15, 2024

GalRogozinski Aug 15, 2024

GalRogozinski Aug 15, 2024

GalRogozinski Aug 15, 2024

GalRogozinski left a comment

Fix static analysis issues #480

Fix static analysis issues #480

Conversation

MatheusFranco99 commented Aug 14, 2024

Overview

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GalRogozinski Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GalRogozinski left a comment

Choose a reason for hiding this comment

GalRogozinski Aug 15, 2024 •

edited

Loading