lib/protocol: Apply input filtering on file names

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

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
 }

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)
+	}
+}

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 {
+func checkFilenames(fs []FileInfo) error {
-	var out []FileInfo
+	for _, f := range fs {
-	for i, f := range fs {
+		if err := checkFilename(f.Name); err != nil {
-		switch f.Name {
+			return fmt.Errorf("%q: %v", f.Name, err)
-		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)
-			}
 		}
 	}
-	if out != nil {
+	return nil
-		return out
+}
+
+// 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) {

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)
+		}
+	}
+}