Merge pull request #1445 from restic/clean-node-name

restorer: Clean node names

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

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, nodeTarget, nodeLocation, *node.Subtree, idx)
-			err = res.restoreTo(ctx, dst, subp, *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 {
+func (res *Restorer) restoreNodeTo(ctx context.Context, node *Node, target, location string, idx *HardlinkIndex) error {
-	debug.Log("node %v, dir %v, dst %v", node.Name, dir, dst)
+	debug.Log("%v %v %v", node.Name, target, location)
-	dstPath := filepath.Join(dst, dir, node.Name)
 
-	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
 }
 

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)
+				}
+			}
+		})
+	}
+}