data connection stealing and bounce attack mitigation

README.md

@@ -26,7 +26,7 @@ If you're interested in a fully featured FTP server, you should use [sftpgo](htt
  * Active socket connections (PORT and EPRT commands)
  * IPv6 support (EPSV + EPRT)
  * Small memory footprint
- * Clean code: No sleep, no panic, no global sync (only around control/transfer connection per client) 
+ * Clean code: No sleep, no panic, no global sync (only around control/transfer connection per client)
  * Uses only the standard library except for:
    * [afero](https://github.com/spf13/afero) for generic file systems handling
    * [go-kit log](https://github.com/go-kit/kit/tree/master/log) (optional) for logging
@@ -48,7 +48,7 @@ If you're interested in a fully featured FTP server, you should use [sftpgo](htt
 The easiest way to test this library is to use [ftpserver](https://github.com/fclairamb/ftpserver).
 
 ## The driver
-The simplest way to get a good understanding of how the driver shall be implemented, you can have a look at the [tests driver](https://github.com/fclairamb/ftpserverlib/blob/master/driver_test.go). 
+The simplest way to get a good understanding of how the driver shall be implemented, you can have a look at the [tests driver](https://github.com/fclairamb/ftpserverlib/blob/master/driver_test.go).
 
 ### The base API
 
@@ -142,6 +142,10 @@ type Settings struct {
 	DisableSYST              bool             // Disable SYST
 	EnableCOMB               bool             // Enable COMB support
 	DefaultTransferType      TransferType     // Transfer type to use if the client don't send the TYPE command
+	// ActiveConnectionsCheck defines the security requirements for active connections
+	ActiveConnectionsCheck DataConnectionRequirement
+	// PasvConnectionsCheck defines the security requirements for passive connections
+	PasvConnectionsCheck DataConnectionRequirement
 }
 ```
 
@@ -193,7 +197,7 @@ type ClientDriverExtensionHasher interface {
 
 I wanted to make a system which would accept files through FTP and redirect them to something else. Go seemed like the obvious choice and it seemed there was a lot of libraries available but it turns out none of them were in a useable state.
 
-* [micahhausler/go-ftp](https://github.com/micahhausler/go-ftp) is a  minimalistic implementation 
+* [micahhausler/go-ftp](https://github.com/micahhausler/go-ftp) is a  minimalistic implementation
 * [shenfeng/ftpd.go](https://github.com/shenfeng/ftpd.go) is very basic and 4 years old.
 * [yob/graval](https://github.com/yob/graval) is 3 years old and “experimental”.
 * [goftp/server](https://github.com/goftp/server) seemed OK but I couldn't use it on both Filezilla and the MacOs ftp client.

client_handler.go

@@ -635,6 +635,56 @@ func (c *clientHandler) TransferClose(err error) {
 	}
 }
 
+func (c *clientHandler) checkDataConnectionRequirement(dataConnIP net.IP, channelType DataChannel) error {
+	var requirement DataConnectionRequirement
+
+	switch channelType {
+	case DataChannelActive:
+		requirement = c.server.settings.ActiveConnectionsCheck
+	case DataChannelPassive:
+		requirement = c.server.settings.PasvConnectionsCheck
+	}
+
+	switch requirement {
+	case IPMatchRequired:
+		controlConnIP, err := getIPFromRemoteAddr(c.RemoteAddr())
+		if err != nil {
+			return err
+		}
+
+		if !controlConnIP.Equal(dataConnIP) {
+			return &ipValidationError{error: fmt.Sprintf("data connection ip address %v "+
+				"does not match control connection ip address %v",
+				dataConnIP, controlConnIP)}
+		}
+
+		return nil
+	case IPMatchDisabled:
+		return nil
+	default:
+		return &ipValidationError{error: fmt.Sprintf("unhandled data connection requirement: %v",
+			requirement)}
+	}
+}
+
+func getIPFromRemoteAddr(remoteAddr net.Addr) (net.IP, error) {
+	if remoteAddr == nil {
+		return nil, &ipValidationError{error: "nil remote address"}
+	}
+
+	ip, _, err := net.SplitHostPort(remoteAddr.String())
+	if err != nil {
+		return nil, err
+	}
+
+	remoteIP := net.ParseIP(ip)
+	if remoteIP == nil {
+		return nil, &ipValidationError{error: fmt.Sprintf("invalid remote IP: %v", ip)}
+	}
+
+	return remoteIP, nil
+}
+
 func parseLine(line string) (string, string) {
 	params := strings.SplitN(line, " ", 2)
 	if len(params) == 1 {

client_handler_test.go

@@ -344,3 +344,115 @@ func TestUnknownCommand(t *testing.T) {
 	require.Equal(t, StatusSyntaxErrorNotRecognised, rc)
 	require.Equal(t, fmt.Sprintf("Unknown command %#v", cmd), response)
 }
+
+// testNetConn implements net.Conn interface
+type testNetConn struct {
+	remoteAddr net.Addr
+}
+
+func (*testNetConn) Read(b []byte) (n int, err error) {
+	return
+}
+
+func (*testNetConn) Write(b []byte) (n int, err error) {
+	return
+}
+
+func (*testNetConn) Close() error {
+	return nil
+}
+
+func (*testNetConn) LocalAddr() net.Addr {
+	return nil
+}
+
+func (c *testNetConn) RemoteAddr() net.Addr {
+	return c.remoteAddr
+}
+
+func (*testNetConn) SetDeadline(t time.Time) error {
+	return nil
+}
+
+func (*testNetConn) SetReadDeadline(t time.Time) error {
+	return nil
+}
+
+func (*testNetConn) SetWriteDeadline(t time.Time) error {
+	return nil
+}
+
+// testNetListener implements net.Listener interface
+type testNetListener struct {
+	conn net.Conn
+}
+
+func (l *testNetListener) Accept() (net.Conn, error) {
+	if l.conn != nil {
+		return l.conn, nil
+	}
+
+	return nil, &net.AddrError{}
+}
+
+func (*testNetListener) Close() error {
+	return nil
+}
+
+func (*testNetListener) Addr() net.Addr {
+	return nil
+}
+
+func TestDataConnectionRequirements(t *testing.T) {
+	controlConnIP := net.ParseIP("192.168.1.1")
+
+	c := clientHandler{
+		conn: &testNetConn{
+			remoteAddr: &net.TCPAddr{IP: controlConnIP, Port: 21},
+		},
+		server: &FtpServer{
+			settings: &Settings{
+				PasvConnectionsCheck:   IPMatchRequired,
+				ActiveConnectionsCheck: IPMatchRequired,
+			},
+		},
+	}
+
+	err := c.checkDataConnectionRequirement(controlConnIP, DataChannelPassive)
+	assert.NoError(t, err) // ip match
+
+	err = c.checkDataConnectionRequirement(net.ParseIP("192.168.1.2"), DataChannelActive)
+	if assert.Error(t, err) {
+		assert.Contains(t, err.Error(), "does not match control connection ip address")
+	}
+
+	c.conn = &testNetConn{
+		remoteAddr: &net.IPAddr{IP: controlConnIP},
+	}
+
+	err = c.checkDataConnectionRequirement(controlConnIP, DataChannelPassive)
+	assert.Error(t, err)
+
+	// nil remote address
+	c.conn = &testNetConn{}
+	err = c.checkDataConnectionRequirement(controlConnIP, DataChannelActive)
+	assert.Error(t, err)
+
+	// invalid IP
+	c.conn = &testNetConn{
+		remoteAddr: &net.TCPAddr{IP: nil, Port: 21},
+	}
+
+	err = c.checkDataConnectionRequirement(controlConnIP, DataChannelPassive)
+	if assert.Error(t, err) {
+		assert.Contains(t, err.Error(), "invalid remote IP")
+	}
+
+	// invalid setting
+	c.server.settings.PasvConnectionsCheck = 100
+	err = c.checkDataConnectionRequirement(controlConnIP, DataChannelPassive)
+
+	if assert.Error(t, err) {
+		assert.Contains(t, err.Error(), "unhandled data connection requirement")
+	}
+}

driver.go

@@ -33,14 +33,23 @@ type MainDriver interface {
 // MainDriverExtensionTLSVerifier is an extension that allows to verify the TLS connection
 // estabilished on the control channel
 type MainDriverExtensionTLSVerifier interface {
-
 	// VerifyConnection is called when receiving the "USER" command.
 	// If it returns a non-nil error, the client will receive a 530 error and it will be disconnected.
 	// If it returns a non-nil ClientDriver and a nil error the client will be authenticated.
 	// If it returns a nil ClientDriver and a nil error the user password is required
 	VerifyConnection(cc ClientContext, user string, tlsConn *tls.Conn) (ClientDriver, error)
 }
 
+// MainDriverExtensionPassiveWrapper is an extension that allows to wrap the listener
+// used for passive connection
+type MainDriverExtensionPassiveWrapper interface {
+	// WrapPassiveListener is called after creating the listener for passive
+	// data connections.
+	// You can wrap the passed listener or just return it unmodified.
+	// Returning an error will cause the passive connection to fail
+	WrapPassiveListener(listener net.Listener) (net.Listener, error)
+}
+
 // ClientDriver is the base FS implementation that allows to manipulate files
 type ClientDriver interface {
 	afero.Fs
@@ -185,6 +194,18 @@ const (
 	ImplicitEncryption
 )
 
+// DataConnectionRequirement is the enumerable that represents the supported
+// protection mode for data channels
+type DataConnectionRequirement int
+
+// Supported data connection requirements
+const (
+	// IPMatchRequired requires matching peer IP addresses of control and data connection
+	IPMatchRequired DataConnectionRequirement = iota
+	// IPMatchDisabled disables checking peer IP addresses of control and data connection
+	IPMatchDisabled
+)
+
 // Settings defines all the server settings
 // nolint: maligned
 type Settings struct {
@@ -209,4 +230,8 @@ type Settings struct {
 	DisableSYST              bool             // Disable SYST
 	EnableCOMB               bool             // Enable COMB support
 	DefaultTransferType      TransferType     // Transfer type to use if the client don't send the TYPE command
+	// ActiveConnectionsCheck defines the security requirements for active connections
+	ActiveConnectionsCheck DataConnectionRequirement
+	// PasvConnectionsCheck defines the security requirements for passive connections
+	PasvConnectionsCheck DataConnectionRequirement
 }

driver_test.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"io"
 	"io/ioutil"
+	"net"
 	"os"
 	"strings"
 	"sync"
@@ -101,6 +102,7 @@ type TestServerDriver struct {
 	clientMU             sync.Mutex
 	Clients              []ClientContext
 	TLSVerificationReply tlsVerificationReply
+	errPassiveListener   error
 }
 
 // TestClientDriver defines a minimal serverftp client driver
@@ -326,6 +328,14 @@ func (driver *TestServerDriver) VerifyConnection(cc ClientContext, user string,
 	return nil, nil
 }
 
+func (driver *TestServerDriver) WrapPassiveListener(listener net.Listener) (net.Listener, error) {
+	if driver.errPassiveListener != nil {
+		return nil, driver.errPassiveListener
+	}
+
+	return listener, nil
+}
+
 // OpenFile opens a file in 3 possible modes: read, write, appending write (use appropriate flags)
 func (driver *TestClientDriver) OpenFile(path string, flag int, perm os.FileMode) (afero.File, error) {
 	if strings.Contains(path, "fail-to-open") {

transfer_active.go

@@ -30,7 +30,17 @@ func (c *clientHandler) handlePORT(param string) error {
 	}
 
 	if err != nil {
-		c.writeMessage(StatusSyntaxErrorNotRecognised, fmt.Sprintf("Problem parsing %s: %v", param, err))
+		c.writeMessage(StatusSyntaxErrorParameters, fmt.Sprintf("Problem parsing %s: %v", param, err))
+
+		return nil
+	}
+
+	err = c.checkDataConnectionRequirement(raddr.IP, DataChannelActive)
+	if err != nil {
+		// we don't want to expose the full error to the client, we just log it
+		c.logger.Warn("Could not validate active data connection requirement", "err", err)
+		c.writeMessage(StatusSyntaxErrorParameters, "Your request does not meet "+
+			"the configured security requirements")
 
 		return nil
 	}

transfer_pasv.go

@@ -35,6 +35,8 @@ type passiveTransferHandler struct {
 	settings    *Settings        // Settings
 	info        string           // transfer info
 	logger      log.Logger       // Logger
+	// data connection requirement checker
+	checkDataConn func(dataConnIP net.IP, channelType DataChannel) error
 }
 
 type ipValidationError struct {
@@ -142,24 +144,35 @@ func (c *clientHandler) handlePASV(param string) error {
 	// The listener will either be plain TCP or TLS
 	var listener net.Listener
 
+	listener = tcpListener
+
+	if wrapper, ok := c.server.driver.(MainDriverExtensionPassiveWrapper); ok {
+		listener, err = wrapper.WrapPassiveListener(listener)
+		if err != nil {
+			c.logger.Error("Could not wrap passive connection", "err", err)
+			c.writeMessage(StatusServiceNotAvailable, fmt.Sprintf("Could not listen for passive connection: %v", err))
+
+			return nil
+		}
+	}
+
 	if c.HasTLSForTransfers() || c.server.settings.TLSRequired == ImplicitEncryption {
 		if tlsConfig, err := c.server.driver.GetTLSConfig(); err == nil {
-			listener = tls.NewListener(tcpListener, tlsConfig)
+			listener = tls.NewListener(listener, tlsConfig)
 		} else {
 			c.writeMessage(StatusServiceNotAvailable, fmt.Sprintf("Cannot get a TLS config: %v", err))
 
 			return nil
 		}
-	} else {
-		listener = tcpListener
 	}
 
 	p := &passiveTransferHandler{
-		tcpListener: tcpListener,
-		listener:    listener,
-		Port:        tcpListener.Addr().(*net.TCPAddr).Port,
-		settings:    c.server.settings,
-		logger:      c.logger,
+		tcpListener:   tcpListener,
+		listener:      listener,
+		Port:          tcpListener.Addr().(*net.TCPAddr).Port,
+		settings:      c.server.settings,
+		logger:        c.logger,
+		checkDataConn: c.checkDataConnectionRequirement,
 	}
 
 	// We should rewrite this part
@@ -201,6 +214,20 @@ func (p *passiveTransferHandler) ConnectionWait(wait time.Duration) (net.Conn, e
 		if err != nil {
 			return nil, err
 		}
+
+		ip, err := getIPFromRemoteAddr(p.connection.RemoteAddr())
+		if err != nil {
+			p.logger.Warn("Could get remote passive IP address", "err", err)
+
+			return nil, err
+		}
+
+		if err := p.checkDataConn(ip, DataChannelPassive); err != nil {
+			// we don't want to expose the full error to the client, we just log it
+			p.logger.Warn("Could not validate passive data connection requirement", "err", err)
+
+			return nil, &ipValidationError{error: "data connection security requirements not met"}
+		}
 	}
 
 	return p.connection, nil