From d8d545994be399705c483ea924e71c3e6348d99d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim Rühsen?= <address@hidden>
Date: Tue, 11 Aug 2015 16:48:08 +0200
Subject: [PATCH] Fix IP address exposure in FTP code

* src/ftp.c (getftp): Do not use PORT when PASV fails.
* tests/FTPServer.px: Add pasv_not_supported server flag.
* tests/Makefile.am: Add Test-ftp-pasv-not-supported.px
* tests/Test-ftp-pasv-not-supported.px: New test

Fix IP address exposure when automatically falling back from
passive mode to active mode (using the PORT command). A behavior that
may be used to expose a client's privacy even when using a proxy.
---
 src/ftp.c                            | 19 +++++++-----
 tests/FTPServer.pm                   |  8 +++++
 tests/Makefile.am                    |  3 +-
 tests/Test-ftp-pasv-not-supported.px | 57 ++++++++++++++++++++++++++++++++++++
 4 files changed, 79 insertions(+), 8 deletions(-)
 create mode 100755 tests/Test-ftp-pasv-not-supported.px

diff --git a/src/ftp.c b/src/ftp.c
index 68f1a33..9dab99c 100644
--- a/src/ftp.c
+++ b/src/ftp.c
@@ -252,7 +252,6 @@ getftp (struct url *u, wgint passed_expected_bytes, wgint *qtyread,
   char *respline, *tms;
   const char *user, *passwd, *tmrate;
   int cmd = con->cmd;
-  bool pasv_mode_open = false;
   wgint expected_bytes = 0;
   bool got_expected_bytes = false;
   bool rest_failed = false;
@@ -883,13 +882,19 @@ Error in server response, closing control connection.\n"));
                           ? CONERROR : CONIMPOSSIBLE);
                 }

-              pasv_mode_open = true;  /* Flag to avoid accept port */
               if (!opt.server_response)
                 logputs (LOG_VERBOSE, _("done.    "));
-            } /* err==FTP_OK */
-        }
+            }
+          else
+            return err;

-      if (!pasv_mode_open)   /* Try to use a port command if PASV failed */
+          /*
+           * We do not want to fall back from PASSIVE mode to ACTIVE mode !
+           * The reason is the PORT command exposes the client's real IP address
+           * to the server. Bad for someone who relies on privacy via a ftp proxy.
+           */
+        }
+      else
         {
           err = ftp_do_port (csock, &local_sock);
           /* FTPRERR, WRITEFAILED, bindport (FTPSYSERR), HOSTERR,
@@ -1148,8 +1153,8 @@ Error in server response, closing control connection.\n"));
     }

   /* If no transmission was required, then everything is OK.  */
-  if (!pasv_mode_open)  /* we are not using pasive mode so we need
-                              to accept */
+  if (!opt.ftp_pasv)  /* we are not using passive mode so we need
+                         to accept */
     {
       /* Wait for the server to connect to the address we're waiting
          at.  */
diff --git a/tests/FTPServer.pm b/tests/FTPServer.pm
index c0a6e47..a5185d6 100644
--- a/tests/FTPServer.pm
+++ b/tests/FTPServer.pm
@@ -740,6 +740,14 @@ sub run
                     last;
                 }

+                if (defined($self->{_server_behavior}{pasv_not_supported})
+                    && $cmd eq 'PASV')
+                {
+                    print {$conn->{socket}}
+                      "500 PASV not supported.\r\n";
+                    next;
+                }
+
                 # Run the command.
                 &{$command_table->{$cmd}}($conn, $cmd, $rest);
             }
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 5d387aa..daf162f 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -127,7 +127,8 @@ PX_TESTS = \
              Test--start-pos.px \
              Test--start-pos--continue.px \
              Test--httpsonly-r.px \
-             Test-204.px
+             Test-204.px \
+             Test-ftp-pasv-not-supported.px

 EXTRA_DIST = FTPServer.pm FTPTest.pm HTTPServer.pm HTTPTest.pm \
              WgetTests.pm WgetFeature.pm WgetFeature.cfg $(PX_TESTS) \
diff --git a/tests/Test-ftp-pasv-not-supported.px b/tests/Test-ftp-pasv-not-supported.px
new file mode 100755
index 0000000..0b79996
--- /dev/null
+++ b/tests/Test-ftp-pasv-not-supported.px
@@ -0,0 +1,57 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use FTPTest;
+
+# This file exercises a problem in Wget, where if an error was
+# encountered in ftp.c:getftp before the actual file download
+# had started, Wget would believe that it had already downloaded the
+# full contents of the file, and would send a corresponding (erroneous)
+# REST value.
+
+###############################################################################
+
+# From bug report. :)
+my $afile = <<EOF;
+I've included log output (using the -d switch) from when this happens
+below. You'll see that for the retry wget sends a REST command to
+reset the start position before starting the RETR command. I'm
+confused about the argument to REST: 51132. It's the full length in
+bytes of the file to be retrieved. The RETR then shows the entire
+contents of the file being skipped, and wget announces that it
+successfully retrieved and saved 0 bytes.
+EOF
+
+$afile =~ s/\n/\r\n/g;
+
+
+# code, msg, headers, content
+my %urls = (
+    '/afile.txt' => {
+        content => $afile,
+    },
+);
+
+my $cmdline = $WgetTest::WGETPATH . " -S ftp://localhost:{{port}}/afile.txt";
+
+my $expected_error_code = 8;
+
+my %expected_downloaded_files = (
+    'afile.txt' => {
+        content => $afile,
+    },
+);
+
+###############################################################################
+
+my $the_test = FTPTest->new (
+                             server_behavior => {pasv_not_supported => 1},
+                             input => \%urls,
+                             cmdline => $cmdline,
+                             errcode => $expected_error_code,
+                             output => \%expected_downloaded_files);
+exit !$the_test->run();
+
+# vim: et ts=4 sw=4
--
2.5.0