From 54b50e3d5299c7f88e9b6a9295fae562890a3088 Mon Sep 17 00:00:00 2001 From: Simon Frei Date: Wed, 25 Nov 2020 22:52:46 +0100 Subject: [PATCH] lib/fs, lib/model: Cover more windowsyness sanitizing paths (fixes #7075) (#7158) --- lib/fs/util.go | 69 +++++++++++++++++++++++++++++++++++------ lib/fs/util_test.go | 47 ++++++++++++++++++++++++++++ lib/model/model.go | 41 ++---------------------- lib/model/model_test.go | 46 --------------------------- 4 files changed, 109 insertions(+), 94 deletions(-) diff --git a/lib/fs/util.go b/lib/fs/util.go index ad530d668..7a9f05d12 100644 --- a/lib/fs/util.go +++ b/lib/fs/util.go @@ -12,6 +12,7 @@ import ( "path/filepath" "runtime" "strings" + "unicode" ) func ExpandTilde(path string) (string, error) { @@ -72,15 +73,8 @@ func WindowsInvalidFilename(name string) error { // Names ending in space or period are not valid. return errInvalidFilenameWindowsSpacePeriod } - upperCased := strings.ToUpper(part) - for _, disallowed := range windowsDisallowedNames { - if upperCased == disallowed { - return errInvalidFilenameWindowsReservedName - } - if strings.HasPrefix(upperCased, disallowed+".") { - // nul.txt.jpg is also disallowed - return errInvalidFilenameWindowsReservedName - } + if windowsIsReserved(part) { + return errInvalidFilenameWindowsReservedName } } @@ -92,6 +86,63 @@ func WindowsInvalidFilename(name string) error { return nil } +// SanitizePath takes a string that might contain all kinds of special +// characters and makes a valid, similar, path name out of it. +// +// Spans of invalid characters, whitespace and/or non-UTF-8 sequences are +// replaced by a single space. The result is always UTF-8 and contains only +// printable characters, as determined by unicode.IsPrint. +// +// Invalid characters are non-printing runes, things not allowed in file names +// in Windows, and common shell metacharacters. Even if asterisks and pipes +// and stuff are allowed on Unixes in general they might not be allowed by +// the filesystem and may surprise the user and cause shell oddness. This +// function is intended for file names we generate on behalf of the user, +// and surprising them with odd shell characters in file names is unkind. +// +// We include whitespace in the invalid characters so that multiple +// whitespace is collapsed to a single space. Additionally, whitespace at +// either end is removed. +// +// If the result is a name disallowed on windows, a hyphen is prepended. +func SanitizePath(path string) string { + var b strings.Builder + + disallowed := `<>:"'/\|?*[]{};:!@$%&^#` + windowsDisallowedCharacters + prev := ' ' + for _, c := range path { + if !unicode.IsPrint(c) || c == unicode.ReplacementChar || + strings.ContainsRune(disallowed, c) { + c = ' ' + } + + if !(c == ' ' && prev == ' ') { + b.WriteRune(c) + } + prev = c + } + + path = strings.TrimSpace(b.String()) + if windowsIsReserved(path) { + path = "-" + path + } + return path +} + +func windowsIsReserved(part string) bool { + upperCased := strings.ToUpper(part) + for _, disallowed := range windowsDisallowedNames { + if upperCased == disallowed { + return true + } + if strings.HasPrefix(upperCased, disallowed+".") { + // nul.txt.jpg is also disallowed + return true + } + } + return false +} + // IsParent compares paths purely lexicographically, meaning it returns false // if path and parent aren't both absolute or relative. func IsParent(path, parent string) bool { diff --git a/lib/fs/util_test.go b/lib/fs/util_test.go index 93d62d0ee..0b934308a 100644 --- a/lib/fs/util_test.go +++ b/lib/fs/util_test.go @@ -7,8 +7,11 @@ package fs import ( + "math/rand" "runtime" "testing" + "unicode" + "unicode/utf8" ) func TestCommonPrefix(t *testing.T) { @@ -70,3 +73,47 @@ func TestWindowsInvalidFilename(t *testing.T) { } } } + +func TestSanitizePath(t *testing.T) { + cases := [][2]string{ + {"", ""}, + {"foo", "foo"}, + {`\*/foo\?/bar[{!@$%^&*#()}]`, "foo bar ()"}, + {"Räksmörgås", "Räksmörgås"}, + {`Räk \/ smörgås`, "Räk smörgås"}, + {"هذا هو *\x07?اسم الملف", "هذا هو اسم الملف"}, + {`../foo.txt`, `.. foo.txt`}, + {" \t \n filename in \t space\r", "filename in space"}, + {"你\xff好", `你 好`}, + {"\000 foo", "foo"}, + } + + for _, tc := range cases { + res := SanitizePath(tc[0]) + if res != tc[1] { + t.Errorf("SanitizePath(%q) => %q, expected %q", tc[0], res, tc[1]) + } + } +} + +// Fuzz test: SanitizePath must always return strings of printable UTF-8 +// characters when fed random data. +// +// Note that space is considered printable, but other whitespace runes are not. +func TestSanitizePathFuzz(t *testing.T) { + buf := make([]byte, 128) + + for i := 0; i < 100; i++ { + rand.Read(buf) + path := SanitizePath(string(buf)) + if !utf8.ValidString(path) { + t.Errorf("SanitizePath(%q) => %q, not valid UTF-8", buf, path) + continue + } + for _, c := range path { + if !unicode.IsPrint(c) { + t.Errorf("non-printable rune %q in sanitized path", c) + } + } + } +} diff --git a/lib/model/model.go b/lib/model/model.go index d88e07aff..56aabd4bf 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -19,7 +19,6 @@ import ( "strings" stdsync "sync" "time" - "unicode" "github.com/pkg/errors" "github.com/thejerf/suture/v4" @@ -1467,8 +1466,8 @@ func (m *model) handleAutoAccepts(deviceID protocol.DeviceID, folder protocol.Fo defaultPath := m.cfg.Options().DefaultFolderPath defaultPathFs := fs.NewFilesystem(fs.FilesystemTypeBasic, defaultPath) pathAlternatives := []string{ - sanitizePath(folder.Label), - sanitizePath(folder.ID), + fs.SanitizePath(folder.Label), + fs.SanitizePath(folder.ID), } for _, path := range pathAlternatives { if _, err := defaultPathFs.Lstat(path); !fs.IsNotExist(err) { @@ -2751,42 +2750,6 @@ func (m *syncMutexMap) Get(key string) sync.Mutex { return v.(sync.Mutex) } -// sanitizePath takes a string that might contain all kinds of special -// characters and makes a valid, similar, path name out of it. -// -// Spans of invalid characters, whitespace and/or non-UTF-8 sequences are -// replaced by a single space. The result is always UTF-8 and contains only -// printable characters, as determined by unicode.IsPrint. -// -// Invalid characters are non-printing runes, things not allowed in file names -// in Windows, and common shell metacharacters. Even if asterisks and pipes -// and stuff are allowed on Unixes in general they might not be allowed by -// the filesystem and may surprise the user and cause shell oddness. This -// function is intended for file names we generate on behalf of the user, -// and surprising them with odd shell characters in file names is unkind. -// -// We include whitespace in the invalid characters so that multiple -// whitespace is collapsed to a single space. Additionally, whitespace at -// either end is removed. -func sanitizePath(path string) string { - var b strings.Builder - - prev := ' ' - for _, c := range path { - if !unicode.IsPrint(c) || c == unicode.ReplacementChar || - strings.ContainsRune(`<>:"'/\|?*[]{};:!@$%&^#`, c) { - c = ' ' - } - - if !(c == ' ' && prev == ' ') { - b.WriteRune(c) - } - prev = c - } - - return strings.TrimSpace(b.String()) -} - type deviceIDSet map[protocol.DeviceID]struct{} func (s deviceIDSet) add(ids []protocol.DeviceID) { diff --git a/lib/model/model_test.go b/lib/model/model_test.go index 24ef57e3f..dcae3eae8 100644 --- a/lib/model/model_test.go +++ b/lib/model/model_test.go @@ -25,8 +25,6 @@ import ( "sync/atomic" "testing" "time" - "unicode" - "unicode/utf8" "github.com/syncthing/syncthing/lib/config" "github.com/syncthing/syncthing/lib/db" @@ -3283,50 +3281,6 @@ func TestRequestLimit(t *testing.T) { } } -func TestSanitizePath(t *testing.T) { - cases := [][2]string{ - {"", ""}, - {"foo", "foo"}, - {`\*/foo\?/bar[{!@$%^&*#()}]`, "foo bar ()"}, - {"Räksmörgås", "Räksmörgås"}, - {`Räk \/ smörgås`, "Räk smörgås"}, - {"هذا هو *\x07?اسم الملف", "هذا هو اسم الملف"}, - {`../foo.txt`, `.. foo.txt`}, - {" \t \n filename in \t space\r", "filename in space"}, - {"你\xff好", `你 好`}, - {"\000 foo", "foo"}, - } - - for _, tc := range cases { - res := sanitizePath(tc[0]) - if res != tc[1] { - t.Errorf("sanitizePath(%q) => %q, expected %q", tc[0], res, tc[1]) - } - } -} - -// Fuzz test: sanitizePath must always return strings of printable UTF-8 -// characters when fed random data. -// -// Note that space is considered printable, but other whitespace runes are not. -func TestSanitizePathFuzz(t *testing.T) { - buf := make([]byte, 128) - - for i := 0; i < 100; i++ { - rand.Read(buf) - path := sanitizePath(string(buf)) - if !utf8.ValidString(path) { - t.Errorf("sanitizePath(%q) => %q, not valid UTF-8", buf, path) - continue - } - for _, c := range path { - if !unicode.IsPrint(c) { - t.Errorf("non-printable rune %q in sanitized path", c) - } - } - } -} - // TestConnCloseOnRestart checks that there is no deadlock when calling Close // on a protocol connection that has a blocking reader (blocking writer can't // be done as the test requires clusterconfigs to go through).