From 9c95ed5f6f4dbdaa66bf6f343bad3a2520e11399 Mon Sep 17 00:00:00 2001 From: Angus Lees Date: Mon, 15 Oct 2018 15:30:57 +1100 Subject: [PATCH] Don't misinterpret command output as string format Previous code tried to interpret the string output as a `fmt.Sprintf` string formatter, mangling `%` into `%!(MISSING)`, etc. Thanks to go, I don't belive these are exploitable (unlike the similar error in C). Since this seemed to be a common error in the codebase, I did a quick audit by visually inspecting the results of `git grep 'f([^"]'`. I don't have a good suggestion for automated tests to prevent this in future :( Example error (look for `(MISSING)`): ``` E0927 05:31:07.618429 11227 driver-call.go:237] Failed to unmarshal output for command: unmount, output: "2018-09-27 05:31:07.191711 I | exec: Running command: df --type ceph /var/lib/kubelet/pods/95461479-c216-11e8-bcf0-02030782ac80/volumes/ceph.rook.io~rook/oe-scratch\n2018-09-27 05:46:43.808596 I | Filesystem 1K-blocks Used Available Use%!M(MISSING)ounted on\n2018-09-27 05:46:43.808659 I | 10.107.25.147:6790,10.109.173.79:6790,10.104.85.255:6790:/ 151678976 49410048 102268928 33%!/(MISSING)var/lib/kubelet/pods/95461479-c216-11e8-bcf0-02030782ac80/volumes/ceph.rook.io~rook/oe-scratch\n{\"status\":\"Success\"}\n", error: invalid character '-' after top-level value ``` Signed-off-by: Angus Lees --- pkg/daemon/ceph/osd/agent.go | 2 +- pkg/operator/ceph/agent/agent.go | 2 +- pkg/operator/ceph/object/rgw.go | 8 ++++---- pkg/util/exec/exec.go | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/daemon/ceph/osd/agent.go b/pkg/daemon/ceph/osd/agent.go index 1422a34fd..c7945e8ab 100644 --- a/pkg/daemon/ceph/osd/agent.go +++ b/pkg/daemon/ceph/osd/agent.go @@ -208,7 +208,7 @@ func (a *OsdAgent) removeDevices(context *clusterd.Context, removedDevicesScheme if len(errorMessages) > 0 { // at least one OSD failed, return an overall error - return fmt.Errorf(strings.Join(errorMessages, "\n")) + return fmt.Errorf("%s", strings.Join(errorMessages, "\n")) } return nil diff --git a/pkg/operator/ceph/agent/agent.go b/pkg/operator/ceph/agent/agent.go index b175f2c48..f7e4648ae 100644 --- a/pkg/operator/ceph/agent/agent.go +++ b/pkg/operator/ceph/agent/agent.go @@ -192,7 +192,7 @@ func (a *Agent) discoverFlexvolumeDir() (flexvolumeDirPath, source string) { // determining where the path of the flexvolume dir on the node nodeConfigURI, err := k8sutil.NodeConfigURI() if err != nil { - logger.Warningf(err.Error()) + logger.Warning(err.Error()) return getDefaultFlexvolumeDir() } nodeConfig, err := a.clientset.CoreV1().RESTClient().Get().RequestURI(nodeConfigURI).DoRaw() diff --git a/pkg/operator/ceph/object/rgw.go b/pkg/operator/ceph/object/rgw.go index 54a5ebf17..263a702f1 100644 --- a/pkg/operator/ceph/object/rgw.go +++ b/pkg/operator/ceph/object/rgw.go @@ -98,11 +98,11 @@ func startRGWPods(context *clusterd.Context, store cephv1beta1.ObjectStore, vers if update { err := k8sutil.DeleteDeployment(context.Clientset, store.Namespace, instanceName(store)) if err != nil { - logger.Warningf(err.Error()) + logger.Warning(err.Error()) } err = k8sutil.DeleteDaemonset(context.Clientset, store.Namespace, instanceName(store)) if err != nil { - logger.Warningf(err.Error()) + logger.Warning(err.Error()) } } @@ -157,11 +157,11 @@ func DeleteStore(context *clusterd.Context, store cephv1beta1.ObjectStore) error // Make a best effort to delete the rgw pods err = k8sutil.DeleteDeployment(context.Clientset, store.Namespace, instanceName(store)) if err != nil { - logger.Warningf(err.Error()) + logger.Warning(err.Error()) } err = k8sutil.DeleteDaemonset(context.Clientset, store.Namespace, instanceName(store)) if err != nil { - logger.Warningf(err.Error()) + logger.Warning(err.Error()) } // Delete the rgw keyring diff --git a/pkg/util/exec/exec.go b/pkg/util/exec/exec.go index 60ea20e46..450808121 100644 --- a/pkg/util/exec/exec.go +++ b/pkg/util/exec/exec.go @@ -154,7 +154,7 @@ func (*CommandExecutor) ExecuteCommandWithOutputFile(debug bool, actionName stri cmdOut, err := cmd.CombinedOutput() // if there was anything that went to stdout/stderr then log it, even before we return an error if string(cmdOut) != "" { - logger.Infof(string(cmdOut)) + logger.Info(string(cmdOut)) } if err != nil { return string(cmdOut), err @@ -203,7 +203,7 @@ func logOutput(name string, stdout, stderr io.ReadCloser) { lastLine := "" for in.Scan() { lastLine = in.Text() - childLogger.Infof(lastLine) + childLogger.Info(lastLine) } }