diff --git a/lib/model/model.go b/lib/model/model.go index 0f5a7bdb5..6065be426 100644 --- a/lib/model/model.go +++ b/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 +} diff --git a/lib/model/model_test.go b/lib/model/model_test.go index c5b235fb6..a17f17d85 100644 --- a/lib/model/model_test.go +++ b/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{}) diff --git a/lib/model/rwfolder.go b/lib/model/rwfolder.go index a90682cfc..3f40eee88 100644 --- a/lib/model/rwfolder.go +++ b/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 }