diff --git a/.changelog/23319.txt b/.changelog/23319.txt new file mode 100644 index 00000000000..d18b0d7399c --- /dev/null +++ b/.changelog/23319.txt @@ -0,0 +1,3 @@ +```release-note:security +migration: Added a check for relative paths escaping the allocation directory when unpacking archive during migration, to harden clients against compromised peer clients sending malicious archives +``` diff --git a/client/allocwatcher/alloc_watcher.go b/client/allocwatcher/alloc_watcher.go index ee0f096aee4..5f5ac776ace 100644 --- a/client/allocwatcher/alloc_watcher.go +++ b/client/allocwatcher/alloc_watcher.go @@ -587,6 +587,12 @@ func (p *remotePrevAlloc) streamAllocDir(ctx context.Context, resp io.ReadCloser p.prevAllocID, p.allocID, err) } + if escapes, err := escapingfs.PathEscapesAllocDir(dest, "", hdr.Name); err != nil { + return fmt.Errorf("error evaluating object: %w", err) + } else if escapes { + return fmt.Errorf("archive contains object that escapes alloc dir") + } + if hdr.Name == errorFilename { // Error snapshotting on the remote side, try to read // the message out of the file and return it. @@ -618,12 +624,12 @@ func (p *remotePrevAlloc) streamAllocDir(ctx context.Context, resp io.ReadCloser return fmt.Errorf("error creating symlink: %w", err) } - escapes, err := escapingfs.PathEscapesAllocDir(dest, "", hdr.Name) - if err != nil { - return fmt.Errorf("error evaluating symlink: %w", err) - } - if escapes { - return fmt.Errorf("archive contains symlink that escapes alloc dir") + for _, path := range []string{hdr.Name, hdr.Linkname} { + if escapes, err := escapingfs.PathEscapesAllocDir(dest, "", path); err != nil { + return fmt.Errorf("error evaluating symlink: %w", err) + } else if escapes { + return fmt.Errorf("archive contains symlink that escapes alloc dir") + } } continue diff --git a/client/allocwatcher/alloc_watcher_test.go b/client/allocwatcher/alloc_watcher_test.go index d22dc8fef2a..c79a4ca2e1b 100644 --- a/client/allocwatcher/alloc_watcher_test.go +++ b/client/allocwatcher/alloc_watcher_test.go @@ -23,6 +23,7 @@ import ( "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/testutil" + "github.com/shoenig/test/must" "github.com/stretchr/testify/require" ) @@ -276,3 +277,38 @@ func TestPrevAlloc_StreamAllocDir_Error(t *testing.T) { t.Fatalf("expected foo.txt to be size 1 but found %d", fi.Size()) } } + +// TestPrevAlloc_StreamAllocDir_FileEscape asserts that an error is returned +// when the tar archive contains a file that escapes the allocation directory. +func TestPrevAlloc_StreamAllocDir_FileEscape(t *testing.T) { + ci.Parallel(t) + + // This test only unit tests streamAllocDir so we only need a partially + // complete remotePrevAlloc + prevAlloc := &remotePrevAlloc{ + logger: testlog.HCLogger(t), + allocID: "123", + prevAllocID: "abc", + migrate: true, + } + + // Create a tar archive with a file that escapes the allocation directory + tarBuf := bytes.NewBuffer(nil) + tw := tar.NewWriter(tarBuf) + err := tw.WriteHeader(&tar.Header{ + Name: "../escape.txt", + Mode: 0666, + Size: 1, + ModTime: time.Now(), + Typeflag: tar.TypeReg, + }) + must.NoError(t, err) + _, err = tw.Write([]byte{'a'}) + t.Cleanup(func() { tw.Close() }) + must.NoError(t, err) + + // Attempt to stream the allocation directory + dest := t.TempDir() + err = prevAlloc.streamAllocDir(context.Background(), io.NopCloser(tarBuf), dest) + must.EqError(t, err, "archive contains object that escapes alloc dir") +} diff --git a/client/allocwatcher/alloc_watcher_unix_test.go b/client/allocwatcher/alloc_watcher_unix_test.go index 6416175bfc9..78f93e70835 100644 --- a/client/allocwatcher/alloc_watcher_unix_test.go +++ b/client/allocwatcher/alloc_watcher_unix_test.go @@ -115,6 +115,27 @@ func TestPrevAlloc_StreamAllocDir_BadSymlink(t *testing.T) { must.EqError(t, err, "archive contains symlink that escapes alloc dir") } +func TestPrevAlloc_StreamAllocDir_BadSymlink_Linkname(t *testing.T) { + ci.Parallel(t) + + // Create a tar archive with a symlink that attempts to escape the allocation directory + var buf bytes.Buffer + tw := tar.NewWriter(&buf) + t.Cleanup(func() { tw.Close() }) + must.NoError(t, tw.WriteHeader(&tar.Header{ + Typeflag: tar.TypeSymlink, + Name: "symlink", + Linkname: "../escape_attempt", + Mode: 0600, + })) + + newDir := t.TempDir() + prevAlloc := &remotePrevAlloc{logger: testlog.HCLogger(t)} + err := prevAlloc.streamAllocDir(context.Background(), io.NopCloser(&buf), newDir) + + must.EqError(t, err, "archive contains symlink that escapes alloc dir") +} + func testTar(dir string) (*bytes.Buffer, error) { buf := new(bytes.Buffer)