lib/model: Double check results in filepath.Join where needed

Wherever we have untrusted relative paths, make sure they are not
escaping their folder root.

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

lib/model/model.go

@@ -119,6 +119,7 @@ var (
 	errDeviceUnknown       = errors.New("unknown device")
 	errDevicePaused        = errors.New("device is paused")
 	errDeviceIgnored       = errors.New("device is ignored")
+	errNotRelative         = errors.New("not a relative path")
 )
 
 // NewModel creates and starts a new model. The model starts in read-only mode,
@@ -1091,23 +1092,8 @@ func (m *Model) Request(deviceID protocol.DeviceID, folder, name string, offset
 	folderIgnores := m.folderIgnores[folder]
 	m.fmut.RUnlock()
 
-	// filepath.Join() returns a filepath.Clean()ed path, which (quoting the
-	// docs for clarity here):
-	//
-	//     Clean returns the shortest path name equivalent to path by purely lexical
-	//     processing. It applies the following rules iteratively until no further
-	//     processing can be done:
-	//
-	//     1. Replace multiple Separator elements with a single one.
-	//     2. Eliminate each . path name element (the current directory).
-	//     3. Eliminate each inner .. path name element (the parent directory)
-	//        along with the non-.. element that precedes it.
-	//     4. Eliminate .. elements that begin a rooted path:
-	//        that is, replace "/.." by "/" at the beginning of a path,
-	//        assuming Separator is '/'.
-	fn := filepath.Join(folderPath, name)
-
-	if !strings.HasPrefix(fn, folderPath) {
+	fn, err := rootedJoinedPath(folderPath, name)
+	if err != nil {
 		// Request tries to escape!
 		l.Debugf("%v Invalid REQ(in) tries to escape: %s: %q / %q o=%d s=%d", m, deviceID, folder, name, offset, len(buf))
 		return protocol.ErrInvalid
@@ -1152,7 +1138,7 @@ func (m *Model) Request(deviceID protocol.DeviceID, folder, name string, offset
 		// file has finished downloading.
 	}
 
-	err := readOffsetIntoBuf(fn, offset, buf)
+	err = readOffsetIntoBuf(fn, offset, buf)
 	if os.IsNotExist(err) {
 		return protocol.ErrNoSuchFile
 	} else if err != nil {
@@ -1700,7 +1686,9 @@ func (m *Model) ScanFolderSubdirs(folder string, subs []string) error {
 func (m *Model) internalScanFolderSubdirs(folder string, subDirs []string) error {
 	for i, sub := range subDirs {
 		sub = osutil.NativeFilename(sub)
-		if p := filepath.Clean(filepath.Join(folder, sub)); !strings.HasPrefix(p, folder) {
+		// We test each path by joining with "root". What we join with is
+		// not relevant, we just want the dotdot escape detection here.
+		if _, err := rootedJoinedPath("root", sub); err != nil {
 			return errors.New("invalid subpath")
 		}
 		subDirs[i] = sub
@@ -2550,8 +2538,6 @@ 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 {
-	// We check things in a certain order here...
-
 	switch {
 	case ignoreDelete && file.IsDeleted():
 		// ignoreDelete first because it's a very cheap test so a win if it
@@ -2560,8 +2546,6 @@ func shouldIgnore(file db.FileIntf, matcher *ignore.Matcher, ignoreDelete bool)
 		return true
 
 	case matcher.Match(file.FileName()).IsIgnored():
-		// ignore patterns second because ignoring them is a valid way to
-		// silence warnings about them being invalid and so on.
 		return true
 	}
 
@@ -2606,3 +2590,57 @@ func (s folderDeviceSet) sortedDevices(folder string) []protocol.DeviceID {
 	sort.Sort(protocol.DeviceIDs(devs))
 	return devs
 }
+
+// rootedJoinedPath takes a root and a supposedly relative path inside that
+// root and returns the joined path. An error is returned if the joined path
+// is not in fact inside the root.
+func rootedJoinedPath(root, rel string) (string, error) {
+	// The root must not be empty.
+	if root == "" {
+		return "", errInvalidFilename
+	}
+
+	pathSep := string(os.PathSeparator)
+
+	// The expected prefix for the resulting path is the root, with a path
+	// separator at the end.
+	expectedPrefix := filepath.FromSlash(root)
+	if !strings.HasSuffix(expectedPrefix, pathSep) {
+		expectedPrefix += pathSep
+	}
+
+	// The relative path should be clean from internal dotdots and similar
+	// funkyness.
+	rel = filepath.FromSlash(rel)
+	if filepath.Clean(rel) != rel {
+		return "", errInvalidFilename
+	}
+
+	// It is not acceptable to attempt to traverse upwards or refer to the
+	// root itself.
+	switch rel {
+	case ".", "..", pathSep:
+		return "", errNotRelative
+	}
+	if strings.HasPrefix(rel, ".."+pathSep) {
+		return "", errNotRelative
+	}
+
+	if strings.HasPrefix(rel, pathSep+pathSep) {
+		// The relative path may pretend to be an absolute path within the
+		// root, but the double path separator on Windows implies something
+		// else. It would get cleaned by the Join below, but it's out of
+		// spec anyway.
+		return "", errNotRelative
+	}
+
+	// The supposedly correct path is the one filepath.Join will return, as
+	// it does cleaning and so on. Check that one first to make sure no
+	// obvious escape attempts have been made.
+	joined := filepath.Join(root, rel)
+	if !strings.HasPrefix(joined, expectedPrefix) {
+		return "", errNotRelative
+	}
+
+	return joined, nil
+}

lib/model/model_test.go

@@ -2123,6 +2123,151 @@ func TestIssue3496(t *testing.T) {
 	}
 }
 
+func TestRootedJoinedPath(t *testing.T) {
+	type testcase struct {
+		root   string
+		rel    string
+		joined string
+		ok     bool
+	}
+	cases := []testcase{
+		// Valid cases
+		{"foo", "bar", "foo/bar", true},
+		{"foo", "/bar", "foo/bar", true},
+		{"foo/", "bar", "foo/bar", true},
+		{"foo/", "/bar", "foo/bar", true},
+		{"baz/foo", "bar", "baz/foo/bar", true},
+		{"baz/foo", "/bar", "baz/foo/bar", true},
+		{"baz/foo/", "bar", "baz/foo/bar", true},
+		{"baz/foo/", "/bar", "baz/foo/bar", true},
+		{"foo", "bar/baz", "foo/bar/baz", true},
+		{"foo", "/bar/baz", "foo/bar/baz", true},
+		{"foo/", "bar/baz", "foo/bar/baz", true},
+		{"foo/", "/bar/baz", "foo/bar/baz", true},
+		{"baz/foo", "bar/baz", "baz/foo/bar/baz", true},
+		{"baz/foo", "/bar/baz", "baz/foo/bar/baz", true},
+		{"baz/foo/", "bar/baz", "baz/foo/bar/baz", true},
+		{"baz/foo/", "/bar/baz", "baz/foo/bar/baz", true},
+
+		// Not escape attempts, but oddly formatted relative paths. Disallowed.
+		{"foo", "./bar", "", false},
+		{"baz/foo", "./bar", "", false},
+		{"foo", "./bar/baz", "", false},
+		{"baz/foo", "./bar/baz", "", false},
+		{"baz/foo", "bar/../baz", "", false},
+		{"baz/foo", "/bar/../baz", "", false},
+		{"baz/foo", "./bar/../baz", "", false},
+		{"baz/foo", "bar/../baz", "", false},
+		{"baz/foo", "/bar/../baz", "", false},
+		{"baz/foo", "./bar/../baz", "", false},
+
+		// Results in an allowed path, but does it by probing. Disallowed.
+		{"foo", "../foo", "", false},
+		{"foo", "../foo/bar", "", false},
+		{"baz/foo", "../foo/bar", "", false},
+		{"baz/foo", "../../baz/foo/bar", "", false},
+		{"baz/foo", "bar/../../foo/bar", "", false},
+		{"baz/foo", "bar/../../../baz/foo/bar", "", false},
+
+		// Escape attempts.
+		{"foo", "", "", false},
+		{"foo", "/", "", false},
+		{"foo", "..", "", false},
+		{"foo", "/..", "", false},
+		{"foo", "../", "", false},
+		{"foo", "../bar", "", false},
+		{"foo", "../foobar", "", false},
+		{"foo/", "../bar", "", false},
+		{"foo/", "../foobar", "", false},
+		{"baz/foo", "../bar", "", false},
+		{"baz/foo", "../foobar", "", false},
+		{"baz/foo/", "../bar", "", false},
+		{"baz/foo/", "../foobar", "", false},
+		{"baz/foo/", "bar/../../quux/baz", "", false},
+
+		// Empty root is a misconfiguration.
+		{"", "/foo", "", false},
+		{"", "foo", "", false},
+		{"", ".", "", false},
+		{"", "..", "", false},
+		{"", "/", "", false},
+		{"", "", "", false},
+
+		// Root=/ is valid, and things should be verified as usual.
+		{"/", "foo", "/foo", true},
+		{"/", "/foo", "/foo", true},
+		{"/", "../foo", "", false},
+		{"/", ".", "", false},
+		{"/", "..", "", false},
+		{"/", "/", "", false},
+		{"/", "", "", false},
+	}
+
+	if runtime.GOOS == "windows" {
+		extraCases := []testcase{
+			{`c:\`, `foo`, `c:\foo`, true},
+			{`\\?\c:\`, `foo`, `\\?\c:\foo`, true},
+			{`c:\`, `\foo`, `c:\foo`, true},
+			{`\\?\c:\`, `\foo`, `\\?\c:\foo`, true},
+
+			{`c:\`, `\\foo`, ``, false},
+			{`c:\`, ``, ``, false},
+			{`c:\`, `.`, ``, false},
+			{`c:\`, `\`, ``, false},
+			{`\\?\c:\`, `\\foo`, ``, false},
+			{`\\?\c:\`, ``, ``, false},
+			{`\\?\c:\`, `.`, ``, false},
+			{`\\?\c:\`, `\`, ``, false},
+
+			// makes no sense, but will be treated simply as a bad filename
+			{`c:\foo`, `d:\bar`, `c:\foo\d:\bar`, true},
+		}
+
+		for _, tc := range cases {
+			// Add case where root is backslashed, rel is forward slashed
+			extraCases = append(extraCases, testcase{
+				root:   filepath.FromSlash(tc.root),
+				rel:    tc.rel,
+				joined: tc.joined,
+				ok:     tc.ok,
+			})
+			// and the opposite
+			extraCases = append(extraCases, testcase{
+				root:   tc.root,
+				rel:    filepath.FromSlash(tc.rel),
+				joined: tc.joined,
+				ok:     tc.ok,
+			})
+			// and both backslashed
+			extraCases = append(extraCases, testcase{
+				root:   filepath.FromSlash(tc.root),
+				rel:    filepath.FromSlash(tc.rel),
+				joined: tc.joined,
+				ok:     tc.ok,
+			})
+		}
+
+		cases = append(cases, extraCases...)
+	}
+
+	for _, tc := range cases {
+		res, err := rootedJoinedPath(tc.root, tc.rel)
+		if tc.ok {
+			if err != nil {
+				t.Errorf("Unexpected error for rootedJoinedPath(%q, %q): %v", tc.root, tc.rel, err)
+				continue
+			}
+			exp := filepath.FromSlash(tc.joined)
+			if res != exp {
+				t.Errorf("Unexpected result for rootedJoinedPath(%q, %q): %q != expected %q", tc.root, tc.rel, res, exp)
+			}
+		} else if err == nil {
+			t.Errorf("Unexpected pass for rootedJoinedPath(%q, %q) => %q", tc.root, tc.rel, res)
+			continue
+		}
+	}
+}
+
 func addFakeConn(m *Model, dev protocol.DeviceID) *fakeConnection {
 	fc := &fakeConnection{id: dev, model: m}
 	m.AddConnection(fc, protocol.HelloResult{})

lib/model/rwfolder.go

@@ -587,7 +587,11 @@ func (f *rwFolder) handleDir(file protocol.FileInfo) {
 		})
 	}()
 
-	realName := filepath.Join(f.dir, file.Name)
+	realName, err := rootedJoinedPath(f.dir, file.Name)
+	if err != nil {
+		f.newError(file.Name, err)
+		return
+	}
 	mode := os.FileMode(file.Permissions & 0777)
 	if f.ignorePermissions(file) {
 		mode = 0777
@@ -681,7 +685,11 @@ func (f *rwFolder) deleteDir(file protocol.FileInfo, matcher *ignore.Matcher) {
 		})
 	}()
 
-	realName := filepath.Join(f.dir, file.Name)
+	realName, err := rootedJoinedPath(f.dir, file.Name)
+	if err != nil {
+		f.newError(file.Name, err)
+		return
+	}
 	// Delete any temporary files lying around in the directory
 	dir, _ := os.Open(realName)
 	if dir != nil {
@@ -730,7 +738,11 @@ func (f *rwFolder) deleteFile(file protocol.FileInfo) {
 		})
 	}()
 
-	realName := filepath.Join(f.dir, file.Name)
+	realName, err := rootedJoinedPath(f.dir, file.Name)
+	if err != nil {
+		f.newError(file.Name, err)
+		return
+	}
 
 	cur, ok := f.model.CurrentFolderFile(f.folderID, file.Name)
 	if ok && f.inConflict(cur.Version, file.Version) {
@@ -795,8 +807,16 @@ func (f *rwFolder) renameFile(source, target protocol.FileInfo) {
 
 	l.Debugln(f, "taking rename shortcut", source.Name, "->", target.Name)
 
-	from := filepath.Join(f.dir, source.Name)
-	to := filepath.Join(f.dir, target.Name)
+	from, err := rootedJoinedPath(f.dir, source.Name)
+	if err != nil {
+		f.newError(source.Name, err)
+		return
+	}
+	to, err := rootedJoinedPath(f.dir, target.Name)
+	if err != nil {
+		f.newError(target.Name, err)
+		return
+	}
 
 	if f.versioner != nil {
 		err = osutil.Copy(from, to)
@@ -918,8 +938,16 @@ func (f *rwFolder) handleFile(file protocol.FileInfo, copyChan chan<- copyBlocks
 	}
 
 	// Figure out the absolute filenames we need once and for all
-	tempName := filepath.Join(f.dir, defTempNamer.TempName(file.Name))
-	realName := filepath.Join(f.dir, file.Name)
+	tempName, err := rootedJoinedPath(f.dir, defTempNamer.TempName(file.Name))
+	if err != nil {
+		f.newError(file.Name, err)
+		return
+	}
+	realName, err := rootedJoinedPath(f.dir, file.Name)
+	if err != nil {
+		f.newError(file.Name, err)
+		return
+	}
 
 	if hasCurFile && !curFile.IsDirectory() && !curFile.IsSymlink() {
 		// Check that the file on disk is what we expect it to be according to
@@ -1037,7 +1065,11 @@ func (f *rwFolder) handleFile(file protocol.FileInfo, copyChan chan<- copyBlocks
 // shortcutFile sets file mode and modification time, when that's the only
 // thing that has changed.
 func (f *rwFolder) shortcutFile(file protocol.FileInfo) error {
-	realName := filepath.Join(f.dir, file.Name)
+	realName, err := rootedJoinedPath(f.dir, file.Name)
+	if err != nil {
+		f.newError(file.Name, err)
+		return err
+	}
 	if !f.ignorePermissions(file) {
 		if err := os.Chmod(realName, os.FileMode(file.Permissions&0777)); err != nil {
 			l.Infof("Puller (folder %q, file %q): shortcut: chmod: %v", f.folderID, file.Name, err)
@@ -1112,7 +1144,11 @@ func (f *rwFolder) copierRoutine(in <-chan copyBlocksState, pullChan chan<- pull
 
 			buf = buf[:int(block.Size)]
 			found := f.model.finder.Iterate(folders, block.Hash, func(folder, file string, index int32) bool {
-				fd, err := os.Open(filepath.Join(folderRoots[folder], file))
+				inFile, err := rootedJoinedPath(folderRoots[folder], file)
+				if err != nil {
+					return false
+				}
+				fd, err := os.Open(inFile)
 				if err != nil {
 					return false
 				}