From ad7099816fb08956c1d6bb611e75b9149c63f87a Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 14 May 2024 17:36:56 +0200 Subject: [PATCH] chore: finish addressing ~easy gosec warnings (#1603) With this change, we finish addressing all the `gosec` warnings that, on paper, looked relatively easy to fix. The remaining warnings need more thinking to be either dismissed or addressed properly. Part of https://github.com/ooni/probe/issues/2722 --- cmd/ooniprobe/internal/autorun/autorun_darwin.go | 4 ++-- cmd/ooniprobe/internal/cli/reset/reset.go | 4 +++- cmd/ooniprobe/internal/cli/rm/rm.go | 5 +++-- cmd/ooniprobe/internal/config/parser.go | 2 +- cmd/ooniprobe/internal/nettests/nettests.go | 4 ++-- cmd/ooniprobe/internal/ooni/ooni.go | 2 +- internal/cmd/oohelperd/main.go | 14 +++++++++++--- internal/database/actions.go | 10 ++++++---- internal/netemx/http.go | 2 +- internal/netemx/https.go | 2 +- internal/testingx/httptestx.go | 4 ++-- pkg/gobash/version.go | 2 +- 12 files changed, 34 insertions(+), 21 deletions(-) diff --git a/cmd/ooniprobe/internal/autorun/autorun_darwin.go b/cmd/ooniprobe/internal/autorun/autorun_darwin.go index fe35e8a895..01e812f20e 100644 --- a/cmd/ooniprobe/internal/autorun/autorun_darwin.go +++ b/cmd/ooniprobe/internal/autorun/autorun_darwin.go @@ -109,11 +109,11 @@ func (managerDarwin) writePlist() error { return err } log.Infof("exec: mkdir -p %s", plistDir) - if err := os.MkdirAll(plistDir, 0755); err != nil { + if err := os.MkdirAll(plistDir, 0700); err != nil { return err } log.Infof("exec: writePlist(%s)", plistPath) - return os.WriteFile(plistPath, out.Bytes(), 0644) + return os.WriteFile(plistPath, out.Bytes(), 0600) } func (managerDarwin) start() error { diff --git a/cmd/ooniprobe/internal/cli/reset/reset.go b/cmd/ooniprobe/internal/cli/reset/reset.go index 7517176247..6b736d54e7 100644 --- a/cmd/ooniprobe/internal/cli/reset/reset.go +++ b/cmd/ooniprobe/internal/cli/reset/reset.go @@ -26,7 +26,9 @@ func init() { return err } if *force { - os.RemoveAll(ctx.Home()) + // trade off: we're not checking for an error here to make the + // OONI directory deletion idempotent + _ = os.RemoveAll(ctx.Home()) log.Infof("Deleted %s", ctx.Home()) } else { log.Infof("Run with --force to delete %s", ctx.Home()) diff --git a/cmd/ooniprobe/internal/cli/rm/rm.go b/cmd/ooniprobe/internal/cli/rm/rm.go index 27f58a519f..ab3409a020 100644 --- a/cmd/ooniprobe/internal/cli/rm/rm.go +++ b/cmd/ooniprobe/internal/cli/rm/rm.go @@ -9,6 +9,7 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/cmd/ooniprobe/internal/cli/root" "github.com/ooni/probe-cli/v3/internal/database" + "github.com/ooni/probe-cli/v3/internal/runtimex" "github.com/upper/db/v4" ) @@ -20,7 +21,7 @@ func deleteAll(d *database.Database, skipInteractive bool) error { Options: []string{"true", "false"}, Default: "false", } - survey.AskOne(confirm, &answer, nil) + runtimex.Try0(survey.AskOne(confirm, &answer, nil)) if answer == "false" { return errors.New("canceled by user") } @@ -80,7 +81,7 @@ func init() { Options: []string{"true", "false"}, Default: "false", } - survey.AskOne(confirm, &answer, nil) + runtimex.Try0(survey.AskOne(confirm, &answer, nil)) if answer == "false" { return errors.New("canceled by user") } diff --git a/cmd/ooniprobe/internal/config/parser.go b/cmd/ooniprobe/internal/config/parser.go index ae762484bd..f44d64a42f 100644 --- a/cmd/ooniprobe/internal/config/parser.go +++ b/cmd/ooniprobe/internal/config/parser.go @@ -67,7 +67,7 @@ func (c *Config) Write() error { if c.path == "" { return errors.New("config file path is empty") } - if err := os.WriteFile(c.path, configJSON, 0644); err != nil { + if err := os.WriteFile(c.path, configJSON, 0600); err != nil { return errors.Wrap(err, "writing config JSON") } return nil diff --git a/cmd/ooniprobe/internal/nettests/nettests.go b/cmd/ooniprobe/internal/nettests/nettests.go index 01e82de2b0..15122ab416 100644 --- a/cmd/ooniprobe/internal/nettests/nettests.go +++ b/cmd/ooniprobe/internal/nettests/nettests.go @@ -246,9 +246,9 @@ func (c *Controller) Run(builder model.ExperimentBuilder, inputs []string) error return errors.Wrap(err, "failed to add test keys to summary") } } - db.UpdateUploadedStatus(c.res) + err := db.UpdateUploadedStatus(c.res) log.Debugf("status.end") - return nil + return err } // OnProgress should be called when a new progress event is available. diff --git a/cmd/ooniprobe/internal/ooni/ooni.go b/cmd/ooniprobe/internal/ooni/ooni.go index 69235caef4..ead4142ad5 100644 --- a/cmd/ooniprobe/internal/ooni/ooni.go +++ b/cmd/ooniprobe/internal/ooni/ooni.go @@ -290,7 +290,7 @@ func InitDefaultConfig(home string) (*config.Config, error) { if err != nil { if os.IsNotExist(err) { log.Debugf("writing default config to %s", configPath) - if err = os.WriteFile(configPath, defaultConfig, 0644); err != nil { + if err = os.WriteFile(configPath, defaultConfig, 0600); err != nil { return nil, err } // If the user did the informed consent procedure in diff --git a/internal/cmd/oohelperd/main.go b/internal/cmd/oohelperd/main.go index ae7f7e1c39..e5c619c47f 100644 --- a/internal/cmd/oohelperd/main.go +++ b/internal/cmd/oohelperd/main.go @@ -100,12 +100,16 @@ func main() { } else { w.Header().Set("WWW-Authenticate", "Basic realm=metrics") w.WriteHeader(401) - w.Write([]byte("401 Unauthorized\n")) + _, _ = w.Write([]byte("401 Unauthorized\n")) } }) // create a listening server for serving ooniprobe requests - srv := &http.Server{Addr: *apiEndpoint, Handler: mux} + srv := &http.Server{ + Addr: *apiEndpoint, + Handler: mux, + ReadHeaderTimeout: 8 * time.Second, + } listener, err := net.Listen("tcp", *apiEndpoint) runtimex.PanicOnError(err, "net.Listen failed") @@ -121,7 +125,11 @@ func main() { pprofMux := http.NewServeMux() pprofMux.Handle("/debug/pprof/profile", http.HandlerFunc(pprof.Profile)) pprofMux.Handle("/debug/pprof/trace", http.HandlerFunc(pprof.Trace)) - pprofSrv := &http.Server{Addr: *pprofEndpoint, Handler: pprofMux} + pprofSrv := &http.Server{ + Addr: *pprofEndpoint, + Handler: pprofMux, + ReadHeaderTimeout: 8 * time.Second, + } go pprofSrv.ListenAndServe() log.Infof("serving CPU profile at http://%s/debug/pprof/profile", *pprofEndpoint) log.Infof("serving execution traces at http://%s/debug/pprof/trace", *pprofEndpoint) diff --git a/internal/database/actions.go b/internal/database/actions.go index 2e754ca2bd..e3035e9eb1 100644 --- a/internal/database/actions.go +++ b/internal/database/actions.go @@ -198,11 +198,10 @@ func (d *Database) DeleteResult(resultID int64) error { return err } if err := res.Delete(); err != nil { - log.WithError(err).Error("failed to delete the result directory") + log.WithError(err).Error("failed to delete the result") return err } - os.RemoveAll(result.MeasurementDir) - return nil + return os.RemoveAll(result.MeasurementDir) } // UpdateUploadedStatus implements WritableDatabase.UpdateUploadedStatus @@ -337,7 +336,10 @@ func (d *Database) CreateOrUpdateURL(urlStr string, categoryCode string, country return err } else { url.CategoryCode = sql.NullString{String: categoryCode, Valid: true} - res.Update(url) + if err := res.Update(url); err != nil { + log.WithError(err).Error("Failed to update the database") + return err + } } return nil diff --git a/internal/netemx/http.go b/internal/netemx/http.go index bc752f5e2a..1f1aa874a0 100644 --- a/internal/netemx/http.go +++ b/internal/netemx/http.go @@ -99,7 +99,7 @@ func (srv *httpCleartextServer) mustListenPortLocked(handler http.Handler, ipAdd listener := runtimex.Try1(srv.unet.ListenTCP("tcp", addr)) // serve requests in a background goroutine - srvr := &http.Server{Handler: handler} + srvr := &http.Server{Handler: handler} // #nosec G112 - just a testing server go srvr.Serve(listener) // make sure we track the server (the .Serve method will close the diff --git a/internal/netemx/https.go b/internal/netemx/https.go index a708c7513b..1388d2cc18 100644 --- a/internal/netemx/https.go +++ b/internal/netemx/https.go @@ -97,7 +97,7 @@ func (srv *httpSecureServer) mustListenPortLocked(handler http.Handler, ipAddr n tlsConfig := srv.unet.MustNewServerTLSConfig(srv.serverNameMain, srv.serverNameExtras...) // serve requests in a background goroutine - srvr := &http.Server{ + srvr := &http.Server{ // #nosec G112 - just a testing server Handler: handler, TLSConfig: tlsConfig, } diff --git a/internal/testingx/httptestx.go b/internal/testingx/httptestx.go index cd19dc56c6..689071e818 100644 --- a/internal/testingx/httptestx.go +++ b/internal/testingx/httptestx.go @@ -68,7 +68,7 @@ func MustNewHTTPServerEx(addr *net.TCPAddr, httpListener TCPListener, handler ht Path: "/", } srv := &HTTPServer{ - Config: &http.Server{Handler: handler}, + Config: &http.Server{Handler: handler}, // #nosec G112 - just a testing server Listener: listener, TLS: nil, URL: baseURL.String(), @@ -113,7 +113,7 @@ func MustNewHTTPServerTLSEx( otherNames = append(otherNames, extraSNIs...) srv := &HTTPServer{ - Config: &http.Server{Handler: handler}, + Config: &http.Server{Handler: handler}, // #nosec G112 - just a testing server Listener: listener, TLS: ca.MustNewServerTLSConfig(commonName, otherNames...), URL: baseURL.String(), diff --git a/pkg/gobash/version.go b/pkg/gobash/version.go index 722c0644ff..f236e0f222 100644 --- a/pkg/gobash/version.go +++ b/pkg/gobash/version.go @@ -284,7 +284,7 @@ func unpackZip(targetDir, archiveFile string) error { } // File - if err := os.MkdirAll(filepath.Dir(outpath), 0755); err != nil { + if err := os.MkdirAll(filepath.Dir(outpath), 0700); err != nil { return err } out, err := os.OpenFile( // #nosec G304 - this is working as intended