fix: gosec issues (evmos#779)

* remove gosec warnigs with medium severity * Improvement(Ethermint): Fix gosec vulnerabilities * Improvement(Evmos): address pr comments * Improvement(Ethermint): Fix flags test by using PersistentFlags() instead of Flags() * Improvement(Ethermint): Fix return of defer function * Improvement(Ethermint): Replace PersistentFlags with Flags * Apply suggestions from code review * Improvement(Ethermint): Use persisentFlags again and remove required attribute for chain id Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
yihuang · Nov 25, 2021 · 2d8be4e · 2d8be4e
1 parent 32eaec8
commit 2d8be4e
Show file tree

Hide file tree

Showing 9 changed files with 63 additions and 29 deletions.
diff --git a/app/export.go b/app/export.go
@@ -186,7 +186,9 @@ func (app *EthermintApp) prepForZeroHeightGenesis(ctx sdk.Context, jailAllowedAd
 		app.StakingKeeper.SetValidator(ctx, validator)
 	}
 
-	iter.Close()
+	if err := iter.Close(); err != nil {
+		return err
+	}
 
 	if _, err := app.StakingKeeper.ApplyAndReturnValidatorSetUpdates(ctx); err != nil {
 		return err

diff --git a/client/testnet.go b/client/testnet.go
@@ -556,7 +556,10 @@ func startTestnet(cmd *cobra.Command, args startArgs) error {
 	}
 
 	cmd.Println("press the Enter Key to terminate")
-	fmt.Scanln() // wait for Enter Key
+	_, err = fmt.Scanln() // wait for Enter Key
+	if err != nil {
+		return err
+	}
 	testnet.Cleanup()
 
 	return nil

diff --git a/cmd/ethermintd/root.go b/cmd/ethermintd/root.go
@@ -119,7 +119,11 @@ func NewRootCmd() (*cobra.Command, params.EncodingConfig) {
 		txCommand(),
 		ethermintclient.KeyCommands(app.DefaultNodeHome),
 	)
-	rootCmd = srvflags.AddTxFlags(rootCmd)
+
+	rootCmd, err := srvflags.AddTxFlags(rootCmd)
+	if err != nil {
+		panic(err)
+	}
 
 	// add rosetta
 	rootCmd.AddCommand(sdkserver.RosettaCommand(encodingConfig.InterfaceRegistry, encodingConfig.Marshaler))

diff --git a/rpc/ethereum/namespaces/debug/api.go b/rpc/ethereum/namespaces/debug/api.go
@@ -19,8 +19,10 @@ import (
 	evmtypes "github.com/tharsis/ethermint/x/evm/types"
 
 	"github.com/cosmos/cosmos-sdk/client"
+	stderrors "github.com/pkg/errors"
 
 	"github.com/cosmos/cosmos-sdk/server"
+
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/common/hexutil"
 	"github.com/ethereum/go-ethereum/consensus/ethash"
@@ -351,7 +353,10 @@ func (a *API) StartCPUProfile(file string) error {
 		}
 		if err := pprof.StartCPUProfile(f); err != nil {
 			a.logger.Debug("cpu profiling already in use", "error", err.Error())
-			f.Close()
+			if err := f.Close(); err != nil {
+				a.logger.Debug("failed to close cpu profile file")
+				return stderrors.Wrap(err, "failed to close cpu profile file")
+			}
 			return err
 		}
 
@@ -375,7 +380,10 @@ func (a *API) StopCPUProfile() error {
 	case a.handler.cpuFile != nil:
 		a.logger.Info("Done writing CPU profile", "profile", a.handler.cpuFilename)
 		pprof.StopCPUProfile()
-		a.handler.cpuFile.Close()
+		if err := a.handler.cpuFile.Close(); err != nil {
+			a.logger.Debug("failed to close cpu file")
+			return stderrors.Wrap(err, "failed to close cpu file")
+		}
 		a.handler.cpuFile = nil
 		a.handler.cpuFilename = ""
 		return nil

diff --git a/rpc/ethereum/namespaces/debug/trace.go b/rpc/ethereum/namespaces/debug/trace.go
@@ -23,6 +23,8 @@ import (
 	"errors"
 	"os"
 	"runtime/trace"
+
+	stderrors "github.com/pkg/errors"
 )
 
 // StartGoTrace turns on tracing, writing to the given file.
@@ -47,7 +49,11 @@ func (a *API) StartGoTrace(file string) error {
 	}
 	if err := trace.Start(f); err != nil {
 		a.logger.Debug("Go tracing already started", "error", err.Error())
-		f.Close()
+		if err := f.Close(); err != nil {
+			a.logger.Debug("failed to close trace file")
+			return stderrors.Wrap(err, "failed to close trace file")
+		}
+
 		return err
 	}
 	a.handler.traceFile = f
@@ -68,7 +74,10 @@ func (a *API) StopGoTrace() error {
 		return errors.New("trace not in progress")
 	}
 	a.logger.Info("Done writing Go trace", "dump", a.handler.traceFilename)
-	a.handler.traceFile.Close()
+	if err := a.handler.traceFile.Close(); err != nil {
+		a.logger.Debug("failed to close trace file")
+		return stderrors.Wrap(err, "failed to close trace file")
+	}
 	a.handler.traceFile = nil
 	a.handler.traceFilename = ""
 	return nil

diff --git a/rpc/ethereum/namespaces/debug/utils.go b/rpc/ethereum/namespaces/debug/utils.go
@@ -50,7 +50,9 @@ func writeProfile(name, file string, log log.Logger) error {
 	}
 
 	if err := p.WriteTo(f, 0); err != nil {
-		f.Close()
+		if err := f.Close(); err != nil {
+			return err
+		}
 		return err
 	}
 

diff --git a/server/flags/flags.go b/server/flags/flags.go
@@ -49,7 +49,7 @@ const (
 )
 
 // AddTxFlags adds common flags for commands to post tx
-func AddTxFlags(cmd *cobra.Command) *cobra.Command {
+func AddTxFlags(cmd *cobra.Command) (*cobra.Command, error) {
 	cmd.PersistentFlags().String(flags.FlagChainID, "testnet", "Specify Chain ID for sending Tx")
 	cmd.PersistentFlags().String(flags.FlagFrom, "", "Name or address of private key with which to sign")
 	cmd.PersistentFlags().String(flags.FlagFees, "", "Fees to pay along with transaction; eg: 10aphoton")
@@ -66,13 +66,11 @@ func AddTxFlags(cmd *cobra.Command) *cobra.Command {
 	// ))
 
 	// viper.BindPFlag(flags.FlagTrustNode, cmd.Flags().Lookup(flags.FlagTrustNode))
-
-	// TODO: we need to handle the errors for these, decide if we should return error upward and handle
-	// nolint: errcheck
-	viper.BindPFlag(flags.FlagNode, cmd.Flags().Lookup(flags.FlagNode))
-	// nolint: errcheck
-	viper.BindPFlag(flags.FlagKeyringBackend, cmd.Flags().Lookup(flags.FlagKeyringBackend))
-	// nolint: errcheck
-	cmd.MarkFlagRequired(flags.FlagChainID)
-	return cmd
+	if err := viper.BindPFlag(flags.FlagNode, cmd.PersistentFlags().Lookup(flags.FlagNode)); err != nil {
+		return nil, err
+	}
+	if err := viper.BindPFlag(flags.FlagKeyringBackend, cmd.PersistentFlags().Lookup(flags.FlagKeyringBackend)); err != nil {
+		return nil, err
+	}
+	return cmd, nil
 }
diff --git a/server/start.go b/server/start.go
@@ -219,11 +219,11 @@ func startStandAlone(ctx *server.Context, appCreator types.AppCreator) error {
 }
 
 // legacyAminoCdc is used for the legacy REST API
-func startInProcess(ctx *server.Context, clientCtx client.Context, appCreator types.AppCreator) error {
+func startInProcess(ctx *server.Context, clientCtx client.Context, appCreator types.AppCreator) (err error) {
 	cfg := ctx.Config
 	home := cfg.RootDir
 	logger := ctx.Logger
-	var cpuProfileCleanup func()
+	var cpuProfileCleanup func() error
 
 	if cpuProfile := ctx.Viper.GetString(srvflags.CPUProfile); cpuProfile != "" {
 		fp, err := ethdebug.ExpandHome(cpuProfile)
@@ -241,10 +241,14 @@ func startInProcess(ctx *server.Context, clientCtx client.Context, appCreator ty
 			return err
 		}
 
-		cpuProfileCleanup = func() {
+		cpuProfileCleanup = func() error {
 			ctx.Logger.Info("stopping CPU profiler", "profile", cpuProfile)
 			pprof.StopCPUProfile()
-			f.Close()
+			if err := f.Close(); err != nil {
+				logger.Error("failed to close CPU profiler file", "error", err.Error())
+				return err
+			}
+			return nil
 		}
 	}
 
@@ -359,7 +363,7 @@ func startInProcess(ctx *server.Context, clientCtx client.Context, appCreator ty
 		if config.GRPCWeb.Enable {
 			grpcWebSrv, err = servergrpc.StartGRPCWeb(grpcSrv, config.Config)
 			if err != nil {
-				ctx.Logger.Error("failed to start grpc-web http server: ", err)
+				ctx.Logger.Error("failed to start grpc-web http server", "error", err)
 				return err
 			}
 		}
@@ -429,7 +433,7 @@ func startInProcess(ctx *server.Context, clientCtx client.Context, appCreator ty
 		}
 
 		if cpuProfileCleanup != nil {
-			cpuProfileCleanup()
+			_ = cpuProfileCleanup()
 		}
 
 		if apiSrv != nil {
@@ -439,7 +443,9 @@ func startInProcess(ctx *server.Context, clientCtx client.Context, appCreator ty
 		if grpcSrv != nil {
 			grpcSrv.Stop()
 			if grpcWebSrv != nil {
-				grpcWebSrv.Close()
+				if err := grpcWebSrv.Close(); err != nil {
+					logger.Error("failed to close the grpcWebSrc", "error", err.Error())
+				}
 			}
 		}
 
@@ -474,9 +480,11 @@ func openTraceWriter(traceWriterFile string) (w io.Writer, err error) {
 	if traceWriterFile == "" {
 		return
 	}
+
+	filePath := filepath.Clean(traceWriterFile)
 	return os.OpenFile(
-		traceWriterFile,
+		filePath,
 		os.O_WRONLY|os.O_APPEND|os.O_CREATE,
-		0o666,
+		0o600,
 	)
 }
diff --git a/testutil/network/network.go b/testutil/network/network.go
@@ -334,12 +334,12 @@ func New(l Logger, baseDir string, cfg Config) (*Network, error) {
 		clientDir := filepath.Join(network.BaseDir, nodeDirName, "evmoscli")
 		gentxsDir := filepath.Join(network.BaseDir, "gentxs")
 
-		err := os.MkdirAll(filepath.Join(nodeDir, "config"), 0o755)
+		err := os.MkdirAll(filepath.Join(nodeDir, "config"), 0o750)
 		if err != nil {
 			return nil, err
 		}
 
-		err = os.MkdirAll(clientDir, 0o755)
+		err = os.MkdirAll(clientDir, 0o750)
 		if err != nil {
 			return nil, err
 		}