archive: add breakout tests

Signed-off-by: Tibor Vass <teabee89@gmail.com>

Conflicts:
	pkg/archive/archive.go
		fixed conflict which git couldn't fix with the added BreakoutError

archive/archive.go

@@ -41,6 +41,11 @@ type (
 	Archiver struct {
 		Untar func(io.Reader, string, *TarOptions) error
 	}
+
+	// breakoutError is used to differentiate errors related to breaking out
+	// When testing archive breakout in the unit tests, this error is expected
+	// in order for the test to pass.
+	breakoutError error
 )
 
 var (

archive/archive_test.go

@@ -8,6 +8,7 @@ import (
 	"os"
 	"os/exec"
 	"path"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -169,7 +170,12 @@ func TestTarWithOptions(t *testing.T) {
 // Failing prevents the archives from being uncompressed during ADD
 func TestTypeXGlobalHeaderDoesNotFail(t *testing.T) {
 	hdr := tar.Header{Typeflag: tar.TypeXGlobalHeader}
-	err := createTarFile("pax_global_header", "some_dir", &hdr, nil, true)
+	tmpDir, err := ioutil.TempDir("", "docker-test-archive-pax-test")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(tmpDir)
+	err = createTarFile(filepath.Join(tmpDir, "pax_global_header"), tmpDir, &hdr, nil, true)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -242,3 +248,187 @@ func BenchmarkTarUntar(b *testing.B) {
 		os.RemoveAll(target)
 	}
 }
+
+func TestUntarInvalidFilenames(t *testing.T) {
+	for i, headers := range [][]*tar.Header{
+		{
+			{
+				Name:     "../victim/dotdot",
+				Typeflag: tar.TypeReg,
+				Mode:     0644,
+			},
+		},
+		{
+			{
+				// Note the leading slash
+				Name:     "/../victim/slash-dotdot",
+				Typeflag: tar.TypeReg,
+				Mode:     0644,
+			},
+		},
+	} {
+		if err := testBreakout("untar", "docker-TestUntarInvalidFilenames", headers); err != nil {
+			t.Fatalf("i=%d. %v", i, err)
+		}
+	}
+}
+
+func TestUntarInvalidHardlink(t *testing.T) {
+	for i, headers := range [][]*tar.Header{
+		{ // try reading victim/hello (../)
+			{
+				Name:     "dotdot",
+				Typeflag: tar.TypeLink,
+				Linkname: "../victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // try reading victim/hello (/../)
+			{
+				Name:     "slash-dotdot",
+				Typeflag: tar.TypeLink,
+				// Note the leading slash
+				Linkname: "/../victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // try writing victim/file
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeLink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "loophole-victim/file",
+				Typeflag: tar.TypeReg,
+				Mode:     0644,
+			},
+		},
+		{ // try reading victim/hello (hardlink, symlink)
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeLink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "symlink",
+				Typeflag: tar.TypeSymlink,
+				Linkname: "loophole-victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // Try reading victim/hello (hardlink, hardlink)
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeLink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "hardlink",
+				Typeflag: tar.TypeLink,
+				Linkname: "loophole-victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // Try removing victim directory (hardlink)
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeLink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeReg,
+				Mode:     0644,
+			},
+		},
+	} {
+		if err := testBreakout("untar", "docker-TestUntarInvalidHardlink", headers); err != nil {
+			t.Fatalf("i=%d. %v", i, err)
+		}
+	}
+}
+
+func TestUntarInvalidSymlink(t *testing.T) {
+	for i, headers := range [][]*tar.Header{
+		{ // try reading victim/hello (../)
+			{
+				Name:     "dotdot",
+				Typeflag: tar.TypeSymlink,
+				Linkname: "../victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // try reading victim/hello (/../)
+			{
+				Name:     "slash-dotdot",
+				Typeflag: tar.TypeSymlink,
+				// Note the leading slash
+				Linkname: "/../victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // try writing victim/file
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeSymlink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "loophole-victim/file",
+				Typeflag: tar.TypeReg,
+				Mode:     0644,
+			},
+		},
+		{ // try reading victim/hello (symlink, symlink)
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeSymlink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "symlink",
+				Typeflag: tar.TypeSymlink,
+				Linkname: "loophole-victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // try reading victim/hello (symlink, hardlink)
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeSymlink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "hardlink",
+				Typeflag: tar.TypeLink,
+				Linkname: "loophole-victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // try removing victim directory (symlink)
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeSymlink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeReg,
+				Mode:     0644,
+			},
+		},
+	} {
+		if err := testBreakout("untar", "docker-TestUntarInvalidSymlink", headers); err != nil {
+			t.Fatalf("i=%d. %v", i, err)
+		}
+	}
+}

archive/diff_test.go

@@ -0,0 +1,191 @@
+package archive
+
+import (
+	"testing"
+
+	"github.com/docker/docker/vendor/src/code.google.com/p/go/src/pkg/archive/tar"
+)
+
+func TestApplyLayerInvalidFilenames(t *testing.T) {
+	for i, headers := range [][]*tar.Header{
+		{
+			{
+				Name:     "../victim/dotdot",
+				Typeflag: tar.TypeReg,
+				Mode:     0644,
+			},
+		},
+		{
+			{
+				// Note the leading slash
+				Name:     "/../victim/slash-dotdot",
+				Typeflag: tar.TypeReg,
+				Mode:     0644,
+			},
+		},
+	} {
+		if err := testBreakout("applylayer", "docker-TestApplyLayerInvalidFilenames", headers); err != nil {
+			t.Fatalf("i=%d. %v", i, err)
+		}
+	}
+}
+
+func TestApplyLayerInvalidHardlink(t *testing.T) {
+	for i, headers := range [][]*tar.Header{
+		{ // try reading victim/hello (../)
+			{
+				Name:     "dotdot",
+				Typeflag: tar.TypeLink,
+				Linkname: "../victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // try reading victim/hello (/../)
+			{
+				Name:     "slash-dotdot",
+				Typeflag: tar.TypeLink,
+				// Note the leading slash
+				Linkname: "/../victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // try writing victim/file
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeLink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "loophole-victim/file",
+				Typeflag: tar.TypeReg,
+				Mode:     0644,
+			},
+		},
+		{ // try reading victim/hello (hardlink, symlink)
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeLink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "symlink",
+				Typeflag: tar.TypeSymlink,
+				Linkname: "loophole-victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // Try reading victim/hello (hardlink, hardlink)
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeLink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "hardlink",
+				Typeflag: tar.TypeLink,
+				Linkname: "loophole-victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // Try removing victim directory (hardlink)
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeLink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeReg,
+				Mode:     0644,
+			},
+		},
+	} {
+		if err := testBreakout("applylayer", "docker-TestApplyLayerInvalidHardlink", headers); err != nil {
+			t.Fatalf("i=%d. %v", i, err)
+		}
+	}
+}
+
+func TestApplyLayerInvalidSymlink(t *testing.T) {
+	for i, headers := range [][]*tar.Header{
+		{ // try reading victim/hello (../)
+			{
+				Name:     "dotdot",
+				Typeflag: tar.TypeSymlink,
+				Linkname: "../victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // try reading victim/hello (/../)
+			{
+				Name:     "slash-dotdot",
+				Typeflag: tar.TypeSymlink,
+				// Note the leading slash
+				Linkname: "/../victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // try writing victim/file
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeSymlink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "loophole-victim/file",
+				Typeflag: tar.TypeReg,
+				Mode:     0644,
+			},
+		},
+		{ // try reading victim/hello (symlink, symlink)
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeSymlink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "symlink",
+				Typeflag: tar.TypeSymlink,
+				Linkname: "loophole-victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // try reading victim/hello (symlink, hardlink)
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeSymlink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "hardlink",
+				Typeflag: tar.TypeLink,
+				Linkname: "loophole-victim/hello",
+				Mode:     0644,
+			},
+		},
+		{ // try removing victim directory (symlink)
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeSymlink,
+				Linkname: "../victim",
+				Mode:     0755,
+			},
+			{
+				Name:     "loophole-victim",
+				Typeflag: tar.TypeReg,
+				Mode:     0644,
+			},
+		},
+	} {
+		if err := testBreakout("applylayer", "docker-TestApplyLayerInvalidSymlink", headers); err != nil {
+			t.Fatalf("i=%d. %v", i, err)
+		}
+	}
+}

archive/utils_test.go

@@ -0,0 +1,166 @@
+package archive
+
+import (
+	"bytes"
+	"fmt"
+	"io"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"time"
+
+	"github.com/docker/docker/vendor/src/code.google.com/p/go/src/pkg/archive/tar"
+)
+
+var testUntarFns = map[string]func(string, io.Reader) error{
+	"untar": func(dest string, r io.Reader) error {
+		return Untar(r, dest, nil)
+	},
+	"applylayer": func(dest string, r io.Reader) error {
+		return ApplyLayer(dest, ArchiveReader(r))
+	},
+}
+
+// testBreakout is a helper function that, within the provided `tmpdir` directory,
+// creates a `victim` folder with a generated `hello` file in it.
+// `untar` extracts to a directory named `dest`, the tar file created from `headers`.
+//
+// Here are the tested scenarios:
+// - removed `victim` folder				(write)
+// - removed files from `victim` folder			(write)
+// - new files in `victim` folder			(write)
+// - modified files in `victim` folder			(write)
+// - file in `dest` with same content as `victim/hello` (read)
+//
+// When using testBreakout make sure you cover one of the scenarios listed above.
+func testBreakout(untarFn string, tmpdir string, headers []*tar.Header) error {
+	tmpdir, err := ioutil.TempDir("", tmpdir)
+	if err != nil {
+		return err
+	}
+	defer os.RemoveAll(tmpdir)
+
+	dest := filepath.Join(tmpdir, "dest")
+	if err := os.Mkdir(dest, 0755); err != nil {
+		return err
+	}
+
+	victim := filepath.Join(tmpdir, "victim")
+	if err := os.Mkdir(victim, 0755); err != nil {
+		return err
+	}
+	hello := filepath.Join(victim, "hello")
+	helloData, err := time.Now().MarshalText()
+	if err != nil {
+		return err
+	}
+	if err := ioutil.WriteFile(hello, helloData, 0644); err != nil {
+		return err
+	}
+	helloStat, err := os.Stat(hello)
+	if err != nil {
+		return err
+	}
+
+	reader, writer := io.Pipe()
+	go func() {
+		t := tar.NewWriter(writer)
+		for _, hdr := range headers {
+			t.WriteHeader(hdr)
+		}
+		t.Close()
+	}()
+
+	untar := testUntarFns[untarFn]
+	if untar == nil {
+		return fmt.Errorf("could not find untar function %q in testUntarFns", untarFn)
+	}
+	if err := untar(dest, reader); err != nil {
+		if _, ok := err.(breakoutError); !ok {
+			// If untar returns an error unrelated to an archive breakout,
+			// then consider this an unexpected error and abort.
+			return err
+		}
+		// Here, untar detected the breakout.
+		// Let's move on verifying that indeed there was no breakout.
+		fmt.Printf("breakoutError: %v\n", err)
+	}
+
+	// Check victim folder
+	f, err := os.Open(victim)
+	if err != nil {
+		// codepath taken if victim folder was removed
+		return fmt.Errorf("archive breakout: error reading %q: %v", victim, err)
+	}
+	defer f.Close()
+
+	// Check contents of victim folder
+	//
+	// We are only interested in getting 2 files from the victim folder, because if all is well
+	// we expect only one result, the `hello` file. If there is a second result, it cannot
+	// hold the same name `hello` and we assume that a new file got created in the victim folder.
+	// That is enough to detect an archive breakout.
+	names, err := f.Readdirnames(2)
+	if err != nil {
+		// codepath taken if victim is not a folder
+		return fmt.Errorf("archive breakout: error reading directory content of %q: %v", victim, err)
+	}
+	for _, name := range names {
+		if name != "hello" {
+			// codepath taken if new file was created in victim folder
+			return fmt.Errorf("archive breakout: new file %q", name)
+		}
+	}
+
+	// Check victim/hello
+	f, err = os.Open(hello)
+	if err != nil {
+		// codepath taken if read permissions were removed
+		return fmt.Errorf("archive breakout: could not lstat %q: %v", hello, err)
+	}
+	defer f.Close()
+	b, err := ioutil.ReadAll(f)
+	if err != nil {
+		return err
+	}
+	fi, err := f.Stat()
+	if err != nil {
+		return err
+	}
+	if helloStat.IsDir() != fi.IsDir() ||
+		// TODO: cannot check for fi.ModTime() change
+		helloStat.Mode() != fi.Mode() ||
+		helloStat.Size() != fi.Size() ||
+		!bytes.Equal(helloData, b) {
+		// codepath taken if hello has been modified
+		return fmt.Errorf("archive breakout: file %q has been modified. Contents: expected=%q, got=%q. FileInfo: expected=%#v, got=%#v.", hello, helloData, b, helloStat, fi)
+	}
+
+	// Check that nothing in dest/ has the same content as victim/hello.
+	// Since victim/hello was generated with time.Now(), it is safe to assume
+	// that any file whose content matches exactly victim/hello, managed somehow
+	// to access victim/hello.
+	return filepath.Walk(dest, func(path string, info os.FileInfo, err error) error {
+		if info.IsDir() {
+			if err != nil {
+				// skip directory if error
+				return filepath.SkipDir
+			}
+			// enter directory
+			return nil
+		}
+		if err != nil {
+			// skip file if error
+			return nil
+		}
+		b, err := ioutil.ReadFile(path)
+		if err != nil {
+			// Houston, we have a problem. Aborting (space)walk.
+			return err
+		}
+		if bytes.Equal(helloData, b) {
+			return fmt.Errorf("archive breakout: file %q has been accessed via %q", hello, path)
+		}
+		return nil
+	})
+}