From 8a171731ba11e1a7e04e25542f9ac8ec70d9bb24 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 25 Nov 2017 20:36:32 +0100 Subject: [PATCH 1/3] restorer: Add tests for invalid node names --- internal/restic/restorer_test.go | 312 +++++++++++++++++++++++++++++++ 1 file changed, 312 insertions(+) create mode 100644 internal/restic/restorer_test.go diff --git a/internal/restic/restorer_test.go b/internal/restic/restorer_test.go new file mode 100644 index 000000000..2f9d18998 --- /dev/null +++ b/internal/restic/restorer_test.go @@ -0,0 +1,312 @@ +package restic_test + +import ( + "bytes" + "context" + "io/ioutil" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/restic/restic/internal/fs" + "github.com/restic/restic/internal/repository" + "github.com/restic/restic/internal/restic" + rtest "github.com/restic/restic/internal/test" +) + +type Node interface{} + +type Snapshot struct { + Nodes map[string]Node + treeID restic.ID +} + +type File struct { + Data string +} + +type Dir struct { + Nodes map[string]Node +} + +func saveFile(t testing.TB, repo restic.Repository, node File) restic.ID { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + id, err := repo.SaveBlob(ctx, restic.DataBlob, []byte(node.Data), restic.ID{}) + if err != nil { + t.Fatal(err) + } + + return id +} + +func saveDir(t testing.TB, repo restic.Repository, nodes map[string]Node) restic.ID { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + tree := &restic.Tree{} + for name, n := range nodes { + var id restic.ID + switch node := n.(type) { + case File: + id = saveFile(t, repo, node) + tree.Insert(&restic.Node{ + Type: "file", + Mode: 0644, + Name: name, + UID: uint32(os.Getuid()), + GID: uint32(os.Getgid()), + Content: []restic.ID{id}, + }) + case Dir: + id = saveDir(t, repo, node.Nodes) + tree.Insert(&restic.Node{ + Type: "dir", + Mode: 0755, + Name: name, + UID: uint32(os.Getuid()), + GID: uint32(os.Getgid()), + Subtree: &id, + }) + default: + t.Fatalf("unknown node type %T", node) + } + } + + id, err := repo.SaveTree(ctx, tree) + if err != nil { + t.Fatal(err) + } + + return id +} + +func saveSnapshot(t testing.TB, repo restic.Repository, snapshot Snapshot) (restic.Repository, restic.ID) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + treeID := saveDir(t, repo, snapshot.Nodes) + + err := repo.Flush() + if err != nil { + t.Fatal(err) + } + + err = repo.SaveIndex(ctx) + if err != nil { + t.Fatal(err) + } + + sn, err := restic.NewSnapshot([]string{"test"}, nil, "", time.Now()) + if err != nil { + t.Fatal(err) + } + + sn.Tree = &treeID + id, err := repo.SaveJSONUnpacked(ctx, restic.SnapshotFile, sn) + if err != nil { + t.Fatal(err) + } + + return repo, id +} + +// toSlash converts the OS specific path dir to a slash-separated path. +func toSlash(dir string) string { + data := strings.Split(dir, string(filepath.Separator)) + return strings.Join(data, "/") +} + +func TestRestorer(t *testing.T) { + var tests = []struct { + Snapshot + Files map[string]string + ErrorsMust map[string]string + ErrorsMay map[string]string + }{ + // valid test cases + { + Snapshot: Snapshot{ + Nodes: map[string]Node{ + "foo": File{"content: foo\n"}, + "dirtest": Dir{ + Nodes: map[string]Node{ + "file": File{"content: file\n"}, + }, + }, + }, + }, + Files: map[string]string{ + "foo": "content: foo\n", + "dirtest/file": "content: file\n", + }, + }, + { + Snapshot: Snapshot{ + Nodes: map[string]Node{ + "top": File{"toplevel file"}, + "dir": Dir{ + Nodes: map[string]Node{ + "file": File{"file in dir"}, + "subdir": Dir{ + Nodes: map[string]Node{ + "file": File{"file in subdir"}, + }, + }, + }, + }, + }, + }, + Files: map[string]string{ + "top": "toplevel file", + "dir/file": "file in dir", + "dir/subdir/file": "file in subdir", + }, + }, + + // test cases with invalid/constructed names + { + Snapshot: Snapshot{ + Nodes: map[string]Node{ + `..\test`: File{"foo\n"}, + `..\..\foo\..\bar\..\xx\test2`: File{"test2\n"}, + }, + }, + ErrorsMay: map[string]string{ + `/#..\test`: "node has invalid name", + `/#..\..\foo\..\bar\..\xx\test2`: "node has invalid name", + }, + }, + { + Snapshot: Snapshot{ + Nodes: map[string]Node{ + `../test`: File{"foo\n"}, + `../../foo/../bar/../xx/test2`: File{"test2\n"}, + }, + }, + ErrorsMay: map[string]string{ + `/#../test`: "node has invalid name", + `/#../../foo/../bar/../xx/test2`: "node has invalid name", + }, + }, + { + Snapshot: Snapshot{ + Nodes: map[string]Node{ + "top": File{"toplevel file"}, + "x": Dir{ + Nodes: map[string]Node{ + "file1": File{"file1"}, + "..": Dir{ + Nodes: map[string]Node{ + "file2": File{"file2"}, + "..": Dir{ + Nodes: map[string]Node{ + "file2": File{"file2"}, + }, + }, + }, + }, + }, + }, + }, + }, + Files: map[string]string{ + "top": "toplevel file", + }, + ErrorsMust: map[string]string{ + `/x#..`: "node has invalid name", + }, + }, + } + + for _, test := range tests { + t.Run("", func(t *testing.T) { + repo, cleanup := repository.TestRepository(t) + defer cleanup() + _, id := saveSnapshot(t, repo, test.Snapshot) + t.Logf("snapshot saved as %v", id.Str()) + + res, err := restic.NewRestorer(repo, id) + if err != nil { + t.Fatal(err) + } + + tempdir, cleanup := rtest.TempDir(t) + defer cleanup() + + res.SelectFilter = func(item, dstpath string, node *restic.Node) (selectedForRestore bool, childMayBeSelected bool) { + t.Logf("restore %v to %v", item, dstpath) + if !fs.HasPathPrefix(tempdir, dstpath) { + t.Errorf("would restore %v to %v, which is not within the target dir %v", + item, dstpath, tempdir) + return false, false + } + return true, true + } + + errors := make(map[string]string) + res.Error = func(dir string, node *restic.Node, err error) error { + t.Logf("restore returned error for %q in dir %v: %v", node.Name, dir, err) + dir = toSlash(dir) + errors[dir+"#"+node.Name] = err.Error() + return nil + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + err = res.RestoreTo(ctx, tempdir) + if err != nil { + t.Fatal(err) + } + + for filename, errorMessage := range test.ErrorsMust { + msg, ok := errors[filename] + if !ok { + t.Errorf("expected error for %v, found none", filename) + continue + } + + if msg != "" && msg != errorMessage { + t.Errorf("wrong error message for %v: got %q, want %q", + filename, msg, errorMessage) + } + + delete(errors, filename) + } + + for filename, errorMessage := range test.ErrorsMay { + msg, ok := errors[filename] + if !ok { + continue + } + + if msg != "" && msg != errorMessage { + t.Errorf("wrong error message for %v: got %q, want %q", + filename, msg, errorMessage) + } + + delete(errors, filename) + } + + for filename, err := range errors { + t.Errorf("unexpected error for %v found: %v", filename, err) + } + + for filename, content := range test.Files { + data, err := ioutil.ReadFile(filepath.Join(tempdir, filepath.FromSlash(filename))) + if err != nil { + t.Errorf("unable to read file %v: %v", filename, err) + continue + } + + if !bytes.Equal(data, []byte(content)) { + t.Errorf("file %v has wrong content: want %q, got %q", filename, content, data) + } + } + }) + } +} From 27d29b9853a56df78200b7b9d7010952a0dca6e6 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sun, 26 Nov 2017 15:17:12 +0100 Subject: [PATCH 2/3] restorer: Make sure node names are clean --- internal/restic/restorer.go | 62 +++++++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 19 deletions(-) diff --git a/internal/restic/restorer.go b/internal/restic/restorer.go index e3c4f4e36..41c8c6fe5 100644 --- a/internal/restic/restorer.go +++ b/internal/restic/restorer.go @@ -39,19 +39,47 @@ func NewRestorer(repo Repository, id ID) (*Restorer, error) { return r, nil } -func (res *Restorer) restoreTo(ctx context.Context, dst string, dir string, treeID ID, idx *HardlinkIndex) error { +// restoreTo restores a tree from the repo to a destination. target is the path in +// the file system, location within the snapshot. +func (res *Restorer) restoreTo(ctx context.Context, target, location string, treeID ID, idx *HardlinkIndex) error { + debug.Log("%v %v %v", target, location, treeID.Str()) tree, err := res.repo.LoadTree(ctx, treeID) if err != nil { - return res.Error(dir, nil, err) + debug.Log("error loading tree %v: %v", treeID.Str(), err) + return res.Error(location, nil, err) } for _, node := range tree.Nodes { - selectedForRestore, childMayBeSelected := res.SelectFilter(filepath.Join(dir, node.Name), - filepath.Join(dst, dir, node.Name), node) + + // ensure that the node name does not contain anything that refers to a + // top-level directory. + nodeName := filepath.Base(filepath.Join(string(filepath.Separator), node.Name)) + if nodeName != node.Name { + debug.Log("node %q has invalid name %q", node.Name, nodeName) + err := res.Error(location, node, errors.New("node has invalid name")) + if err != nil { + return err + } + continue + } + + nodeTarget := filepath.Join(target, nodeName) + nodeLocation := filepath.Join(location, nodeName) + + if target == nodeTarget || !fs.HasPathPrefix(target, nodeTarget) { + debug.Log("node %q has invalid target path %q", node.Name, nodeTarget) + err := res.Error(nodeLocation, node, errors.New("node has invalid path")) + if err != nil { + return err + } + continue + } + + selectedForRestore, childMayBeSelected := res.SelectFilter(nodeLocation, nodeTarget, node) debug.Log("SelectFilter returned %v %v", selectedForRestore, childMayBeSelected) if selectedForRestore { - err = res.restoreNodeTo(ctx, node, dir, dst, idx) + err = res.restoreNodeTo(ctx, node, nodeTarget, nodeLocation, idx) if err != nil { return err } @@ -62,10 +90,9 @@ func (res *Restorer) restoreTo(ctx context.Context, dst string, dir string, tree return errors.Errorf("Dir without subtree in tree %v", treeID.Str()) } - subp := filepath.Join(dir, node.Name) - err = res.restoreTo(ctx, dst, subp, *node.Subtree, idx) + err = res.restoreTo(ctx, nodeTarget, nodeLocation, *node.Subtree, idx) if err != nil { - err = res.Error(subp, node, err) + err = res.Error(nodeLocation, node, err) if err != nil { return err } @@ -74,7 +101,7 @@ func (res *Restorer) restoreTo(ctx context.Context, dst string, dir string, tree if selectedForRestore { // Restore directory timestamp at the end. If we would do it earlier, restoring files within // the directory would overwrite the timestamp of the directory they are in. - err = node.RestoreTimestamps(filepath.Join(dst, dir, node.Name)) + err = node.RestoreTimestamps(nodeTarget) if err != nil { return err } @@ -85,13 +112,12 @@ func (res *Restorer) restoreTo(ctx context.Context, dst string, dir string, tree return nil } -func (res *Restorer) restoreNodeTo(ctx context.Context, node *Node, dir string, dst string, idx *HardlinkIndex) error { - debug.Log("node %v, dir %v, dst %v", node.Name, dir, dst) - dstPath := filepath.Join(dst, dir, node.Name) +func (res *Restorer) restoreNodeTo(ctx context.Context, node *Node, target, location string, idx *HardlinkIndex) error { + debug.Log("%v %v %v", node.Name, target, location) - err := node.CreateAt(ctx, dstPath, res.repo, idx) + err := node.CreateAt(ctx, target, res.repo, idx) if err != nil { - debug.Log("node.CreateAt(%s) error %v", dstPath, err) + debug.Log("node.CreateAt(%s) error %v", target, err) } // Did it fail because of ENOENT? @@ -99,22 +125,20 @@ func (res *Restorer) restoreNodeTo(ctx context.Context, node *Node, dir string, debug.Log("create intermediate paths") // Create parent directories and retry - err = fs.MkdirAll(filepath.Dir(dstPath), 0700) + err = fs.MkdirAll(filepath.Dir(target), 0700) if err == nil || os.IsExist(errors.Cause(err)) { - err = node.CreateAt(ctx, dstPath, res.repo, idx) + err = node.CreateAt(ctx, target, res.repo, idx) } } if err != nil { debug.Log("error %v", err) - err = res.Error(dstPath, node, err) + err = res.Error(location, node, err) if err != nil { return err } } - debug.Log("successfully restored %v", node.Name) - return nil } From c8096ca8d25101e9c24d026a0237ebdf2773148c Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sun, 26 Nov 2017 15:28:18 +0100 Subject: [PATCH 3/3] Add entry to CHANGELOG --- CHANGELOG.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index be8c73f53..1fa230901 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,23 @@ released version of restic from the perspective of the user. Important Changes in 0.X.Y ========================== + * A vulnerability was found in the restic restorer, which allowed attackers in + special circumstances to restore files to a location outside of the target + directory. Due to the circumstances we estimate this to be a low-risk + vulnerability, but urge all users to upgrade to the latest version of restic. + + Exploiting the vulnerability requires a Linux/Unix system which saves + backups via restic and a Windows systems which restores files from the repo. + In addition, the attackers need to be able to create create files with + arbitrary names which are then saved to the restic repo. For example, by + creating a file named "..\test.txt" (which is a perfectly legal filename on + Linux) and restoring a snapshot containing this file on Windows, it would be + written to the parent of the target directory. + + We'd like to thank Tyler Spivey for reporting this responsibly! + + https://github.com/restic/restic/pull/1445 + * The s3 backend used the subdir `restic` within a bucket if no explicit path after the bucket name was specified. Since this version, restic does not use this default path any more. If you created a repo on s3 in a bucket without