From dd668e59f17287c71bc8dcbeeb5d3f35beb9f1bc Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Fri, 30 Aug 2019 12:23:10 +0200 Subject: [PATCH] Address security concerns reported by 'gosec' gosec reports several issues, none of them looks very critical. With this change the following concerns have been addressed: [pkg/cephfs/nodeserver.go:229] - G302: Expect file permissions to be 0600 or less (Confidence: HIGH, Severity: MEDIUM) > os.Chmod(targetPath, 0777) [pkg/cephfs/util.go:39] - G204: Subprocess launched with variable (Confidence: HIGH, Severity: MEDIUM) > exec.Command(program, args...) [pkg/rbd/nodeserver.go:156] - G302: Expect file permissions to be 0600 or less (Confidence: HIGH, Severity: MEDIUM) > os.Chmod(stagingTargetPath, 0777) [pkg/rbd/nodeserver.go:205] - G302: Expect file permissions to be 0600 or less (Confidence: HIGH, Severity: MEDIUM) > os.OpenFile(mountPath, os.O_CREATE|os.O_RDWR, 0750) [pkg/rbd/rbd_util.go:797] - G304: Potential file inclusion via variable (Confidence: HIGH, Severity: MEDIUM) > ioutil.ReadFile(fPath) [pkg/util/cephcmds.go:35] - G204: Subprocess launched with variable (Confidence: HIGH, Severity: MEDIUM) > exec.Command(program, args...) [pkg/util/credentials.go:47] - G104: Errors unhandled. (Confidence: HIGH, Severity: LOW) > os.Remove(tmpfile.Name()) [pkg/util/credentials.go:92] - G104: Errors unhandled. (Confidence: HIGH, Severity: LOW) > os.Remove(cr.KeyFile) [pkg/util/pidlimit.go:74] - G304: Potential file inclusion via variable (Confidence: HIGH, Severity: MEDIUM) > os.Open(pidsMax) URL: https://github.com/securego/gosec Signed-off-by: Niels de Vos --- pkg/cephfs/nodeserver.go | 1 + pkg/cephfs/util.go | 2 +- pkg/rbd/nodeserver.go | 3 ++- pkg/rbd/rbd_util.go | 2 +- pkg/util/cephcmds.go | 2 +- pkg/util/credentials.go | 6 ++++-- pkg/util/pidlimit.go | 2 +- 7 files changed, 11 insertions(+), 7 deletions(-) diff --git a/pkg/cephfs/nodeserver.go b/pkg/cephfs/nodeserver.go index 27494aa44..7859a06c7 100644 --- a/pkg/cephfs/nodeserver.go +++ b/pkg/cephfs/nodeserver.go @@ -226,6 +226,7 @@ func (ns *NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis klog.Infof(util.Log(ctx, "cephfs: successfully bind-mounted volume %s to %s"), volID, targetPath) + // #nosec - allow anyone to write inside the target path err = os.Chmod(targetPath, 0777) if err != nil { klog.Errorf(util.Log(ctx, "failed to change targetpath permission for volume %s: %v"), volID, err) diff --git a/pkg/cephfs/util.go b/pkg/cephfs/util.go index e1b9af8a1..c3453402b 100644 --- a/pkg/cephfs/util.go +++ b/pkg/cephfs/util.go @@ -36,7 +36,7 @@ type volumeID string func execCommand(ctx context.Context, program string, args ...string) (stdout, stderr []byte, err error) { var ( - cmd = exec.Command(program, args...) // nolint: gosec + cmd = exec.Command(program, args...) // nolint: gosec, #nosec sanitizedArgs = util.StripSecretInArgs(args) stdoutBuf bytes.Buffer stderrBuf bytes.Buffer diff --git a/pkg/rbd/nodeserver.go b/pkg/rbd/nodeserver.go index 2f2df3a3d..883f09880 100644 --- a/pkg/rbd/nodeserver.go +++ b/pkg/rbd/nodeserver.go @@ -153,6 +153,7 @@ func (ns *NodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol } isMounted = true + // #nosec - allow anyone to write inside the target path err = os.Chmod(stagingTargetPath, 0777) if err != nil { return nil, status.Error(codes.Internal, err.Error()) @@ -202,7 +203,7 @@ func (ns *NodeServer) undoStagingTransaction(ctx context.Context, stagingParentP func (ns *NodeServer) createStageMountPoint(ctx context.Context, mountPath string, isBlock bool) error { if isBlock { - pathFile, err := os.OpenFile(mountPath, os.O_CREATE|os.O_RDWR, 0750) + pathFile, err := os.OpenFile(mountPath, os.O_CREATE|os.O_RDWR, 0600) if err != nil { klog.Errorf(util.Log(ctx, "failed to create mountPath:%s with error: %v"), mountPath, err) return status.Error(codes.Internal, err.Error()) diff --git a/pkg/rbd/rbd_util.go b/pkg/rbd/rbd_util.go index 011f1345a..a9d885a45 100644 --- a/pkg/rbd/rbd_util.go +++ b/pkg/rbd/rbd_util.go @@ -794,7 +794,7 @@ func lookupRBDImageMetadataStash(path string) (rbdImageMetadataStash, error) { var imgMeta rbdImageMetadataStash fPath := filepath.Join(path, stashFileName) - encodedBytes, err := ioutil.ReadFile(fPath) + encodedBytes, err := ioutil.ReadFile(fPath) // #nosec - intended reading from fPath if err != nil { if !os.IsNotExist(err) { return imgMeta, fmt.Errorf("failed to read stashed JSON image metadata from path (%s): (%v)", fPath, err) diff --git a/pkg/util/cephcmds.go b/pkg/util/cephcmds.go index 034f95619..386cfb061 100644 --- a/pkg/util/cephcmds.go +++ b/pkg/util/cephcmds.go @@ -32,7 +32,7 @@ import ( // ExecCommand executes passed in program with args and returns separate stdout and stderr streams func ExecCommand(program string, args ...string) (stdout, stderr []byte, err error) { var ( - cmd = exec.Command(program, args...) // nolint: gosec + cmd = exec.Command(program, args...) // nolint: gosec, #nosec sanitizedArgs = StripSecretInArgs(args) stdoutBuf bytes.Buffer stderrBuf bytes.Buffer diff --git a/pkg/util/credentials.go b/pkg/util/credentials.go index 97e26d820..63f0891bb 100644 --- a/pkg/util/credentials.go +++ b/pkg/util/credentials.go @@ -44,7 +44,8 @@ func storeKey(key string) (string, error) { } defer func() { if err != nil { - os.Remove(tmpfile.Name()) + // don't complain about unhandled error + _ = os.Remove(tmpfile.Name()) } }() @@ -89,7 +90,8 @@ func newCredentialsFromSecret(idField, keyField string, secrets map[string]strin } func (cr *Credentials) DeleteCredentials() { - os.Remove(cr.KeyFile) + // don't complain about unhandled error + _ = os.Remove(cr.KeyFile) } func NewUserCredentials(secrets map[string]string) (*Credentials, error) { diff --git a/pkg/util/pidlimit.go b/pkg/util/pidlimit.go index 0a6a52f3b..c76cdf63d 100644 --- a/pkg/util/pidlimit.go +++ b/pkg/util/pidlimit.go @@ -71,7 +71,7 @@ func GetPIDLimit() (int, error) { return 0, err } - f, err := os.Open(pidsMax) + f, err := os.Open(pidsMax) // #nosec - intended reading from /sys/... if err != nil { return 0, err }