Merge pull request from GHSA-g5v4-5x39-vwhx

* check hard link * no following symbolic link * bug fix * add initial test to reproduce GHSA-g5v4-5x39-vwhx Signed-off-by: jdolitsky <[email protected]> * fix test for symbolic link * fix bug * add test for hardlink Signed-off-by: jdolitsky <[email protected]> * catch the parent folder * remove check for hard link for consistency * remove unncessary test for hard links * Revert "remove unncessary test for hard links" This reverts commit b3136611810f49074dfc6aef158b3d24466d2ed9. * Revert "remove check for hard link for consistency" This reverts commit d7b7346598c92ff9c430a42763d810b34d3f1ac2. * check links for all link types * add tests Co-authored-by: jdolitsky <[email protected]>
oras-project · Jan 22, 2021 · 96cd904 · 96cd904
1 parent 200d032
commit 96cd904
Show file tree

Hide file tree

Showing 2 changed files with 170 additions and 4 deletions.
diff --git a/pkg/content/utils.go b/pkg/content/utils.go
@@ -101,15 +101,24 @@ func extractTarDirectory(root, prefix string, r io.Reader) error {
 
 		// Name check
 		name := header.Name
-		path, err := filepath.Rel(prefix, name)
+		path, err := ensureBasePath(root, prefix, name)
 		if err != nil {
 			return err
 		}
-		if strings.HasPrefix(path, "../") {
-			return fmt.Errorf("%q does not have prefix %q", name, prefix)
-		}
 		path = filepath.Join(root, path)
 
+		// Link check
+		switch header.Typeflag {
+		case tar.TypeLink, tar.TypeSymlink:
+			link := header.Linkname
+			if !filepath.IsAbs(link) {
+				link = filepath.Join(filepath.Dir(name), link)
+			}
+			if _, err := ensureBasePath(root, prefix, link); err != nil {
+				return err
+			}
+		}
+
 		// Create content
 		switch header.Typeflag {
 		case tar.TypeReg:
@@ -132,6 +141,34 @@ func extractTarDirectory(root, prefix string, r io.Reader) error {
 	}
 }
 
+// ensureBasePath ensures the target path is in the base path,
+// returning its relative path to the base path.
+func ensureBasePath(root, base, target string) (string, error) {
+	path, err := filepath.Rel(base, target)
+	if err != nil {
+		return "", err
+	}
+	cleanPath := filepath.ToSlash(filepath.Clean(path))
+	if cleanPath == ".." || strings.HasPrefix(cleanPath, "../") {
+		return "", fmt.Errorf("%q is outside of %q", target, base)
+	}
+
+	// No symbolic link allowed in the relative path
+	dir := filepath.Dir(path)
+	for dir != "." {
+		if info, err := os.Lstat(filepath.Join(root, dir)); err != nil {
+			if !os.IsNotExist(err) {
+				return "", err
+			}
+		} else if info.Mode()&os.ModeSymlink != 0 {
+			return "", fmt.Errorf("no symbolic link allowed between %q and %q", base, target)
+		}
+		dir = filepath.Dir(dir)
+	}
+
+	return path, nil
+}
+
 func writeFile(path string, r io.Reader, perm os.FileMode) error {
 	file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm)
 	if err != nil {

diff --git a/pkg/oras/oras_test.go b/pkg/oras/oras_test.go
@@ -1,8 +1,13 @@
 package oras
 
 import (
+	"archive/tar"
+	"bytes"
+	"compress/gzip"
 	"context"
+	_ "crypto/sha256"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -17,6 +22,7 @@ import (
 	"github.com/docker/distribution/configuration"
 	"github.com/docker/distribution/registry"
 	_ "github.com/docker/distribution/registry/storage/driver/inmemory"
+	digest "github.com/opencontainers/go-digest"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/phayes/freeport"
 	"github.com/stretchr/testify/suite"
@@ -313,6 +319,129 @@ func (suite *ORASTestSuite) Test_3_Conditional_Pull() {
 	}
 }
 
+// Test for vulnerability GHSA-g5v4-5x39-vwhx
+func (suite *ORASTestSuite) Test_4_GHSA_g5v4_5x39_vwhx() {
+	var testVulnerability = func(headers []tar.Header, tag string, expectedError string) {
+		// Step 1: build malicious tar+gzip
+		buf := bytes.NewBuffer(nil)
+		digester := digest.Canonical.Digester()
+		zw := gzip.NewWriter(io.MultiWriter(buf, digester.Hash()))
+		tarDigester := digest.Canonical.Digester()
+		tw := tar.NewWriter(io.MultiWriter(zw, tarDigester.Hash()))
+		for _, header := range headers {
+			err := tw.WriteHeader(&header)
+			suite.Nil(err, "error writing header")
+		}
+		err := tw.Close()
+		suite.Nil(err, "error closing tar")
+		err = zw.Close()
+		suite.Nil(err, "error closing gzip")
+
+		// Step 2: construct malicious descriptor
+		evilDesc := ocispec.Descriptor{
+			MediaType: ocispec.MediaTypeImageLayerGzip,
+			Digest:    digester.Digest(),
+			Size:      int64(buf.Len()),
+			Annotations: map[string]string{
+				orascontent.AnnotationDigest: tarDigester.Digest().String(),
+				orascontent.AnnotationUnpack: "true",
+				ocispec.AnnotationTitle:      "foo",
+			},
+		}
+
+		// Step 3: upload malicious artifact to registry
+		memoryStore := orascontent.NewMemoryStore()
+		memoryStore.Set(evilDesc, buf.Bytes())
+		ref := fmt.Sprintf("%s/evil:%s", suite.DockerRegistryHost, tag)
+		_, err = Push(newContext(), newResolver(), ref, memoryStore, []ocispec.Descriptor{evilDesc})
+		suite.Nil(err, "no error pushing test data")
+
+		// Step 4: pull malicious tar with oras filestore and ensure error
+		tempDir, err := ioutil.TempDir("", "oras_test")
+		if err != nil {
+			suite.FailNow("error creating temp directory", err)
+		}
+		defer os.RemoveAll(tempDir)
+		store := orascontent.NewFileStore(tempDir)
+		defer store.Close()
+		ref = fmt.Sprintf("%s/evil:%s", suite.DockerRegistryHost, tag)
+		_, _, err = Pull(newContext(), newResolver(), ref, store)
+		suite.NotNil(err, "error expected pulling malicious tar")
+		suite.Contains(err.Error(),
+			expectedError,
+			"did not get correct error message",
+		)
+	}
+
+	tests := []struct {
+		name          string
+		headers       []tar.Header
+		tag           string
+		expectedError string
+	}{
+		{
+			name: "Test symbolic link path traversal",
+			headers: []tar.Header{
+				{
+					Typeflag: tar.TypeDir,
+					Name:     "foo/subdir/",
+					Mode:     0755,
+				},
+				{ // Symbolic link to `foo`
+					Typeflag: tar.TypeSymlink,
+					Name:     "foo/subdir/parent",
+					Linkname: "..",
+					Mode:     0755,
+				},
+				{ // Symbolic link to `../etc/passwd`
+					Typeflag: tar.TypeSymlink,
+					Name:     "foo/subdir/parent/passwd",
+					Linkname: "../../etc/passwd",
+					Mode:     0644,
+				},
+				{ // Symbolic link to `../etc`
+					Typeflag: tar.TypeSymlink,
+					Name:     "foo/subdir/parent/etc",
+					Linkname: "../../etc",
+					Mode:     0644,
+				},
+			},
+			tag:           "symlink_path",
+			expectedError: "no symbolic link allowed",
+		},
+		{
+			name: "Test symbolic link pointing to outside",
+			headers: []tar.Header{
+				{ // Symbolic link to `/etc/passwd`
+					Typeflag: tar.TypeSymlink,
+					Name:     "foo/passwd",
+					Linkname: "../../../etc/passwd",
+					Mode:     0644,
+				},
+			},
+			tag:           "symlink",
+			expectedError: "is outside of",
+		},
+		{
+			name: "Test hard link pointing to outside",
+			headers: []tar.Header{
+				{ // Hard link to `/etc/passwd`
+					Typeflag: tar.TypeLink,
+					Name:     "foo/passwd",
+					Linkname: "../../../etc/passwd",
+					Mode:     0644,
+				},
+			},
+			tag:           "hardlink",
+			expectedError: "is outside of",
+		},
+	}
+	for _, test := range tests {
+		suite.T().Log(test.name)
+		testVulnerability(test.headers, test.tag, test.expectedError)
+	}
+}
+
 func TestORASTestSuite(t *testing.T) {
 	suite.Run(t, new(ORASTestSuite))
 }