lib/model, lib/osutil: Verify target directory before pulling / requesting

GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3798

lib/model/model.go

@@ -120,6 +120,7 @@ var (
 	errDevicePaused        = errors.New("device is paused")
 	errDeviceIgnored       = errors.New("device is ignored")
 	errNotRelative         = errors.New("not a relative path")
+	errNotDir              = errors.New("parent is not a directory")
 )
 
 // NewModel creates and starts a new model. The model starts in read-only mode,
@@ -577,7 +578,7 @@ func (m *Model) NeedSize(folder string) db.Counts {
 		ignores := m.folderIgnores[folder]
 		cfg := m.folderCfgs[folder]
 		rf.WithNeedTruncated(protocol.LocalDeviceID, func(f db.FileIntf) bool {
-			if shouldIgnore(f, ignores, cfg.IgnoreDelete) {
+			if shouldIgnore(f, ignores, cfg.IgnoreDelete, defTempNamer) {
 				return true
 			}
 
@@ -641,7 +642,7 @@ func (m *Model) NeedFolderFiles(folder string, page, perpage int) ([]db.FileInfo
 	ignores := m.folderIgnores[folder]
 	cfg := m.folderCfgs[folder]
 	rf.WithNeedTruncated(protocol.LocalDeviceID, func(f db.FileIntf) bool {
-		if shouldIgnore(f, ignores, cfg.IgnoreDelete) {
+		if shouldIgnore(f, ignores, cfg.IgnoreDelete, defTempNamer) {
 			return true
 		}
 
@@ -1113,26 +1114,22 @@ func (m *Model) Request(deviceID protocol.DeviceID, folder, name string, offset
 		return protocol.ErrNoSuchFile
 	}
 
-	if info, err := osutil.Lstat(fn); err == nil && info.Mode()&os.ModeSymlink != 0 {
-		target, _, err := symlinks.Read(fn)
-		if err != nil {
-			l.Debugln("symlinks.Read:", err)
-			if os.IsNotExist(err) {
-				return protocol.ErrNoSuchFile
-			}
-			return protocol.ErrGeneric
-		}
-		if _, err := strings.NewReader(target).ReadAt(buf, offset); err != nil {
-			l.Debugln("symlink.Reader.ReadAt", err)
-			return protocol.ErrGeneric
-		}
-		return nil
+	if !osutil.IsDir(folderPath, filepath.Dir(name)) {
+		l.Debugf("%v REQ(in) for file not in dir: %s: %q / %q o=%d s=%d", m, deviceID, folder, name, offset, len(buf))
+		return protocol.ErrNoSuchFile
 	}
 
 	// Only check temp files if the flag is set, and if we are set to advertise
 	// the temp indexes.
 	if fromTemporary && !folderCfg.DisableTempIndexes {
 		tempFn := filepath.Join(folderPath, defTempNamer.TempName(name))
+
+		if info, err := osutil.Lstat(tempFn); err != nil || !info.Mode().IsRegular() {
+			// Reject reads for anything that doesn't exist or is something
+			// other than a regular file.
+			return protocol.ErrNoSuchFile
+		}
+
 		if err := readOffsetIntoBuf(tempFn, offset, buf); err == nil {
 			return nil
 		}
@@ -1140,6 +1137,12 @@ func (m *Model) Request(deviceID protocol.DeviceID, folder, name string, offset
 		// file has finished downloading.
 	}
 
+	if info, err := osutil.Lstat(fn); err != nil || !info.Mode().IsRegular() {
+		// Reject reads for anything that doesn't exist or is something
+		// other than a regular file.
+		return protocol.ErrNoSuchFile
+	}
+
 	err = readOffsetIntoBuf(fn, offset, buf)
 	if os.IsNotExist(err) {
 		return protocol.ErrNoSuchFile
@@ -2537,7 +2540,7 @@ func makeForgetUpdate(files []protocol.FileInfo) []protocol.FileDownloadProgress
 }
 
 // shouldIgnore returns true when a file should be excluded from processing
-func shouldIgnore(file db.FileIntf, matcher *ignore.Matcher, ignoreDelete bool) bool {
+func shouldIgnore(file db.FileIntf, matcher *ignore.Matcher, ignoreDelete bool, tmpNamer tempNamer) bool {
 	switch {
 	case ignoreDelete && file.IsDeleted():
 		// ignoreDelete first because it's a very cheap test so a win if it
@@ -2545,6 +2548,9 @@ func shouldIgnore(file db.FileIntf, matcher *ignore.Matcher, ignoreDelete bool)
 		// deleted files.
 		return true
 
+	case tmpNamer.IsTemporary(file.FileName()):
+		return true
+
 	case ignore.IsInternal(file.FileName()):
 		return true
 

lib/model/model_test.go

@@ -227,6 +227,7 @@ type fakeConnection struct {
 	folder                   string
 	model                    *Model
 	indexFn                  func(string, []protocol.FileInfo)
+	requestFn                func(folder, name string, offset int64, size int, hash []byte, fromTemporary bool) ([]byte, error)
 	mut                      sync.Mutex
 }
 
@@ -271,6 +272,11 @@ func (f *fakeConnection) IndexUpdate(folder string, fs []protocol.FileInfo) erro
 }
 
 func (f *fakeConnection) Request(folder, name string, offset int64, size int, hash []byte, fromTemporary bool) ([]byte, error) {
+	f.mut.Lock()
+	defer f.mut.Unlock()
+	if f.requestFn != nil {
+		return f.requestFn(folder, name, offset, size, hash, fromTemporary)
+	}
 	return f.fileData[name], nil
 }
 
@@ -307,24 +313,35 @@ func (f *fakeConnection) DownloadProgress(folder string, updates []protocol.File
 	})
 }
 
-func (f *fakeConnection) addFile(name string, flags uint32, data []byte) {
+func (f *fakeConnection) addFile(name string, flags uint32, ftype protocol.FileInfoType, data []byte) {
 	f.mut.Lock()
 	defer f.mut.Unlock()
 
 	blocks, _ := scanner.Blocks(bytes.NewReader(data), protocol.BlockSize, int64(len(data)), nil)
 	var version protocol.Vector
-	version.Update(f.id.Short())
+	version = version.Update(f.id.Short())
 
-	f.files = append(f.files, protocol.FileInfo{
-		Name:        name,
-		Type:        protocol.FileInfoTypeFile,
-		Size:        int64(len(data)),
-		ModifiedS:   time.Now().Unix(),
-		Permissions: flags,
-		Version:     version,
-		Sequence:    time.Now().UnixNano(),
-		Blocks:      blocks,
-	})
+	if ftype == protocol.FileInfoTypeFile || ftype == protocol.FileInfoTypeDirectory {
+		f.files = append(f.files, protocol.FileInfo{
+			Name:        name,
+			Type:        ftype,
+			Size:        int64(len(data)),
+			ModifiedS:   time.Now().Unix(),
+			Permissions: flags,
+			Version:     version,
+			Sequence:    time.Now().UnixNano(),
+			Blocks:      blocks,
+		})
+	} else {
+		// Symlink
+		f.files = append(f.files, protocol.FileInfo{
+			Name:          name,
+			Type:          ftype,
+			Version:       version,
+			Sequence:      time.Now().UnixNano(),
+			SymlinkTarget: string(data),
+		})
+	}
 
 	if f.fileData == nil {
 		f.fileData = make(map[string][]byte)
@@ -349,7 +366,7 @@ func BenchmarkRequestOut(b *testing.B) {
 
 	fc := &fakeConnection{id: device1}
 	for _, f := range files {
-		fc.addFile(f.Name, 0644, []byte("some data to return"))
+		fc.addFile(f.Name, 0644, protocol.FileInfoTypeFile, []byte("some data to return"))
 	}
 	m.AddConnection(fc, protocol.HelloResult{})
 	m.Index(device1, "default", files)

lib/model/requests_test.go

@@ -10,7 +10,10 @@ import (
 	"bytes"
 	"io/ioutil"
 	"os"
+	"runtime"
+	"strings"
 	"testing"
+	"time"
 
 	"github.com/syncthing/syncthing/lib/config"
 	"github.com/syncthing/syncthing/lib/db"
@@ -42,7 +45,7 @@ func TestRequestSimple(t *testing.T) {
 
 	// Send an update for the test file, wait for it to sync and be reported back.
 	contents := []byte("test file contents\n")
-	fc.addFile("testfile", 0644, contents)
+	fc.addFile("testfile", 0644, protocol.FileInfoTypeFile, contents)
 	fc.sendIndexUpdate()
 	<-done
 
@@ -57,6 +60,150 @@ func TestRequestSimple(t *testing.T) {
 	}
 }
 
+func TestSymlinkTraversalRead(t *testing.T) {
+	// Verify that a symlink can not be traversed for reading.
+
+	if runtime.GOOS == "windows" {
+		t.Skip("no symlink support on CI")
+		return
+	}
+
+	defer os.RemoveAll("_tmpfolder")
+
+	m, fc := setupModelWithConnection()
+	defer m.Stop()
+
+	// We listen for incoming index updates and trigger when we see one for
+	// the expected test file.
+	done := make(chan struct{})
+	fc.mut.Lock()
+	fc.indexFn = func(folder string, fs []protocol.FileInfo) {
+		for _, f := range fs {
+			if f.Name == "symlink" {
+				close(done)
+				return
+			}
+		}
+	}
+	fc.mut.Unlock()
+
+	// Send an update for the symlink, wait for it to sync and be reported back.
+	contents := []byte("..")
+	fc.addFile("symlink", 0644, protocol.FileInfoTypeSymlinkDirectory, contents)
+	fc.sendIndexUpdate()
+	<-done
+
+	// Request a file by traversing the symlink
+	buf := make([]byte, 10)
+	err := m.Request(device1, "default", "symlink/requests_test.go", 0, nil, false, buf)
+	if err == nil || !bytes.Equal(buf, make([]byte, 10)) {
+		t.Error("Managed to traverse symlink")
+	}
+}
+
+func TestSymlinkTraversalWrite(t *testing.T) {
+	// Verify that a symlink can not be traversed for writing.
+
+	if runtime.GOOS == "windows" {
+		t.Skip("no symlink support on CI")
+		return
+	}
+
+	defer os.RemoveAll("_tmpfolder")
+
+	m, fc := setupModelWithConnection()
+	defer m.Stop()
+
+	// We listen for incoming index updates and trigger when we see one for
+	// the expected names.
+	done := make(chan struct{}, 1)
+	badReq := make(chan string, 1)
+	badIdx := make(chan string, 1)
+	fc.mut.Lock()
+	fc.indexFn = func(folder string, fs []protocol.FileInfo) {
+		for _, f := range fs {
+			if f.Name == "symlink" {
+				done <- struct{}{}
+				return
+			}
+			if strings.HasPrefix(f.Name, "symlink") {
+				badIdx <- f.Name
+				return
+			}
+		}
+	}
+	fc.requestFn = func(folder, name string, offset int64, size int, hash []byte, fromTemporary bool) ([]byte, error) {
+		if name != "symlink" && strings.HasPrefix(name, "symlink") {
+			badReq <- name
+		}
+		return fc.fileData[name], nil
+	}
+	fc.mut.Unlock()
+
+	// Send an update for the symlink, wait for it to sync and be reported back.
+	contents := []byte("..")
+	fc.addFile("symlink", 0644, protocol.FileInfoTypeSymlinkDirectory, contents)
+	fc.sendIndexUpdate()
+	<-done
+
+	// Send an update for things behind the symlink, wait for requests for
+	// blocks for any of them to come back, or index entries. Hopefully none
+	// of that should happen.
+	contents = []byte("testdata testdata\n")
+	fc.addFile("symlink/testfile", 0644, protocol.FileInfoTypeFile, contents)
+	fc.addFile("symlink/testdir", 0644, protocol.FileInfoTypeDirectory, contents)
+	fc.addFile("symlink/testsyml", 0644, protocol.FileInfoTypeSymlinkFile, contents)
+	fc.sendIndexUpdate()
+
+	select {
+	case name := <-badReq:
+		t.Fatal("Should not have requested the data for", name)
+	case name := <-badIdx:
+		t.Fatal("Should not have sent the index entry for", name)
+	case <-time.After(3 * time.Second):
+		// Unfortunately not much else to trigger on here. The puller sleep
+		// interval is 1s so if we didn't get any requests within two
+		// iterations we should be fine.
+	}
+}
+
+func TestRequestCreateTmpSymlink(t *testing.T) {
+	// Verify that the model performs a request and creates a file based on
+	// an incoming index update.
+
+	defer os.RemoveAll("_tmpfolder")
+
+	m, fc := setupModelWithConnection()
+	defer m.Stop()
+
+	// We listen for incoming index updates and trigger when we see one for
+	// the expected test file.
+	badIdx := make(chan string)
+	fc.mut.Lock()
+	fc.indexFn = func(folder string, fs []protocol.FileInfo) {
+		for _, f := range fs {
+			if f.Name == ".syncthing.testlink.tmp" {
+				badIdx <- f.Name
+				return
+			}
+		}
+	}
+	fc.mut.Unlock()
+
+	// Send an update for the test file, wait for it to sync and be reported back.
+	fc.addFile(".syncthing.testlink.tmp", 0644, protocol.FileInfoTypeSymlinkDirectory, []byte(".."))
+	fc.sendIndexUpdate()
+
+	select {
+	case name := <-badIdx:
+		t.Fatal("Should not have sent the index entry for", name)
+	case <-time.After(3 * time.Second):
+		// Unfortunately not much else to trigger on here. The puller sleep
+		// interval is 1s so if we didn't get any requests within two
+		// iterations we should be fine.
+	}
+}
+
 func setupModelWithConnection() (*Model, *fakeConnection) {
 	cfg := defaultConfig.RawCopy()
 	cfg.Folders[0] = config.NewFolderConfiguration("default", "_tmpfolder")

lib/model/rwfolder.go

@@ -382,23 +382,81 @@ func (f *rwFolder) pullerIteration(ignores *ignore.Matcher) int {
 	folderFiles := f.model.folderFiles[f.folderID]
 	f.model.fmut.RUnlock()
 
-	// !!!
-	// WithNeed takes a database snapshot (by necessity). By the time we've
-	// handled a bunch of files it might have become out of date and we might
-	// be attempting to sync with an old version of a file...
-	// !!!
-
 	changed := 0
+	var processDirectly []protocol.FileInfo
+
+	// Iterate the list of items that we need and sort them into piles.
+	// Regular files to pull goes into the file queue, everything else
+	// (directories, symlinks and deletes) goes into the "process directly"
+	// pile.
+
+	folderFiles.WithNeed(protocol.LocalDeviceID, func(intf db.FileIntf) bool {
+		if shouldIgnore(intf, ignores, f.ignoreDelete, defTempNamer) {
+			return true
+		}
+
+		if err := fileValid(intf); err != nil {
+			// The file isn't valid so we can't process it. Pretend that we
+			// tried and set the error for the file.
+			f.newError(intf.FileName(), err)
+			changed++
+			return true
+		}
+
+		file := intf.(protocol.FileInfo)
+
+		switch {
+		case file.IsDeleted():
+			processDirectly = append(processDirectly, file)
+			changed++
+
+		case file.Type == protocol.FileInfoTypeFile:
+			// Queue files for processing after directories and symlinks, if
+			// it has availability.
+
+			devices := folderFiles.Availability(file.Name)
+			for _, dev := range devices {
+				if f.model.ConnectedTo(dev) {
+					f.queue.Push(file.Name, file.Size, file.ModTime())
+					changed++
+					break
+				}
+			}
+
+		default:
+			// Directories, symlinks
+			processDirectly = append(processDirectly, file)
+			changed++
+		}
+
+		return true
+	})
+
+	// Sort the "process directly" pile by number of path components. This
+	// ensures that we handle parents before children.
+
+	sort.Sort(byComponentCount(processDirectly))
+
+	// Process the list.
 
 	fileDeletions := map[string]protocol.FileInfo{}
 	dirDeletions := []protocol.FileInfo{}
 	buckets := map[string][]protocol.FileInfo{}
 
-	handleItem := func(fi protocol.FileInfo) bool {
+	for _, fi := range processDirectly {
+		// Verify that the thing we are handling lives inside a directory,
+		// and not a symlink or empty space.
+		if !osutil.IsDir(f.dir, filepath.Dir(fi.Name)) {
+			f.newError(fi.Name, errNotDir)
+			continue
+		}
+
 		switch {
 		case fi.IsDeleted():
 			// A deleted file, directory or symlink
 			if fi.IsDirectory() {
+				// Perform directory deletions at the end, as we may have
+				// files to delete inside them before we get to that point.
 				dirDeletions = append(dirDeletions, fi)
 			} else {
 				fileDeletions[fi.Name] = fi
@@ -413,63 +471,22 @@ func (f *rwFolder) pullerIteration(ignores *ignore.Matcher) int {
 					buckets[key] = append(buckets[key], df)
 				}
 			}
-			changed++
 
 		case fi.IsDirectory() && !fi.IsSymlink():
 			l.Debugln("Handling directory", fi.Name)
 			f.handleDir(fi)
-			changed++
 
 		case fi.IsSymlink():
 			l.Debugln("Handling symlink", fi.Name)
 			f.handleSymlink(fi)
-			changed++
 
 		default:
-			return false
+			l.Warnln(fi)
+			panic("unhandleable item type, can't happen")
 		}
-		return true
 	}
 
-	folderFiles.WithNeed(protocol.LocalDeviceID, func(intf db.FileIntf) bool {
-		// Needed items are delivered sorted lexicographically. We'll handle
-		// directories as they come along, so parents before children. Files
-		// are queued and the order may be changed later.
-
-		if shouldIgnore(intf, ignores, f.ignoreDelete) {
-			return true
-		}
-
-		if err := fileValid(intf); err != nil {
-			// The file isn't valid so we can't process it. Pretend that we
-			// tried and set the error for the file.
-			f.newError(intf.FileName(), err)
-			changed++
-			return true
-		}
-
-		file := intf.(protocol.FileInfo)
-		l.Debugln(f, "handling", file.Name)
-		if !handleItem(file) {
-			// A new or changed file or symlink. This is the only case where
-			// we do stuff concurrently in the background. We only queue
-			// files where we are connected to at least one device that has
-			// the file.
-
-			devices := folderFiles.Availability(file.Name)
-			for _, dev := range devices {
-				if f.model.ConnectedTo(dev) {
-					f.queue.Push(file.Name, file.Size, file.ModTime())
-					changed++
-					break
-				}
-			}
-		}
-
-		return true
-	})
-
-	// Reorder the file queue according to configuration
+	// Now do the file queue. Reorder it according to configuration.
 
 	switch f.order {
 	case config.OrderRandom:
@@ -486,7 +503,7 @@ func (f *rwFolder) pullerIteration(ignores *ignore.Matcher) int {
 		f.queue.SortNewestFirst()
 	}
 
-	// Process the file queue
+	// Process the file queue.
 
 nextFile:
 	for {
@@ -509,10 +526,17 @@ nextFile:
 			continue
 		}
 
-		// Handles races where an index update arrives changing what the file
-		// is between queueing and retrieving it from the queue, effectively
-		// changing how the file should be handled.
-		if handleItem(fi) {
+		if fi.IsDeleted() || fi.Type != protocol.FileInfoTypeFile {
+			// The item has changed type or status in the index while we
+			// were processing directories above.
+			f.queue.Done(fileName)
+			continue
+		}
+
+		// Verify that the thing we are handling lives inside a directory,
+		// and not a symlink or empty space.
+		if !osutil.IsDir(f.dir, filepath.Dir(fi.Name)) {
+			f.newError(fi.Name, errNotDir)
 			continue
 		}
 
@@ -779,6 +803,7 @@ func (f *rwFolder) deleteDir(file protocol.FileInfo, matcher *ignore.Matcher) {
 		f.newError(file.Name, err)
 		return
 	}
+
 	// Delete any temporary files lying around in the directory
 	dir, _ := os.Open(realName)
 	if dir != nil {
@@ -1727,3 +1752,29 @@ func windowsInvalidFilename(name string) bool {
 	// The path must not contain any disallowed characters
 	return strings.ContainsAny(name, windowsDisallowedCharacters)
 }
+
+// byComponentCount sorts by the number of path components in Name, that is
+// "x/y" sorts before "foo/bar/baz".
+type byComponentCount []protocol.FileInfo
+
+func (l byComponentCount) Len() int {
+	return len(l)
+}
+
+func (l byComponentCount) Less(a, b int) bool {
+	return componentCount(l[a].Name) < componentCount(l[b].Name)
+}
+
+func (l byComponentCount) Swap(a, b int) {
+	l[a], l[b] = l[b], l[a]
+}
+
+func componentCount(name string) int {
+	count := 0
+	for _, codepoint := range name {
+		if codepoint == os.PathSeparator {
+			count++
+		}
+	}
+	return count
+}

lib/osutil/isdir.go

@@ -0,0 +1,49 @@
+// Copyright (C) 2016 The Syncthing Authors.
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this file,
+// You can obtain one at http://mozilla.org/MPL/2.0/.
+
+package osutil
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+)
+
+// IsDir returns true if base and every path component of name up to and
+// including filepath.Join(base, name) is a directory (and not a symlink or
+// similar). Base and name must both be clean and name must be relative to
+// base.
+func IsDir(base, name string) bool {
+	path := base
+	info, err := Lstat(path)
+	if err != nil {
+		return false
+	}
+	if !info.IsDir() {
+		return false
+	}
+
+	if name == "." {
+		// The result of calling IsDir("some/where", filepath.Dir("foo"))
+		return true
+	}
+
+	parts := strings.Split(name, string(os.PathSeparator))
+	for _, part := range parts {
+		path = filepath.Join(path, part)
+		info, err := Lstat(path)
+		if err != nil {
+			return false
+		}
+		if info.Mode()&os.ModeSymlink != 0 {
+			return false
+		}
+		if !info.IsDir() {
+			return false
+		}
+	}
+	return true
+}

lib/osutil/isdir_test.go

@@ -0,0 +1,75 @@
+// Copyright (C) 2016 The Syncthing Authors.
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this file,
+// You can obtain one at http://mozilla.org/MPL/2.0/.
+
+package osutil_test
+
+import (
+	"os"
+	"testing"
+
+	"github.com/syncthing/syncthing/lib/osutil"
+	"github.com/syncthing/syncthing/lib/symlinks"
+)
+
+func TestIsDir(t *testing.T) {
+	if !symlinks.Supported {
+		t.Skip("pointless test")
+		return
+	}
+
+	os.RemoveAll("testdata")
+	defer os.RemoveAll("testdata")
+	os.MkdirAll("testdata/a/b/c", 0755)
+	symlinks.Create("testdata/a/l", "b", symlinks.TargetDirectory)
+
+	// a/l -> b, so a/l/c should resolve by normal stat
+	info, err := osutil.Lstat("testdata/a/l/c")
+	if err != nil {
+		t.Fatal("unexpected error", err)
+	}
+	if !info.IsDir() {
+		t.Fatal("error in setup, a/l/c should be a directory")
+	}
+
+	cases := []struct {
+		name  string
+		isDir bool
+	}{
+		// Exist
+		{".", true},
+		{"a", true},
+		{"a/b", true},
+		{"a/b/c", true},
+		// Don't exist
+		{"x", false},
+		{"a/x", false},
+		{"a/b/x", false},
+		{"a/x/c", false},
+		// Symlink or behind symlink
+		{"a/l", false},
+		{"a/l/c", false},
+	}
+
+	for _, tc := range cases {
+		if res := osutil.IsDir("testdata", tc.name); res != tc.isDir {
+			t.Errorf("IsDir(%q) = %v, should be %v", tc.name, res, tc.isDir)
+		}
+	}
+}
+
+var isDirResult bool
+
+func BenchmarkIsDir(b *testing.B) {
+	os.RemoveAll("testdata")
+	defer os.RemoveAll("testdata")
+	os.MkdirAll("testdata/a/b/c", 0755)
+
+	for i := 0; i < b.N; i++ {
+		isDirResult = osutil.IsDir("testdata", "a/b/c")
+	}
+
+	b.ReportAllocs()
+}