From 2d8be4e85b604d2385540c005ae816a7ee860a9f Mon Sep 17 00:00:00 2001 From: Daniel Burckhardt Date: Thu, 25 Nov 2021 16:12:57 +0100 Subject: [PATCH] fix: gosec issues (#779) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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> --- app/export.go | 4 +++- client/testnet.go | 5 ++++- cmd/ethermintd/root.go | 6 +++++- rpc/ethereum/namespaces/debug/api.go | 12 ++++++++++-- rpc/ethereum/namespaces/debug/trace.go | 13 +++++++++++-- rpc/ethereum/namespaces/debug/utils.go | 4 +++- server/flags/flags.go | 18 ++++++++---------- server/start.go | 26 +++++++++++++++++--------- testutil/network/network.go | 4 ++-- 9 files changed, 63 insertions(+), 29 deletions(-) diff --git a/app/export.go b/app/export.go index e1fd1187..f2cb346a 100644 --- 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 index bc75c37c..fb4e5e10 100644 --- 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 index 3b0e73d5..6ac50016 100644 --- 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 index 35c4e598..ad516afe 100644 --- 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 index f550d3d2..6abbd830 100644 --- 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 index b8a2a40f..7f270ed7 100644 --- 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 index 56356f0c..f19e0d2c 100644 --- 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 index 10422116..3ae3c61e 100644 --- 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 index e76e98b1..62e205f7 100644 --- 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 }