Don't allow separate transfer hosts by default

Author: patrickjuchli

README.md

@@ -69,7 +69,7 @@ client.ftp.verbose = true
 
 Create a client instance. Configure it with a timeout in milliseconds that will be used for any connection made. Use 0 to disable timeouts, default is 30 seconds. Options are: 
 
-- `allowSeparateTransferHost (boolean)`, the FTP spec makes it possible for a server to tell the client to use a different IP address for file transfers than for the initial control connection. Today, this feature is very rarely used. Still, the default for this is set to `true` for backwards-compatibility reasons. If you experience any issues with NAT traversal in local networks or want to provide more security and prevent FTP bounce attacks, set this to `false`.
+- `allowSeparateTransferHost (boolean)`, the FTP spec makes it possible for a server to tell the client to use a different IP address for file transfers than for the initial control connection. This is a potential vector for FTP bounce attacks, so by default this is set to `false` and the library will throw an error if a server tries to redirect transfers to a different host. Set this to `true` only if you are connecting to a server that legitimately requires it.
 
 `close()`
 

src/Client.ts

@@ -60,7 +60,8 @@ export interface ClientOptions {
 }
 
 const defaultClientOptions: ClientOptions = {
-    allowSeparateTransferHost: true,
+    /** For security reasons this library should not allow separate transfer hosts by default. */
+    allowSeparateTransferHost: false,
     maxListingBytes: 40 * 1024 * 1024
 }
 const LIST_COMMANDS_DEFAULT = () => ["LIST -a", "LIST"]

src/transfer.ts

@@ -64,9 +64,11 @@ export async function enterPassiveModeIPv4(ftp: FTPContext): Promise<FTPResponse
 }
 
 /**
- * Prepare a data socket using passive mode over IPv4. Ignore the IP provided by the PASV response,
- * and use the control host IP. This is the same behaviour as with the more modern variant EPSV. Use
- * this to fix issues around NAT or provide more security by preventing FTP bounce attacks. 
+ * Prepare a data socket using passive mode over IPv4. 
+ * 
+ * Will throw an error if the IP provided by the PASV response doesn't match the one of the control connection.
+ * The error will contain detailed information. This is done to provide more security by preventing FTP bounce
+ * attacks.
  */
 export async function enterPassiveModeIPv4_forceControlHostIP(ftp: FTPContext): Promise<FTPResponse> {
     const res = await ftp.request("PASV")
@@ -78,7 +80,13 @@ export async function enterPassiveModeIPv4_forceControlHostIP(ftp: FTPContext):
     if (controlHost === undefined) {
         throw new Error("Control socket is disconnected, can't get remote address.")
     }
-    await connectForPassiveTransfer(controlHost, target.port, ftp)
+    // Strip IPv4-mapped IPv6 prefix (e.g. "::ffff:1.2.3.4" → "1.2.3.4") so the
+    // comparison works regardless of whether the OS uses a dual-stack socket.
+    const normalizedControlHost = controlHost.replace(/^::ffff:/i, "")
+    if (normalizedControlHost !== target.host) {
+        throw new Error(`PASV returned another host (${target.host}) for data transfer that you have connected to (${controlHost}). Even though the FTP protocol allows this, basic-ftp disables this feature by default for security reasons. If you do need this feature, instantiate the Client with the optional paramter "allowSeparateTransferHost: true". See the README documentation for more information.`)
+    }
+    await connectForPassiveTransfer(normalizedControlHost, target.port, ftp)
     return res
 }