Skip to content

Commit ad70998

Browse files
authored
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 ooni/probe#2722
1 parent 8a8e1c4 commit ad70998

File tree

12 files changed

+34
-21
lines changed

12 files changed

+34
-21
lines changed

Diff for: cmd/ooniprobe/internal/autorun/autorun_darwin.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -109,11 +109,11 @@ func (managerDarwin) writePlist() error {
109109
return err
110110
}
111111
log.Infof("exec: mkdir -p %s", plistDir)
112-
if err := os.MkdirAll(plistDir, 0755); err != nil {
112+
if err := os.MkdirAll(plistDir, 0700); err != nil {
113113
return err
114114
}
115115
log.Infof("exec: writePlist(%s)", plistPath)
116-
return os.WriteFile(plistPath, out.Bytes(), 0644)
116+
return os.WriteFile(plistPath, out.Bytes(), 0600)
117117
}
118118

119119
func (managerDarwin) start() error {

Diff for: cmd/ooniprobe/internal/cli/reset/reset.go

+3-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@ func init() {
2626
return err
2727
}
2828
if *force {
29-
os.RemoveAll(ctx.Home())
29+
// trade off: we're not checking for an error here to make the
30+
// OONI directory deletion idempotent
31+
_ = os.RemoveAll(ctx.Home())
3032
log.Infof("Deleted %s", ctx.Home())
3133
} else {
3234
log.Infof("Run with --force to delete %s", ctx.Home())

Diff for: cmd/ooniprobe/internal/cli/rm/rm.go

+3-2
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"github.com/apex/log"
1010
"github.com/ooni/probe-cli/v3/cmd/ooniprobe/internal/cli/root"
1111
"github.com/ooni/probe-cli/v3/internal/database"
12+
"github.com/ooni/probe-cli/v3/internal/runtimex"
1213
"github.com/upper/db/v4"
1314
)
1415

@@ -20,7 +21,7 @@ func deleteAll(d *database.Database, skipInteractive bool) error {
2021
Options: []string{"true", "false"},
2122
Default: "false",
2223
}
23-
survey.AskOne(confirm, &answer, nil)
24+
runtimex.Try0(survey.AskOne(confirm, &answer, nil))
2425
if answer == "false" {
2526
return errors.New("canceled by user")
2627
}
@@ -80,7 +81,7 @@ func init() {
8081
Options: []string{"true", "false"},
8182
Default: "false",
8283
}
83-
survey.AskOne(confirm, &answer, nil)
84+
runtimex.Try0(survey.AskOne(confirm, &answer, nil))
8485
if answer == "false" {
8586
return errors.New("canceled by user")
8687
}

Diff for: cmd/ooniprobe/internal/config/parser.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ func (c *Config) Write() error {
6767
if c.path == "" {
6868
return errors.New("config file path is empty")
6969
}
70-
if err := os.WriteFile(c.path, configJSON, 0644); err != nil {
70+
if err := os.WriteFile(c.path, configJSON, 0600); err != nil {
7171
return errors.Wrap(err, "writing config JSON")
7272
}
7373
return nil

Diff for: cmd/ooniprobe/internal/nettests/nettests.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -246,9 +246,9 @@ func (c *Controller) Run(builder model.ExperimentBuilder, inputs []string) error
246246
return errors.Wrap(err, "failed to add test keys to summary")
247247
}
248248
}
249-
db.UpdateUploadedStatus(c.res)
249+
err := db.UpdateUploadedStatus(c.res)
250250
log.Debugf("status.end")
251-
return nil
251+
return err
252252
}
253253

254254
// OnProgress should be called when a new progress event is available.

Diff for: cmd/ooniprobe/internal/ooni/ooni.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,7 @@ func InitDefaultConfig(home string) (*config.Config, error) {
290290
if err != nil {
291291
if os.IsNotExist(err) {
292292
log.Debugf("writing default config to %s", configPath)
293-
if err = os.WriteFile(configPath, defaultConfig, 0644); err != nil {
293+
if err = os.WriteFile(configPath, defaultConfig, 0600); err != nil {
294294
return nil, err
295295
}
296296
// If the user did the informed consent procedure in

Diff for: internal/cmd/oohelperd/main.go

+11-3
Original file line numberDiff line numberDiff line change
@@ -100,12 +100,16 @@ func main() {
100100
} else {
101101
w.Header().Set("WWW-Authenticate", "Basic realm=metrics")
102102
w.WriteHeader(401)
103-
w.Write([]byte("401 Unauthorized\n"))
103+
_, _ = w.Write([]byte("401 Unauthorized\n"))
104104
}
105105
})
106106

107107
// create a listening server for serving ooniprobe requests
108-
srv := &http.Server{Addr: *apiEndpoint, Handler: mux}
108+
srv := &http.Server{
109+
Addr: *apiEndpoint,
110+
Handler: mux,
111+
ReadHeaderTimeout: 8 * time.Second,
112+
}
109113
listener, err := net.Listen("tcp", *apiEndpoint)
110114
runtimex.PanicOnError(err, "net.Listen failed")
111115

@@ -121,7 +125,11 @@ func main() {
121125
pprofMux := http.NewServeMux()
122126
pprofMux.Handle("/debug/pprof/profile", http.HandlerFunc(pprof.Profile))
123127
pprofMux.Handle("/debug/pprof/trace", http.HandlerFunc(pprof.Trace))
124-
pprofSrv := &http.Server{Addr: *pprofEndpoint, Handler: pprofMux}
128+
pprofSrv := &http.Server{
129+
Addr: *pprofEndpoint,
130+
Handler: pprofMux,
131+
ReadHeaderTimeout: 8 * time.Second,
132+
}
125133
go pprofSrv.ListenAndServe()
126134
log.Infof("serving CPU profile at http://%s/debug/pprof/profile", *pprofEndpoint)
127135
log.Infof("serving execution traces at http://%s/debug/pprof/trace", *pprofEndpoint)

Diff for: internal/database/actions.go

+6-4
Original file line numberDiff line numberDiff line change
@@ -198,11 +198,10 @@ func (d *Database) DeleteResult(resultID int64) error {
198198
return err
199199
}
200200
if err := res.Delete(); err != nil {
201-
log.WithError(err).Error("failed to delete the result directory")
201+
log.WithError(err).Error("failed to delete the result")
202202
return err
203203
}
204-
os.RemoveAll(result.MeasurementDir)
205-
return nil
204+
return os.RemoveAll(result.MeasurementDir)
206205
}
207206

208207
// UpdateUploadedStatus implements WritableDatabase.UpdateUploadedStatus
@@ -337,7 +336,10 @@ func (d *Database) CreateOrUpdateURL(urlStr string, categoryCode string, country
337336
return err
338337
} else {
339338
url.CategoryCode = sql.NullString{String: categoryCode, Valid: true}
340-
res.Update(url)
339+
if err := res.Update(url); err != nil {
340+
log.WithError(err).Error("Failed to update the database")
341+
return err
342+
}
341343
}
342344

343345
return nil

Diff for: internal/netemx/http.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ func (srv *httpCleartextServer) mustListenPortLocked(handler http.Handler, ipAdd
9999
listener := runtimex.Try1(srv.unet.ListenTCP("tcp", addr))
100100

101101
// serve requests in a background goroutine
102-
srvr := &http.Server{Handler: handler}
102+
srvr := &http.Server{Handler: handler} // #nosec G112 - just a testing server
103103
go srvr.Serve(listener)
104104

105105
// make sure we track the server (the .Serve method will close the

Diff for: internal/netemx/https.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ func (srv *httpSecureServer) mustListenPortLocked(handler http.Handler, ipAddr n
9797
tlsConfig := srv.unet.MustNewServerTLSConfig(srv.serverNameMain, srv.serverNameExtras...)
9898

9999
// serve requests in a background goroutine
100-
srvr := &http.Server{
100+
srvr := &http.Server{ // #nosec G112 - just a testing server
101101
Handler: handler,
102102
TLSConfig: tlsConfig,
103103
}

Diff for: internal/testingx/httptestx.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ func MustNewHTTPServerEx(addr *net.TCPAddr, httpListener TCPListener, handler ht
6868
Path: "/",
6969
}
7070
srv := &HTTPServer{
71-
Config: &http.Server{Handler: handler},
71+
Config: &http.Server{Handler: handler}, // #nosec G112 - just a testing server
7272
Listener: listener,
7373
TLS: nil,
7474
URL: baseURL.String(),
@@ -113,7 +113,7 @@ func MustNewHTTPServerTLSEx(
113113
otherNames = append(otherNames, extraSNIs...)
114114

115115
srv := &HTTPServer{
116-
Config: &http.Server{Handler: handler},
116+
Config: &http.Server{Handler: handler}, // #nosec G112 - just a testing server
117117
Listener: listener,
118118
TLS: ca.MustNewServerTLSConfig(commonName, otherNames...),
119119
URL: baseURL.String(),

Diff for: pkg/gobash/version.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,7 @@ func unpackZip(targetDir, archiveFile string) error {
284284
}
285285

286286
// File
287-
if err := os.MkdirAll(filepath.Dir(outpath), 0755); err != nil {
287+
if err := os.MkdirAll(filepath.Dir(outpath), 0700); err != nil {
288288
return err
289289
}
290290
out, err := os.OpenFile( // #nosec G304 - this is working as intended

0 commit comments

Comments
 (0)