From 78ce84b3a1c69f5e4f460571a333ae2e7592fba1 Mon Sep 17 00:00:00 2001 From: Rob Parker Date: Thu, 11 Oct 2018 15:39:22 +0100 Subject: [PATCH] Implement GOSec for security scanning Fix vulnerabilities (#227) * Implement GOSec for security scanning Fix vulnerabilities * Fix lint failure * address PR comments and fix build break * Fix test break in mqsc --- .gitignore | 1 + Makefile | 26 ++++++++++++++++++++++++++ cmd/chkmqhealthy/main.go | 1 + cmd/chkmqready/main.go | 5 ++++- cmd/runmqdevserver/keystore.go | 24 ++++++++++++++++++++---- cmd/runmqdevserver/main.go | 12 ++++++++++-- cmd/runmqdevserver/mqsc.go | 9 ++++++++- cmd/runmqdevserver/template.go | 7 ++++++- cmd/runmqdevserver/tls.go | 1 + cmd/runmqserver/crtmqvol.go | 1 + cmd/runmqserver/license.go | 1 + cmd/runmqserver/logging.go | 17 ++++++++++++++--- cmd/runmqserver/main.go | 12 ++++++++++-- cmd/runmqserver/mirror.go | 10 ++++++++-- cmd/runmqserver/mqconfig.go | 30 +++++++++++++++--------------- cmd/runmqserver/qmgr.go | 14 +++++++++++--- cmd/runmqserver/signals.go | 3 ++- internal/command/command.go | 2 ++ internal/metrics/metrics.go | 12 ++++++++---- internal/metrics/update.go | 1 + 20 files changed, 150 insertions(+), 39 deletions(-) diff --git a/.gitignore b/.gitignore index 8ca3f5a..da814fc 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,4 @@ vendor/github.com/prometheus/client_model/bin/ vendor/github.com/prometheus/client_model/.classpath vendor/github.com/prometheus/client_model/.project vendor/github.com/prometheus/client_model/.settings* +gosec_results.json diff --git a/Makefile b/Makefile index 10302df..5afa075 100644 --- a/Makefile +++ b/Makefile @@ -141,6 +141,32 @@ lint: $(addsuffix /$(wildcard *.go), $(GO_PKG_DIRS)) @# As of 11/04/2018 there is an open issue to fix it: https://github.com/golang/lint/issues/320 golint -set_exit_status $(sort $(dir $(wildcard $(addsuffix /*/*.go, $(GO_PKG_DIRS))))) +.PHONY: gosec +gosec: $(info $(SPACER)$(shell printf "Running gosec test"$(END))) + @gosec -fmt=json -out=gosec_results.json cmd/... internal/... 2> /dev/null ;\ + cat "gosec_results.json" ;\ + cat gosec_results.json | grep HIGH | grep severity > /dev/null ;\ + if [ $$? -eq 0 ]; then \ + printf "\nFAILURE: gosec found files containing HIGH severity issues - see results.json\n" ;\ + exit 1 ;\ + else \ + printf "\ngosec found no HIGH severity issues\n" ;\ + fi ;\ + cat gosec_results.json | grep MEDIUM | grep severity > /dev/null ;\ + if [ $$? -eq 0 ]; then \ + printf "\nFAILURE: gosec found files containing MEDIUM severity issues - see results.json\n" ;\ + exit 1 ;\ + else \ + printf "\ngosec found no MEDIUM severity issues\n" ;\ + fi ;\ + cat gosec_results.json | grep LOW | grep severity > /dev/null;\ + if [ $$? -eq 0 ]; then \ + printf "\nFAILURE: gosec found files containing LOW severity issues - see results.json\n" ;\ + exit 1;\ + else \ + printf "\ngosec found no LOW severity issues\n" ;\ +fi ;\ + .PHONY: unknownos unknownos: $(info $(SPACER)$(shell printf "ERROR: Unknown OS ("$(BASE_OS)") please run specific make targets"$(END))) diff --git a/cmd/chkmqhealthy/main.go b/cmd/chkmqhealthy/main.go index b26bea8..1420805 100644 --- a/cmd/chkmqhealthy/main.go +++ b/cmd/chkmqhealthy/main.go @@ -32,6 +32,7 @@ func queueManagerHealthy() (bool, error) { return false, err } // Specify the queue manager name, just in case someone's created a second queue manager + // #nosec G204 cmd := exec.Command("dspmq", "-n", "-m", name) // Run the command and wait for completion out, err := cmd.CombinedOutput() diff --git a/cmd/chkmqready/main.go b/cmd/chkmqready/main.go index 4bb0c66..3f978d1 100644 --- a/cmd/chkmqready/main.go +++ b/cmd/chkmqready/main.go @@ -37,5 +37,8 @@ func main() { fmt.Println(err) os.Exit(1) } - conn.Close() + err = conn.Close() + if err != nil { + fmt.Println(err) + } } diff --git a/cmd/runmqdevserver/keystore.go b/cmd/runmqdevserver/keystore.go index 87eac87..5027ead 100644 --- a/cmd/runmqdevserver/keystore.go +++ b/cmd/runmqdevserver/keystore.go @@ -65,11 +65,27 @@ func (ks *KeyStore) Create() error { stashFile := ks.Filename[0:len(ks.Filename)-len(extension)] + ".sth" rdbFile := ks.Filename[0:len(ks.Filename)-len(extension)] + ".rdb" crlFile := ks.Filename[0:len(ks.Filename)-len(extension)] + ".crl" - os.Remove(stashFile) - os.Remove(rdbFile) - os.Remove(crlFile) + err = os.Remove(stashFile) + if err != nil { + log.Errorf("Error removing %s: %v", stashFile, err) + return err + } + err = os.Remove(rdbFile) + if err != nil { + log.Errorf("Error removing %s: %v", rdbFile, err) + return err + } + err = os.Remove(crlFile) + if err != nil { + log.Errorf("Error removing %s: %v", crlFile, err) + return err + } + } + err = os.Remove(ks.Filename) + if err != nil { + log.Errorf("Error removing %s: %v", ks.Filename, err) + return err } - os.Remove(ks.Filename) } else if !os.IsNotExist(err) { // If the keystore exists but cannot be accessed then return the error return err diff --git a/cmd/runmqdevserver/main.go b/cmd/runmqdevserver/main.go index 8546bc8..c6e9112 100644 --- a/cmd/runmqdevserver/main.go +++ b/cmd/runmqdevserver/main.go @@ -30,13 +30,17 @@ import ( var log *logger.Logger func setPassword(user string, password string) error { + // #nosec G204 cmd := exec.Command("chpasswd") stdin, err := cmd.StdinPipe() if err != nil { return err } fmt.Fprintf(stdin, "%s:%s", user, password) - stdin.Close() + err = stdin.Close() + if err != nil { + log.Errorf("Error closing password stdin: %v", err) + } _, _, err = command.RunCmd(cmd) if err != nil { return err @@ -165,6 +169,10 @@ func main() { osExit(1) } else { // Replace this process with runmqserver - syscall.Exec("/usr/local/bin/runmqserver", []string{"runmqserver"}, os.Environ()) + // #nosec G204 + err = syscall.Exec("/usr/local/bin/runmqserver", []string{"runmqserver"}, os.Environ()) + if err != nil { + log.Errorf("Error replacing this process with runmqserver: %v", err) + } } } diff --git a/cmd/runmqdevserver/mqsc.go b/cmd/runmqdevserver/mqsc.go index 77e3dfa..93d58ea 100644 --- a/cmd/runmqdevserver/mqsc.go +++ b/cmd/runmqdevserver/mqsc.go @@ -35,7 +35,14 @@ func updateMQSC(appPasswordRequired bool) error { return err } } else { - os.Remove(mqsc) + _, err := os.Stat(mqsc) + if !os.IsNotExist(err) { + err = os.Remove(mqsc) + if err != nil { + log.Errorf("Error removing file %s: %v", mqsc, err) + return err + } + } } return nil } diff --git a/cmd/runmqdevserver/template.go b/cmd/runmqdevserver/template.go index 7941633..28f8aa8 100644 --- a/cmd/runmqdevserver/template.go +++ b/cmd/runmqdevserver/template.go @@ -36,7 +36,11 @@ func processTemplateFile(templateFile, destFile string, data interface{}) error _, err = os.Stat(dir) if err != nil { if os.IsNotExist(err) { - os.MkdirAll(dir, 0660) + err = os.MkdirAll(dir, 0660) + if err != nil { + log.Error(err) + return err + } mqmUID, mqmGID, err := command.LookupMQM() if err != nil { log.Error(err) @@ -51,6 +55,7 @@ func processTemplateFile(templateFile, destFile string, data interface{}) error return err } } + // #nosec G302 f, err := os.OpenFile(destFile, os.O_CREATE|os.O_WRONLY, 0660) defer f.Close() err = t.Execute(f, data) diff --git a/cmd/runmqdevserver/tls.go b/cmd/runmqdevserver/tls.go index 7e6ed2c..8e21ed6 100644 --- a/cmd/runmqdevserver/tls.go +++ b/cmd/runmqdevserver/tls.go @@ -85,6 +85,7 @@ func configureTLS(qmName string, inputFile string, passPhrase string) error { _, err = os.Stat(dir) if err != nil { if os.IsNotExist(err) { + // #nosec G301 err = os.MkdirAll(dir, 0770) if err != nil { return err diff --git a/cmd/runmqserver/crtmqvol.go b/cmd/runmqserver/crtmqvol.go index 3ff017f..cadd833 100644 --- a/cmd/runmqserver/crtmqvol.go +++ b/cmd/runmqserver/crtmqvol.go @@ -29,6 +29,7 @@ func createVolume(path string) error { fi, err := os.Stat(dataPath) if err != nil { if os.IsNotExist(err) { + // #nosec G301 err = os.MkdirAll(dataPath, 0755) if err != nil { return err diff --git a/cmd/runmqserver/license.go b/cmd/runmqserver/license.go index 309a74b..19f199e 100644 --- a/cmd/runmqserver/license.go +++ b/cmd/runmqserver/license.go @@ -78,6 +78,7 @@ func checkLicense() (bool, error) { return true, nil case ok && lic == "view": file := filepath.Join("/opt/mqm/licenses", resolveLicenseFile()) + // #nosec G304 buf, err := ioutil.ReadFile(file) if err != nil { log.Println(err) diff --git a/cmd/runmqserver/logging.go b/cmd/runmqserver/logging.go index d46ac97..70bee1d 100644 --- a/cmd/runmqserver/logging.go +++ b/cmd/runmqserver/logging.go @@ -33,7 +33,7 @@ import ( // var debug = false var log *logger.Logger -var collectDiagOnFail bool = false +var collectDiagOnFail = false func logTerminationf(format string, args ...interface{}) { logTermination(fmt.Sprintf(format, args)) @@ -108,8 +108,12 @@ func configureLogger(name string) (mirrorFunc, error) { return func(msg string) { // Parse the JSON message, and print a simplified version var obj map[string]interface{} - json.Unmarshal([]byte(msg), &obj) - fmt.Printf(formatSimple(obj["ibm_datetime"].(string), obj["message"].(string))) + err := json.Unmarshal([]byte(msg), &obj) + if err != nil { + fmt.Printf("Failed to Unmarshall JSON - %v", err) + } else { + fmt.Printf(formatSimple(obj["ibm_datetime"].(string), obj["message"].(string))) + } }, nil default: log, err = logger.NewLogger(os.Stdout, d, false, name) @@ -124,20 +128,27 @@ func logDiagnostics() { log.Debug("--- Start Diagnostics ---") // show the directory ownership/permissions + // #nosec G104 out, _, _ := command.Run("ls", "-l", "/mnt/") log.Debugf("/mnt/:\n%s", out) + // #nosec G104 out, _, _ = command.Run("ls", "-l", "/mnt/mqm") log.Debugf("/mnt/mqm:\n%s", out) + // #nosec G104 out, _, _ = command.Run("ls", "-l", "/mnt/mqm/data") log.Debugf("/mnt/mqm/data:\n%s", out) + // #nosec G104 out, _, _ = command.Run("ls", "-l", "/var/mqm") log.Debugf("/var/mqm:\n%s", out) + // #nosec G104 out, _, _ = command.Run("ls", "-l", "/var/mqm/errors") log.Debugf("/var/mqm/errors:\n%s", out) // Print out summary of any FDCs + // #nosec G204 cmd := exec.Command("/opt/mqm/bin/ffstsummary") cmd.Dir = "/var/mqm/errors" + // #nosec G104 outB, _ := cmd.CombinedOutput() log.Debugf("ffstsummary:\n%s", string(outB)) diff --git a/cmd/runmqserver/main.go b/cmd/runmqserver/main.go index 102c81e..4df645b 100644 --- a/cmd/runmqserver/main.go +++ b/cmd/runmqserver/main.go @@ -129,7 +129,11 @@ func doMain() error { logTermination(err) return err } - configureQueueManager() + err = configureQueueManager() + if err != nil { + logTermination(err) + return err + } enableMetrics := os.Getenv("MQ_ENABLE_METRICS") if enableMetrics == "true" || enableMetrics == "1" { @@ -145,7 +149,11 @@ func doMain() error { // Reap zombies now, just in case we've already got some signalControl <- reapNow // Write a file to indicate that chkmqready should now work as normal - ready.Set() + err = ready.Set() + if err != nil { + logTermination(err) + return err + } // Wait for terminate signal <-signalControl return nil diff --git a/cmd/runmqserver/mirror.go b/cmd/runmqserver/mirror.go index 14f67c3..19b44e7 100644 --- a/cmd/runmqserver/mirror.go +++ b/cmd/runmqserver/mirror.go @@ -139,7 +139,10 @@ func mirrorLog(ctx context.Context, wg *sync.WaitGroup, path string, fromStart b // Always start at the beginning if we've been told to go from the start if offset != 0 && !fromStart { log.Debugf("Seeking offset %v in file %v", offset, path) - f.Seek(offset, 0) + _, err = f.Seek(offset, 0) + if err != nil { + log.Errorf("Unable to return to offset %v: %v", offset, err) + } } closing := false for { @@ -159,7 +162,10 @@ func mirrorLog(ctx context.Context, wg *sync.WaitGroup, path string, fromStart b // could skip all those messages. This could happen with a very small // MQ error log size. mirrorAvailableMessages(f, mf) - f.Close() + err = f.Close() + if err != nil { + log.Errorf("Unable to close mirror file handle: %v", err) + } // Re-open file log.Debugf("Re-opening error log file %v", path) f, err = os.OpenFile(path, os.O_RDONLY, 0) diff --git a/cmd/runmqserver/mqconfig.go b/cmd/runmqserver/mqconfig.go index da8ed2d..143415f 100644 --- a/cmd/runmqserver/mqconfig.go +++ b/cmd/runmqserver/mqconfig.go @@ -24,19 +24,20 @@ import ( "github.com/genuinetools/amicontained/container" ) -func logContainerRuntime() error { +func logContainerRuntime() { r, err := container.DetectRuntime() if err != nil { - return err + log.Printf("Failed to get container runtime: %v", err) + return } log.Printf("Container runtime: %v", r) - return nil } -func logBaseImage() error { +func logBaseImage() { buf, err := ioutil.ReadFile("/etc/os-release") if err != nil { - return err + log.Printf("Failed to read /etc/os-release: %v", err) + return } lines := strings.Split(string(buf), "\n") for _, l := range lines { @@ -44,41 +45,40 @@ func logBaseImage() error { words := strings.Split(l, "\"") if len(words) >= 2 { log.Printf("Base image: %v", words[1]) - return nil + return } } } - return nil } // logCapabilities logs the Linux capabilities (e.g. setuid, setgid). See https://docs.docker.com/engine/reference/run/#runtime-privilege-and-linux-capabilities -func logCapabilities() error { +func logCapabilities() { caps, err := container.Capabilities() if err != nil { - return err + log.Printf("Failed to get container capabilities: %v", err) + return } for k, v := range caps { if len(v) > 0 { log.Printf("Capabilities (%s set): %v", strings.ToLower(k), strings.Join(v, ",")) } } - return nil } // logSeccomp logs the seccomp enforcing mode, which affects which kernel calls can be made -func logSeccomp() error { +func logSeccomp() { s, err := container.SeccompEnforcingMode() if err != nil { - return err + log.Printf("Failed to get container SeccompEnforcingMode: %v", err) + return } log.Printf("seccomp enforcing mode: %v", s) - return nil } // logSecurityAttributes logs the security attributes of the current process. // The security attributes indicate whether AppArmor or SELinux are being used, // and what the level of confinement is. -func logSecurityAttributes() error { +func logSecurityAttributes() { a, err := readProc("/proc/self/attr/current") // On some systems, if AppArmor or SELinux are not installed, you get an // error when you try and read `/proc/self/attr/current`, even though the @@ -87,10 +87,10 @@ func logSecurityAttributes() error { a = "none" } log.Printf("Process security attributes: %v", a) - return nil } func readProc(filename string) (value string, err error) { + // #nosec G304 buf, err := ioutil.ReadFile(filename) if err != nil { return "", err diff --git a/cmd/runmqserver/qmgr.go b/cmd/runmqserver/qmgr.go index ec9138d..ffabdc9 100644 --- a/cmd/runmqserver/qmgr.go +++ b/cmd/runmqserver/qmgr.go @@ -90,6 +90,7 @@ func configureQueueManager() error { for _, file := range files { if strings.HasSuffix(file.Name(), ".mqsc") { abs := filepath.Join(configDir, file.Name()) + // #nosec G204 cmd := exec.Command("runmqsc") stdin, err := cmd.StdinPipe() if err != nil { @@ -97,6 +98,7 @@ func configureQueueManager() error { return err } // Open the MQSC file for reading + // #nosec G304 f, err := os.Open(abs) if err != nil { log.Printf("Error opening %v: %v", abs, err) @@ -104,10 +106,16 @@ func configureQueueManager() error { // Copy the contents to stdin of the runmqsc process _, err = io.Copy(stdin, f) if err != nil { - log.Printf("Error reading %v: %v", abs, err) + log.Errorf("Error reading %v: %v", abs, err) + } + err = f.Close() + if err != nil { + log.Errorf("Failed to close MQSC file handle: %v", err) + } + err = stdin.Close() + if err != nil { + log.Errorf("Failed to close MQSC stdin: %v", err) } - f.Close() - stdin.Close() // Run the command and wait for completion out, err := cmd.CombinedOutput() if err != nil { diff --git a/cmd/runmqserver/signals.go b/cmd/runmqserver/signals.go index 47c3dfb..003565d 100644 --- a/cmd/runmqserver/signals.go +++ b/cmd/runmqserver/signals.go @@ -43,7 +43,8 @@ func signalHandler(qmgr string) chan int { log.Printf("Signal received: %v", sig) signal.Stop(reapSignals) signal.Stop(stopSignals) - metrics.StopMetricsGathering() + metrics.StopMetricsGathering(log) + // #nosec G104 stopQueueManager(qmgr) // One final reap reapZombies() diff --git a/internal/command/command.go b/internal/command/command.go index 024b877..7790c55 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -53,11 +53,13 @@ func RunCmd(cmd *exec.Cmd) (string, int, error) { // Do not use this function to run shell built-ins (like "cd"), because // the error handling works differently func Run(name string, arg ...string) (string, int, error) { + // #nosec G204 return RunCmd(exec.Command(name, arg...)) } // RunAsMQM runs the specified command as the mqm user func RunAsMQM(name string, arg ...string) (string, int, error) { + // #nosec G204 cmd := exec.Command(name, arg...) cmd.SysProcAttr = &syscall.SysProcAttr{} uid, gid, err := LookupMQM() diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go index 7c0aaae..0947465 100644 --- a/internal/metrics/metrics.go +++ b/internal/metrics/metrics.go @@ -44,7 +44,7 @@ func GatherMetrics(qmName string, log *logger.Logger) { err := startMetricsGathering(qmName, log) if err != nil { log.Errorf("Metrics Error: %s", err.Error()) - StopMetricsGathering() + StopMetricsGathering(log) } } @@ -76,6 +76,7 @@ func startMetricsGathering(qmName string, log *logger.Logger) error { http.Handle("/metrics", prometheus.Handler()) http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) + // #nosec G104 w.Write([]byte("Status: METRICS ACTIVE")) }) @@ -83,7 +84,7 @@ func startMetricsGathering(qmName string, log *logger.Logger) error { err = metricsServer.ListenAndServe() if err != nil && err != http.ErrServerClosed { log.Errorf("Metrics Error: Failed to handle metrics request: %v", err) - StopMetricsGathering() + StopMetricsGathering(log) } }() @@ -91,7 +92,7 @@ func startMetricsGathering(qmName string, log *logger.Logger) error { } // StopMetricsGathering stops gathering metrics for the queue manager -func StopMetricsGathering() { +func StopMetricsGathering(log *logger.Logger) { if metricsEnabled { @@ -101,6 +102,9 @@ func StopMetricsGathering() { // Shutdown HTTP server timeout, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - metricsServer.Shutdown(timeout) + err := metricsServer.Shutdown(timeout) + if err != nil { + log.Errorf("Failed to shutdown metrics server: %v", err) + } } } diff --git a/internal/metrics/update.go b/internal/metrics/update.go index 9aceceb..f2f470a 100644 --- a/internal/metrics/update.go +++ b/internal/metrics/update.go @@ -62,6 +62,7 @@ func processMetrics(log *logger.Logger, qmName string) { firstConnect = false startChannel <- true } + // #nosec G104 metrics, _ = initialiseMetrics(log) }