From aca1babb8a71a1413b7f575068ff4d3d3f20f065 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 5 Jun 2024 14:51:56 +0200 Subject: [PATCH] osbuild-worker-executor: appease gosec Note that gosec IMHO is a bit silly here, the heuristics used are note very good, i.e. the code is already validating the external inputs and it's not clear to me that "filepath.Clean()" will help but it seems to supress the error. I hope gosec provides value in other places, here it seems to be adding work :/ I also excluded "gosec" from any _test.go files, I do not see why we should gosec tests? --- .golangci.yml | 3 +++ cmd/osbuild-worker-executor/handler_build.go | 25 +++++++++++++++----- cmd/osbuild-worker-executor/main.go | 5 ++-- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 86dce5fb34..0ee29297c4 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -23,3 +23,6 @@ issues: # Maximum count of issues with the same text. Set to 0 to disable. Default is 3. max-same-issues: 0 + exclude-rules: + - path: ^cmd/osbuild-worker-executor/.*_test\.go$ + linters: ["gosec"] diff --git a/cmd/osbuild-worker-executor/handler_build.go b/cmd/osbuild-worker-executor/handler_build.go index 62057e2154..a790ae523b 100644 --- a/cmd/osbuild-worker-executor/handler_build.go +++ b/cmd/osbuild-worker-executor/handler_build.go @@ -46,8 +46,10 @@ func runOsbuild(buildDir string, control *controlJSON, output io.Writer) (string } // stream output over http wf := writeFlusher{w: output, flusher: flusher} + // note that the "filepath.Clean()" is here for gosec:G304 + logfPath := filepath.Clean(filepath.Join(buildDir, "build.log")) // and also write to our internal log - logf, err := os.Create(filepath.Join(buildDir, "build.log")) + logf, err := os.Create(logfPath) if err != nil { return "", fmt.Errorf("cannot create log file: %v", err) } @@ -79,6 +81,7 @@ func runOsbuild(buildDir string, control *controlJSON, output io.Writer) (string return "", err } + // #nosec G204 cmd = exec.Command( "tar", "-Scf", @@ -152,7 +155,8 @@ func handleManifestJSON(atar *tar.Reader, buildDir string) error { } manifestJSONPath := filepath.Join(buildDir, "manifest.json") - f, err := os.Create(manifestJSONPath) + // note that the filepath.Clean() is here just to appease gosec:G304 + f, err := os.Create(filepath.Clean(manifestJSONPath)) if err != nil { return fmt.Errorf("cannot create manifest.json: %v", err) } @@ -180,16 +184,19 @@ func handleIncludedSources(atar *tar.Reader, buildDir string) error { } // ensure we only allow "store/" things - if filepath.Clean(hdr.Name) != strings.TrimSuffix(hdr.Name, "/") { - return fmt.Errorf("name not clean: %v != %v", filepath.Clean(hdr.Name), hdr.Name) + name := hdr.Name + if filepath.Clean(name) != strings.TrimSuffix(name, "/") { + return fmt.Errorf("name not clean: %v != %v", filepath.Clean(name), name) } - if !strings.HasPrefix(hdr.Name, "store/") { + if !strings.HasPrefix(name, "store/") { return fmt.Errorf("expected store/ prefix, got %v", hdr.Name) } + // note that the extra filepath.Clean() is just there to + // appease gosec G305 + target := filepath.Join(buildDir, filepath.Clean(name)) // this assume "well" behaving tars, i.e. all dirs that lead // up to the tar are included etc - target := filepath.Join(buildDir, hdr.Name) mode := os.FileMode(hdr.Mode) switch hdr.Typeflag { case tar.TypeDir: @@ -197,6 +204,12 @@ func handleIncludedSources(atar *tar.Reader, buildDir string) error { return fmt.Errorf("unpack: %w", err) } case tar.TypeReg: + // Note that the G304 here is silly, target is carefully + // checked (and is no error in the flow above). Mode is + // not relevant for an information leak. But it seems + // there is no way to get a debug trace from gosec what + // exactly is wrong. + // #nosec G304 f, err := os.OpenFile(target, os.O_RDWR|os.O_CREATE, mode) if err != nil { return fmt.Errorf("unpack: %w", err) diff --git a/cmd/osbuild-worker-executor/main.go b/cmd/osbuild-worker-executor/main.go index 1407202633..59edfcbc90 100644 --- a/cmd/osbuild-worker-executor/main.go +++ b/cmd/osbuild-worker-executor/main.go @@ -39,8 +39,9 @@ func run(ctx context.Context, args []string, getenv func(string) string) error { srv := newServer(logger, config) httpServer := &http.Server{ - Addr: net.JoinHostPort(config.Host, config.Port), - Handler: srv, + Addr: net.JoinHostPort(config.Host, config.Port), + Handler: srv, + ReadHeaderTimeout: 10 * time.Second, } go func() { logger.Printf("listening on %s\n", httpServer.Addr)