fix: Fix security issues related to file closing and paths (G307 & G3…

…04) (#6200) Signed-off-by: Yuan Tang <[email protected]>
argoproj · Jun 23, 2021 · 4fd38fa · 4fd38fa
1 parent 1f5a804
commit 4fd38fa
Show file tree

Hide file tree

Showing 21 changed files with 68 additions and 35 deletions.
diff --git a/cmd/argo/lint/lint.go b/cmd/argo/lint/lint.go
@@ -130,11 +130,15 @@ func Lint(ctx context.Context, opts *LintOptions) (*LintResults, error) {
 			case err != nil:
 				return err
 			case strings.HasPrefix(path, "/dev/") || lintExt[filepath.Ext(path)]:
-				f, err := os.Open(path)
+				f, err := os.Open(filepath.Clean(path))
 				if err != nil {
 					return err
 				}
-				defer f.Close()
+				defer func() {
+					if err := f.Close(); err != nil {
+						log.Fatalf("Error closing file[%s]: %v", path, err)
+					}
+				}()
 				r = f
 			case info.IsDir():
 				return nil // skip

diff --git a/cmd/argoexec/commands/emissary.go b/cmd/argoexec/commands/emissary.go
@@ -78,7 +78,7 @@ func NewEmissaryCommand() *cobra.Command {
 					for _, y := range x.Dependencies {
 						logger.Infof("waiting for dependency %q", y)
 						for {
-							data, err := ioutil.ReadFile(varRunArgo + "/ctr/" + y + "/exitcode")
+							data, err := ioutil.ReadFile(filepath.Clean(varRunArgo + "/ctr/" + y + "/exitcode"))
 							if os.IsNotExist(err) {
 								time.Sleep(time.Second)
 								continue
@@ -132,7 +132,7 @@ func NewEmissaryCommand() *cobra.Command {
 
 			go func() {
 				for {
-					data, _ := ioutil.ReadFile(varRunArgo + "/ctr/" + containerName + "/signal")
+					data, _ := ioutil.ReadFile(filepath.Clean(varRunArgo + "/ctr/" + containerName + "/signal"))
 					_ = os.Remove(varRunArgo + "/ctr/" + containerName + "/signal")
 					s, _ := strconv.Atoi(string(data))
 					if s > 0 {
@@ -212,7 +212,7 @@ func saveParameter(srcPath string) error {
 		logger.Infof("no need to save parameter - on overlapping volume: %s", srcPath)
 		return nil
 	}
-	src, err := os.Open(srcPath)
+	src, err := os.Open(filepath.Clean(srcPath))
 	if os.IsNotExist(err) { // might be optional, so we ignore
 		logger.WithError(err).Errorf("cannot save parameter %s", srcPath)
 		return nil

diff --git a/examples/validator.go b/examples/validator.go
@@ -44,7 +44,7 @@ func ValidateArgoYamlRecursively(fromPath string, skipFileNames []string) (map[s
 		if filepath.Ext(path) != ".yaml" {
 			return nil
 		}
-		yamlBytes, err := ioutil.ReadFile(path)
+		yamlBytes, err := ioutil.ReadFile(filepath.Clean(path))
 		if err != nil {
 			return err
 		}

diff --git a/hack/crds.go b/hack/crds.go
@@ -2,12 +2,13 @@ package main
 
 import (
 	"io/ioutil"
+	"path/filepath"
 
 	"sigs.k8s.io/yaml"
 )
 
 func cleanCRD(filename string) {
-	data, err := ioutil.ReadFile(filename)
+	data, err := ioutil.ReadFile(filepath.Clean(filename))
 	if err != nil {
 		panic(err)
 	}
@@ -52,7 +53,7 @@ func cleanCRD(filename string) {
 }
 
 func removeCRDValidation(filename string) {
-	data, err := ioutil.ReadFile(filename)
+	data, err := ioutil.ReadFile(filepath.Clean(filename))
 	if err != nil {
 		panic(err)
 	}

diff --git a/hack/docgen.go b/hack/docgen.go
@@ -228,7 +228,7 @@ func (c *DocGeneratorContext) loadFiles() {
 		panic(err)
 	}
 	for _, fileName := range files {
-		bytes, err := ioutil.ReadFile(fileName)
+		bytes, err := ioutil.ReadFile(filepath.Clean(fileName))
 		if err != nil {
 			panic(err)
 		}

diff --git a/hack/swagger/kubeifyswagger.go b/hack/swagger/kubeifyswagger.go
@@ -3,11 +3,12 @@ package main
 import (
 	"encoding/json"
 	"os"
+	"path/filepath"
 	"reflect"
 )
 
 func kubeifySwagger(in, out string) {
-	f, err := os.Open(in)
+	f, err := os.Open(filepath.Clean(in))
 	if err != nil {
 		panic(err)
 	}

diff --git a/pkg/apis/workflow/v1alpha1/marshall.go b/pkg/apis/workflow/v1alpha1/marshall.go
@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
+	"path/filepath"
 
 	"sigs.k8s.io/yaml"
 )
@@ -18,7 +19,7 @@ func MustUnmarshal(text, v interface{}) {
 	case []byte:
 		if x[0] == '@' {
 			filename := string(x[1:])
-			y, err := ioutil.ReadFile(filename)
+			y, err := ioutil.ReadFile(filepath.Clean(filename))
 			if err != nil {
 				panic(fmt.Errorf("failed to read file %s: %w", filename, err))
 			}

diff --git a/server/artifacts/artifact_server.go b/server/artifacts/artifact_server.go
@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"os"
 	"path"
+	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
@@ -183,12 +184,16 @@ func (a *ArtifactServer) returnArtifact(ctx context.Context, w http.ResponseWrit
 		return err
 	}
 
-	file, err := os.Open(tmpPath)
+	file, err := os.Open(filepath.Clean(tmpPath))
 	if err != nil {
 		return err
 	}
 
-	defer file.Close()
+	defer func() {
+		if err := file.Close(); err != nil {
+			log.Fatalf("Error closing file[%s]: %v", tmpPath, err)
+		}
+	}()
 
 	stats, err := file.Stat()
 	if err != nil {

diff --git a/test/e2e/fixtures/given.go b/test/e2e/fixtures/given.go
@@ -3,6 +3,7 @@ package fixtures
 import (
 	"fmt"
 	"io/ioutil"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -71,7 +72,7 @@ func (g *Given) readResource(text string, v metav1.Object) {
 	}
 
 	{
-		file, err := ioutil.ReadFile(file)
+		file, err := ioutil.ReadFile(filepath.Clean(file))
 		if err != nil {
 			g.t.Fatal(err)
 		}

diff --git a/test/e2e/fixtures/util.go b/test/e2e/fixtures/util.go
@@ -6,6 +6,7 @@ import (
 	"io/ioutil"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"strings"
 	"time"
 
@@ -58,7 +59,7 @@ func LoadObject(text string) (runtime.Object, error) {
 	var yaml string
 	if strings.HasPrefix(text, "@") {
 		file := strings.TrimPrefix(text, "@")
-		f, err := ioutil.ReadFile(file)
+		f, err := ioutil.ReadFile(filepath.Clean(file))
 		if err != nil {
 			return nil, err
 		}

diff --git a/util/archive/archive.go b/util/archive/archive.go
@@ -92,7 +92,7 @@ func tarDir(sourcePath string, tw *tar.Writer) error {
 		if !info.Mode().IsRegular() {
 			return nil
 		}
-		f, err := os.Open(fpath)
+		f, err := os.Open(filepath.Clean(fpath))
 		if err != nil {
 			return errors.InternalWrapError(err)
 		}
@@ -113,7 +113,7 @@ func tarDir(sourcePath string, tw *tar.Writer) error {
 }
 
 func tarFile(sourcePath string, tw *tar.Writer) error {
-	f, err := os.Open(sourcePath)
+	f, err := os.Open(filepath.Clean(sourcePath))
 	if err != nil {
 		return errors.InternalWrapError(err)
 	}

diff --git a/workflow/artifacts/artifactory/artifactory.go b/workflow/artifacts/artifactory/artifactory.go
@@ -5,6 +5,7 @@ import (
 	"io"
 	"net/http"
 	"os"
+	"path/filepath"
 
 	"github.com/argoproj/argo-workflows/v3/errors"
 	wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
@@ -54,7 +55,7 @@ func (a *ArtifactDriver) Load(artifact *wfv1.Artifact, path string) error {
 
 // UpLoad artifact to an artifactory URL
 func (a *ArtifactDriver) Save(path string, artifact *wfv1.Artifact) error {
-	f, err := os.Open(path)
+	f, err := os.Open(filepath.Clean(path))
 	if err != nil {
 		return err
 	}

diff --git a/workflow/artifacts/gcs/gcs.go b/workflow/artifacts/gcs/gcs.go
@@ -124,7 +124,11 @@ func downloadObject(client *storage.Client, bucket, key, objName, path string) e
 	if err != nil {
 		return fmt.Errorf("os create %s: %v", localPath, err)
 	}
-	defer out.Close()
+	defer func() {
+		if err := out.Close(); err != nil {
+			log.Fatalf("Error closing file[%s]: %v", localPath, err)
+		}
+	}()
 	_, err = io.Copy(out, rc)
 	if err != nil {
 		return fmt.Errorf("io copy: %v", err)
@@ -236,11 +240,15 @@ func uploadObjects(client *storage.Client, bucket, key, path string) error {
 
 // upload an object to GCS
 func uploadObject(client *storage.Client, bucket, key, localPath string) error {
-	f, err := os.Open(localPath)
+	f, err := os.Open(filepath.Clean(localPath))
 	if err != nil {
 		return fmt.Errorf("os open: %v", err)
 	}
-	defer f.Close()
+	defer func() {
+		if err := f.Close(); err != nil {
+			log.Fatalf("Error closing file[%s]: %v", localPath, err)
+		}
+	}()
 	ctx := context.Background()
 	wc := client.Bucket(bucket).Object(key).NewWriter(ctx)
 	if _, err = io.Copy(wc, f); err != nil {

diff --git a/workflow/executor/common/common.go b/workflow/executor/common/common.go
@@ -6,6 +6,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"path/filepath"
 	"strings"
 	"syscall"
 	"time"
@@ -152,7 +153,7 @@ func CopyArchive(ctx context.Context, c KubernetesClientInterface, containerName
 	if err != nil {
 		return err
 	}
-	f, err := os.OpenFile(destPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0o666)
+	f, err := os.OpenFile(filepath.Clean(destPath), os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0o666)
 	if err != nil {
 		return err
 	}

diff --git a/workflow/executor/docker/docker.go b/workflow/executor/docker/docker.go
@@ -9,6 +9,7 @@ import (
 	"io/ioutil"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"runtime"
 	"strconv"
 	"strings"
@@ -73,7 +74,7 @@ func (d *DockerExecutor) CopyFile(containerName string, sourcePath string, destP
 	if err != nil {
 		return err
 	}
-	copiedFile, err := os.Open(destPath)
+	copiedFile, err := os.Open(filepath.Clean(destPath))
 	if err != nil {
 		return err
 	}

diff --git a/workflow/executor/emissary/binary.go b/workflow/executor/emissary/binary.go
@@ -4,14 +4,15 @@ import (
 	"io"
 	"os"
 	"os/exec"
+	"path/filepath"
 )
 
 func copyBinary() error {
 	name, err := exec.LookPath("argoexec")
 	if err != nil {
 		return err
 	}
-	in, err := os.Open(name)
+	in, err := os.Open(filepath.Clean(name))
 	if err != nil {
 		return err
 	}

diff --git a/workflow/executor/emissary/emissary.go b/workflow/executor/emissary/emissary.go
@@ -66,7 +66,7 @@ func (e emissary) writeTemplate(t wfv1.Template) error {
 }
 
 func (e emissary) GetFileContents(_ string, sourcePath string) (string, error) {
-	data, err := ioutil.ReadFile(filepath.Join("/var/run/argo/outputs/parameters", sourcePath))
+	data, err := ioutil.ReadFile(filepath.Clean(filepath.Join("/var/run/argo/outputs/parameters", sourcePath)))
 	return string(data), err
 }
 
@@ -76,7 +76,7 @@ func (e emissary) CopyFile(_ string, sourcePath string, destPath string, _ int)
 	// TODO - warn the user we ignored compression?
 	sourceFile := filepath.Join("/var/run/argo/outputs/artifacts", sourcePath+".tgz")
 	log.Infof("%s -> %s", sourceFile, destPath)
-	src, err := os.Open(sourceFile)
+	src, err := os.Open(filepath.Clean(sourceFile))
 	if err != nil {
 		return err
 	}
@@ -100,7 +100,7 @@ func (e emissary) GetOutputStream(_ context.Context, containerName string, combi
 	}
 	var files []io.ReadCloser
 	for _, name := range names {
-		f, err := os.Open("/var/run/argo/ctr/" + containerName + "/" + name)
+		f, err := os.Open(filepath.Clean("/var/run/argo/ctr/" + containerName + "/" + name))
 		if os.IsNotExist(err) {
 			continue
 		} else if err != nil {

diff --git a/workflow/executor/executor.go b/workflow/executor/executor.go
@@ -501,7 +501,7 @@ func (we *WorkflowExecutor) SaveParameters(ctx context.Context) error {
 		} else {
 			log.Infof("Copying %s from volume mount", param.ValueFrom.Path)
 			mountedPath := filepath.Join(common.ExecutorMainFilesystemDir, param.ValueFrom.Path)
-			data, err := ioutil.ReadFile(mountedPath)
+			data, err := ioutil.ReadFile(filepath.Clean(mountedPath))
 			if err != nil {
 				// We have a default value to use instead of returning an error
 				if param.ValueFrom.Default != nil {
@@ -549,7 +549,7 @@ func (we *WorkflowExecutor) SaveLogs(ctx context.Context) (*wfv1.Artifact, error
 
 // GetSecret will retrieve the Secrets from VolumeMount
 func (we *WorkflowExecutor) GetSecret(ctx context.Context, accessKeyName string, accessKey string) (string, error) {
-	file, err := ioutil.ReadFile(filepath.Join(common.SecretVolMountPath, accessKeyName, accessKey))
+	file, err := ioutil.ReadFile(filepath.Clean(filepath.Join(common.SecretVolMountPath, accessKeyName, accessKey)))
 	if err != nil {
 		return "", err
 	}
@@ -750,11 +750,15 @@ func (we *WorkflowExecutor) AddAnnotation(ctx context.Context, key, value string
 // isTarball returns whether or not the file is a tarball
 func isTarball(filePath string) (bool, error) {
 	log.Infof("Detecting if %s is a tarball", filePath)
-	f, err := os.Open(filePath)
+	f, err := os.Open(filepath.Clean(filePath))
 	if err != nil {
 		return false, err
 	}
-	defer f.Close()
+	defer func() {
+		if err := f.Close(); err != nil {
+			log.Fatalf("Error closing file[%s]: %v", filePath, err)
+		}
+	}()
 	gzr, err := gzip.NewReader(f)
 	if err != nil {
 		return false, nil
@@ -815,7 +819,7 @@ func unzip(zipPath string, destPath string) error {
 				if err = os.MkdirAll(filepath.Dir(path), f.Mode()); err != nil {
 					return err
 				}
-				f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode())
+				f, err := os.OpenFile(filepath.Clean(path), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode())
 				if err != nil {
 					return err
 				}

diff --git a/workflow/executor/pns/pns.go b/workflow/executor/pns/pns.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
+	"path/filepath"
 	"strings"
 	"sync"
 	"syscall"
@@ -68,7 +69,7 @@ func (p *PNSExecutor) GetFileContents(containerName string, sourcePath string) (
 		return "", err
 	}
 	defer func() { _ = p.exitChroot() }()
-	out, err := ioutil.ReadFile(sourcePath)
+	out, err := ioutil.ReadFile(filepath.Clean(sourcePath))
 	if err != nil {
 		return "", err
 	}