fix: Fix Zip Slip Vulnerability (#1864)

* Fix Zip Slip Vulnerability * Apply suggestions from code review Co-authored-by: Benjamin Lindner <50365642+lindnerby@users.noreply.github.com> --------- Co-authored-by: Benjamin Lindner <50365642+lindnerby@users.noreply.github.com>
kyma-project · Nov 27, 2023 · 9c2d8c4 · 9c2d8c4
1 parent f3eb5fb
commit 9c2d8c4
Show file tree

Hide file tree

Showing 2 changed files with 95 additions and 25 deletions.
diff --git a/internal/deploy/istioctl/istio.go b/internal/deploy/istioctl/istio.go
@@ -16,21 +16,28 @@ import (
 
 	"go.uber.org/zap"
 
-	"github.com/kyma-project/cli/internal/files"
 	"github.com/pkg/errors"
 	"gopkg.in/yaml.v3"
+
+	"github.com/kyma-project/cli/internal/files"
 )
 
-const defaultIstioChartPath = "/resources/istio/Chart.yaml"
-const archSupport = "1.6"
-const envVar = "ISTIOCTL_PATH"
-const dirName = "istio"
-const binName = "istioctl"
-const winBinName = "istioctl.exe"
-const downloadURL = "https://github.com/istio/istio/releases/download/"
-const tarGzName = "istio.tar.gz"
-const tarName = "istio.tar"
-const zipName = "istio.zip"
+const (
+	defaultIstioChartPath = "/resources/istio/Chart.yaml"
+	archSupport           = "1.6"
+	envVar                = "ISTIOCTL_PATH"
+	dirName               = "istio"
+	binName               = "istioctl"
+	winBinName            = "istioctl.exe"
+	downloadURL           = "https://github.com/istio/istio/releases/download/"
+	tarGzName             = "istio.tar.gz"
+	tarName               = "istio.tar"
+	zipName               = "istio.zip"
+)
+
+var (
+	errIstioSourcepath = errors.New("istio source path contains `..`")
+)
 
 type operatingSystem struct {
 	name string
@@ -197,7 +204,8 @@ func (i *Installation) downloadIstio() error {
 	// Istioctl download links
 	if i.osExt == darwin.ext || i.osExt == linux.ext {
 		nonArchURL = fmt.Sprintf("%s%s/istio-%s-%s.tar.gz", i.downloadURL, i.istioVersion, i.istioVersion, i.osExt)
-		archURL = fmt.Sprintf("%s%s/istio-%s-%s-%s.tar.gz", i.downloadURL, i.istioVersion, i.istioVersion, i.osExt, i.istioArch)
+		archURL = fmt.Sprintf("%s%s/istio-%s-%s-%s.tar.gz", i.downloadURL, i.istioVersion, i.istioVersion, i.osExt,
+			i.istioArch)
 	} else {
 		nonArchURL = fmt.Sprintf("%s%s/istio-%s-%s.zip", i.downloadURL, i.istioVersion, i.istioVersion, i.osExt)
 	}
@@ -334,6 +342,9 @@ func unGzip(source, target string, deleteSource bool) error {
 }
 
 func unTar(source, target string, deleteSource bool) error {
+	if strings.Contains(source, "..") {
+		return errIstioSourcepath
+	}
 	reader, err := os.Open(source)
 	if err != nil {
 		return err

diff --git a/internal/deploy/istioctl/istio_test.go b/internal/deploy/istioctl/istio_test.go
@@ -29,9 +29,24 @@ func TestInstallation_getIstioVersion(t *testing.T) {
 		expectedVersion string
 		wantErr         bool
 	}{
-		{name: "Fetch Istio Version", fields: fields{IstioChartPath: "testdata/Chart.yaml"}, expectedVersion: "1.11.2", wantErr: false},
-		{name: "Istio Chart not existing", fields: fields{IstioChartPath: "testdata/nonExisting.yaml"}, expectedVersion: "", wantErr: true},
-		{name: "Corrupted Istio Chart", fields: fields{IstioChartPath: "testdata/corruptedChart.yaml"}, expectedVersion: "", wantErr: true},
+		{
+			name:            "Fetch Istio Version",
+			fields:          fields{IstioChartPath: "testdata/Chart.yaml"},
+			expectedVersion: "1.11.2",
+			wantErr:         false,
+		},
+		{
+			name:            "Istio Chart not existing",
+			fields:          fields{IstioChartPath: "testdata/nonExisting.yaml"},
+			expectedVersion: "",
+			wantErr:         true,
+		},
+		{
+			name:            "Corrupted Istio Chart",
+			fields:          fields{IstioChartPath: "testdata/corruptedChart.yaml"},
+			expectedVersion: "",
+			wantErr:         true,
+		},
 	}
 	for _, tt := range tests {
 		tt := tt
@@ -98,8 +113,16 @@ func Test_unGzip(t *testing.T) {
 		args    args
 		wantErr bool
 	}{
-		{name: "unGzip File", args: args{source: "testdata/istio_mock.tar.gz", target: "testdata/istio.tar", deleteSource: false}, wantErr: false},
-		{name: "File does not exist", args: args{source: "testdata/nonexistent.tar.gz", target: "testdata/istio.tar", deleteSource: false}, wantErr: true},
+		{
+			name:    "unGzip File",
+			args:    args{source: "testdata/istio_mock.tar.gz", target: "testdata/istio.tar", deleteSource: false},
+			wantErr: false,
+		},
+		{
+			name:    "File does not exist",
+			args:    args{source: "testdata/nonexistent.tar.gz", target: "testdata/istio.tar", deleteSource: false},
+			wantErr: true,
+		},
 	}
 	for _, tt := range tests {
 		tt := tt
@@ -133,8 +156,24 @@ func Test_unTar(t *testing.T) {
 		expectedFile string
 		wantErr      bool
 	}{
-		{name: "unTar File", args: args{source: "testdata/istio_mock.tar", target: "testdata", deleteSource: false}, expectedFile: "testdata/istio.txt", wantErr: false},
-		{name: "File does not exist", args: args{source: "testdata/nonexistent.tar", target: "testdata", deleteSource: false}, expectedFile: "testdata/istio.txt", wantErr: true},
+		{
+			name:         "unTar File",
+			args:         args{source: "testdata/istio_mock.tar", target: "testdata", deleteSource: false},
+			expectedFile: "testdata/istio.txt",
+			wantErr:      false,
+		},
+		{
+			name:         "File does not exist",
+			args:         args{source: "testdata/nonexistent.tar", target: "testdata", deleteSource: false},
+			expectedFile: "testdata/istio.txt",
+			wantErr:      true,
+		},
+		{
+			name:         "File path contains `..` - Zip Slip Check",
+			args:         args{source: "../istiomock.tar", target: "testdata", deleteSource: false},
+			expectedFile: "testdata/istio.txt",
+			wantErr:      true,
+		},
 	}
 	for _, tt := range tests {
 		tt := tt
@@ -167,8 +206,18 @@ func Test_unZip(t *testing.T) {
 		expectedFile string
 		wantErr      bool
 	}{
-		{name: "unZip File", args: args{source: "testdata/istio_mock.zip", target: "testdata", deleteSource: false}, expectedFile: "testdata/istio.txt", wantErr: false},
-		{name: "File does not exist", args: args{source: "testdata/nonexistent.zip", target: "testdata", deleteSource: false}, expectedFile: "testdata/istio.txt", wantErr: true},
+		{
+			name:         "unZip File",
+			args:         args{source: "testdata/istio_mock.zip", target: "testdata", deleteSource: false},
+			expectedFile: "testdata/istio.txt",
+			wantErr:      false,
+		},
+		{
+			name:         "File does not exist",
+			args:         args{source: "testdata/nonexistent.zip", target: "testdata", deleteSource: false},
+			expectedFile: "testdata/istio.txt",
+			wantErr:      true,
+		},
 	}
 	for _, tt := range tests {
 		tt := tt
@@ -216,21 +265,31 @@ func TestInstallation_downloadFile(t *testing.T) {
 		getFunc func(url string) (*http.Response, error)
 		wantErr bool
 	}{
-		{name: "Download Istioctl", fields: fields{Client: &mockClient{}}, args: args{filepath: "tmp", filename: "mock_download.txt", url: "someUrl"},
+		{
+			name:   "Download Istioctl",
+			fields: fields{Client: &mockClient{}},
+			args:   args{filepath: "tmp", filename: "mock_download.txt", url: "someUrl"},
 			getFunc: func(url string) (*http.Response, error) {
 				jsonBody := `{"name":"Istioctl","full_name":"Istioctl binary mock download","bin":{"data": "some binary"}}`
 				r := io.NopCloser(bytes.NewReader([]byte(jsonBody)))
 				return &http.Response{
 					StatusCode: 200,
 					Body:       r,
 				}, nil
-			}, wantErr: false},
-		{name: "404 - Not found", fields: fields{Client: &mockClient{}}, args: args{filepath: "tmp", filename: "mock_download.txt", url: "someUrl"},
+			},
+			wantErr: false,
+		},
+		{
+			name:   "404 - Not found",
+			fields: fields{Client: &mockClient{}},
+			args:   args{filepath: "tmp", filename: "mock_download.txt", url: "someUrl"},
 			getFunc: func(url string) (*http.Response, error) {
 				return &http.Response{
 					StatusCode: 404,
 				}, errors.New("404 - Not Found")
-			}, wantErr: true},
+			},
+			wantErr: true,
+		},
 	}
 	for _, tt := range tests {
 		tt := tt