From 3266aae1c3ffd298ea2aada170b14ede05260296 Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Thu, 1 Dec 2016 12:35:32 +0000 Subject: [PATCH] lib/protocol: Apply input filtering on file names GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3775 --- lib/protocol/nativemodel_windows.go | 43 ++++++++++++-- lib/protocol/nativemodel_windows_test.go | 29 ++++++++++ lib/protocol/protocol.go | 71 ++++++++++++++++-------- lib/protocol/protocol_test.go | 40 +++++++++++++ 4 files changed, 156 insertions(+), 27 deletions(-) create mode 100644 lib/protocol/nativemodel_windows_test.go diff --git a/lib/protocol/nativemodel_windows.go b/lib/protocol/nativemodel_windows.go index 0997386a..04932dd2 100644 --- a/lib/protocol/nativemodel_windows.go +++ b/lib/protocol/nativemodel_windows.go @@ -6,29 +6,64 @@ package protocol // Windows uses backslashes as file separator -import "path/filepath" +import ( + "path/filepath" + "strings" +) type nativeModel struct { Model } func (m nativeModel) Index(deviceID DeviceID, folder string, files []FileInfo) { - fixupFiles(folder, files) + files = fixupFiles(files) m.Model.Index(deviceID, folder, files) } func (m nativeModel) IndexUpdate(deviceID DeviceID, folder string, files []FileInfo) { - fixupFiles(folder, files) + files = fixupFiles(files) m.Model.IndexUpdate(deviceID, folder, files) } func (m nativeModel) Request(deviceID DeviceID, folder string, name string, offset int64, hash []byte, fromTemporary bool, buf []byte) error { + if strings.Contains(name, `\`) { + l.Warnln("Dropping request for %s, contains invalid path separator", name) + return ErrNoSuchFile + } + name = filepath.FromSlash(name) return m.Model.Request(deviceID, folder, name, offset, hash, fromTemporary, buf) } -func fixupFiles(folder string, files []FileInfo) { +func fixupFiles(files []FileInfo) []FileInfo { + var out []FileInfo for i := range files { + if strings.Contains(files[i].Name, `\`) { + l.Warnln("Dropping index entry for %s, contains invalid path separator", files[i].Name) + if out == nil { + // Most incoming updates won't contain anything invalid, so + // we delay the allocation and copy to output slice until we + // really need to do it, then copy all the so-far valid + // files to it. + out = make([]FileInfo, i, len(files)-1) + copy(out, files) + } + continue + } + + // Fixup the path separators files[i].Name = filepath.FromSlash(files[i].Name) + + if out != nil { + out = append(out, files[i]) + } } + + if out != nil { + // We did some filtering + return out + } + + // Unchanged + return files } diff --git a/lib/protocol/nativemodel_windows_test.go b/lib/protocol/nativemodel_windows_test.go new file mode 100644 index 00000000..2308f4fa --- /dev/null +++ b/lib/protocol/nativemodel_windows_test.go @@ -0,0 +1,29 @@ +// Copyright (C) 2016 The Protocol Authors. + +package protocol + +import "testing" +import "reflect" + +func TestFixupFiles(t *testing.T) { + files := []FileInfo{ + {Name: "foo/bar"}, + {Name: `foo\bar`}, + {Name: "foo/baz"}, + {Name: "foo/quux"}, + {Name: `foo\fail`}, + } + + // Filenames should be slash converted, except files which already have + // backslashes in them which are instead filtered out. + expected := []FileInfo{ + {Name: `foo\bar`}, + {Name: `foo\baz`}, + {Name: `foo\quux`}, + } + + fixed := fixupFiles(files) + if !reflect.DeepEqual(fixed, expected) { + t.Errorf("Got %v, expected %v", fixed, expected) + } +} diff --git a/lib/protocol/protocol.go b/lib/protocol/protocol.go index b147a1a2..fd355dc8 100644 --- a/lib/protocol/protocol.go +++ b/lib/protocol/protocol.go @@ -7,6 +7,8 @@ import ( "errors" "fmt" "io" + "path" + "strings" "sync" "time" @@ -55,6 +57,8 @@ var ( ErrTimeout = errors.New("read timeout") ErrSwitchingConnections = errors.New("switching connections") errUnknownMessage = errors.New("unknown message") + errInvalidFilename = errors.New("filename is invalid") + errUncleanFilename = errors.New("filename not in canonical format") ) type Model interface { @@ -310,6 +314,9 @@ func (c *rawConnection) readerLoop() (err error) { if state != stateReady { return fmt.Errorf("protocol error: index message in state %d", state) } + if err := checkFilenames(msg.Files); err != nil { + return fmt.Errorf("protocol error: index: %v", err) + } c.handleIndex(*msg) state = stateReady @@ -318,6 +325,9 @@ func (c *rawConnection) readerLoop() (err error) { if state != stateReady { return fmt.Errorf("protocol error: index update message in state %d", state) } + if err := checkFilenames(msg.Files); err != nil { + return fmt.Errorf("protocol error: index update: %v", err) + } c.handleIndexUpdate(*msg) state = stateReady @@ -326,6 +336,9 @@ func (c *rawConnection) readerLoop() (err error) { if state != stateReady { return fmt.Errorf("protocol error: request message in state %d", state) } + if err := checkFilename(msg.Name); err != nil { + return fmt.Errorf("protocol error: request: %q: %v", msg.Name, err) + } // Requests are handled asynchronously go c.handleRequest(*msg) @@ -451,38 +464,50 @@ func (c *rawConnection) readHeader() (Header, error) { func (c *rawConnection) handleIndex(im Index) { l.Debugf("Index(%v, %v, %d file)", c.id, im.Folder, len(im.Files)) - c.receiver.Index(c.id, im.Folder, filterIndexMessageFiles(im.Files)) + c.receiver.Index(c.id, im.Folder, im.Files) } func (c *rawConnection) handleIndexUpdate(im IndexUpdate) { l.Debugf("queueing IndexUpdate(%v, %v, %d files)", c.id, im.Folder, len(im.Files)) - c.receiver.IndexUpdate(c.id, im.Folder, filterIndexMessageFiles(im.Files)) + c.receiver.IndexUpdate(c.id, im.Folder, im.Files) } -func filterIndexMessageFiles(fs []FileInfo) []FileInfo { - var out []FileInfo - for i, f := range fs { - switch f.Name { - case "", ".", "..", "/": // A few obviously invalid filenames - l.Infof("Dropping invalid filename %q from incoming index", f.Name) - if out == nil { - // Most incoming updates won't contain anything invalid, so we - // delay the allocation and copy to output slice until we - // really need to do it, then copy all the so var valid files - // to it. - out = make([]FileInfo, i, len(fs)-1) - copy(out, fs) - } - default: - if out != nil { - out = append(out, f) - } +func checkFilenames(fs []FileInfo) error { + for _, f := range fs { + if err := checkFilename(f.Name); err != nil { + return fmt.Errorf("%q: %v", f.Name, err) } } - if out != nil { - return out + return nil +} + +// checkFilename verifies that the given filename is valid according to the +// spec on what's allowed over the wire. A filename failing this test is +// grounds for disconnecting the device. +func checkFilename(name string) error { + cleanedName := path.Clean(name) + if cleanedName != name { + // The filename on the wire should be in canonical format. If + // Clean() managed to clean it up, there was something wrong with + // it. + return errUncleanFilename } - return fs + + switch name { + case "", ".", "..": + // These names are always invalid. + return errInvalidFilename + } + if strings.HasPrefix(name, "/") { + // Names are folder relative, not absolute. + return errInvalidFilename + } + if strings.HasPrefix(name, "../") { + // Starting with a dotdot is not allowed. Any other dotdots would + // have been handled by the Clean() call at the top. + return errInvalidFilename + } + return nil } func (c *rawConnection) handleRequest(req Request) { diff --git a/lib/protocol/protocol_test.go b/lib/protocol/protocol_test.go index bce66de4..778f6349 100644 --- a/lib/protocol/protocol_test.go +++ b/lib/protocol/protocol_test.go @@ -273,3 +273,43 @@ func TestLZ4Compression(t *testing.T) { t.Logf("OK #%d, %d -> %d -> %d", i, dataLen, len(comp), dataLen) } } + +func TestCheckFilename(t *testing.T) { + cases := []struct { + name string + ok bool + }{ + // Valid filenames + {"foo", true}, + {"foo/bar/baz", true}, + {"foo/bar:baz", true}, // colon is ok in general, will be filtered on windows + {`\`, true}, // path separator on the wire is forward slash, so as above + {`\.`, true}, + {`\..`, true}, + {".foo", true}, + {"foo..", true}, + + // Invalid filenames + {"foo/..", false}, + {"foo/../bar", false}, + {"../foo/../bar", false}, + {"", false}, + {".", false}, + {"..", false}, + {"/", false}, + {"/.", false}, + {"/..", false}, + {"/foo", false}, + {"./foo", false}, + {"foo./", false}, + {"foo/.", false}, + {"foo/", false}, + } + + for _, tc := range cases { + err := checkFilename(tc.name) + if (err == nil) != tc.ok { + t.Errorf("Unexpected result for checkFilename(%q): %v", tc.name, err) + } + } +}