From 043366ac3248a58662a6fbf47a1dd688a75d0e78 Mon Sep 17 00:00:00 2001

From: Darshit Shah <darnir@gmail.com>

Date: Mon, 8 Sep 2014 00:41:17 +0530

Subject: [PATCH 1/2] Fix R7-2014-15: Arbitrary Symlink Access

Wget was susceptible to a symlink attack which could create arbitrary

files, directories or symbolic links and set their permissions when

retrieving a directory recursively through FTP. This commit changes the

default settings in Wget such that Wget no longer creates local symbolic

links, but rather traverses them and retrieves the pointed-to file in

such a retrieval.

The old behaviour can be attained by passing the --retr-symlinks=no

option to the Wget invokation command.

---

 doc/wget.texi | 23 ++++++++++++-----------

 src/init.c    | 16 ++++++++++++++++

 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/doc/wget.texi b/doc/wget.texi

index a31eb5e..f54e98d 100644

--- a/doc/wget.texi

+++ b/doc/wget.texi

@@ -1883,17 +1883,18 @@ Preserve remote file permissions instead of permissions set by umask.

 @cindex symbolic links, retrieving

 @item --retr-symlinks

-Usually, when retrieving @sc{ftp} directories recursively and a symbolic

-link is encountered, the linked-to file is not downloaded.  Instead, a

-matching symbolic link is created on the local filesystem.  The

-pointed-to file will not be downloaded unless this recursive retrieval

-would have encountered it separately and downloaded it anyway.

-

-When @samp{--retr-symlinks} is specified, however, symbolic links are

-traversed and the pointed-to files are retrieved.  At this time, this

-option does not cause Wget to traverse symlinks to directories and

-recurse through them, but in the future it should be enhanced to do

-this.

+By default, when retrieving @sc{ftp} directories recursively and a symbolic link

+is encountered, the symbolic link is traversed and the pointed-to files are

+retrieved.  Currently, Wget does not traverse symbolic links to directories to

+download them recursively, though this feature may be added in the future.

+

+When @samp{--retr-symlinks=no} is specified, the linked-to file is not

+downloaded.  Instead, a matching symbolic link is created on the local

+filesystem.  The pointed-to file will not be retrieved unless this recursive

+retrieval would have encountered it separately and downloaded it anyway.  This

+option poses a security risk where a malicious FTP Server may cause Wget to

+write to files outside of the intended directories through a specially crafted

+@sc{.listing} file.

 Note that when retrieving a file (not a directory) because it was

 specified on the command-line, rather than because it was recursed to,

diff --git a/src/init.c b/src/init.c

index 93e95f8..94b6f8b 100644

--- a/src/init.c

+++ b/src/init.c

@@ -366,6 +366,22 @@ defaults (void)

   opt.dns_cache = true;

   opt.ftp_pasv = true;

+  /* 2014-09-07  Darshit Shah  <darnir@gmail.com>

+   * opt.retr_symlinks is set to true by default. Creating symbolic links on the

+   * local filesystem pose a security threat by malicious FTP Servers that

+   * server a specially crafted .listing file akin to this:

+   *

+   * lrwxrwxrwx   1 root     root           33 Dec 25  2012 JoCxl6d8rFU -> /

+   * drwxrwxr-x  15 1024     106          4096 Aug 28 02:02 JoCxl6d8rFU

+   *

+   * A .listing file in this fashion makes Wget susceptiple to a symlink attack

+   * wherein the attacker is able to create arbitrary files, directories and

+   * symbolic links on the target system and even set permissions.

+   *

+   * Hence, by default Wget attempts to retrieve the pointed-to files and does

+   * not create the symbolic links locally.

+   */

+  opt.retr_symlinks = true;

 #ifdef HAVE_SSL

   opt.check_cert = true;

-- 

2.1.0

From bfa8c9cc9937f686a4de110e49710061267f8d9e Mon Sep 17 00:00:00 2001

From: Darshit Shah <darnir@gmail.com>

Date: Mon, 8 Sep 2014 15:07:45 +0530

Subject: [PATCH 2/2] Add checks for valid listing file in FTP

When Wget retrieves a file through FTP, it first downloads a .listing

file and parses it for information about the files and other metadata.

Some servers may serve invalid .listing files. This patch checks for one

such known inconsistency wherein multiple lines in a listing file have

the same name. Such a filesystem is clearly not possible and hence we

eliminate duplicate entries here.

Signed-off-by: Darshit Shah <darnir@gmail.com>

---

 src/ftp.c     | 27 +++++++++++++++++++++++++--

 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/src/ftp.c b/src/ftp.c

index 2d54333..054cb61 100644

--- a/src/ftp.c

+++ b/src/ftp.c

@@ -2211,6 +2211,29 @@ has_insecure_name_p (const char *s)

   return false;

 }

+/* Test if the file node is invalid. This can occur due to malformed or

+ * maliciously crafted listing files being returned by the server.

+ *

+ * Currently, this function only tests if there are multiple entries in the

+ * listing file by the same name. However this function can be expanded as more

+ * such illegal listing formats are discovered. */

+static bool

+is_invalid_entry (struct fileinfo *f)

+{

+  struct fileinfo *cur;

+  cur = f;

+  char *f_name = f->name;

+  /* If the node we're currently checking has a duplicate later, we eliminate

+   * the current node and leave the next one intact. */

+  while (cur->next)

+    {

+      cur = cur->next;

+      if (strcmp(f_name, cur->name) == 0)

+          return true;

+    }

+  return false;

+}

+

 /* A near-top-level function to retrieve the files in a directory.

    The function calls ftp_get_listing, to get a linked list of files.

    Then it weeds out the file names that do not match the pattern.

@@ -2248,11 +2271,11 @@ ftp_retrieve_glob (struct url *u, ccon *con, int action)

             f = f->next;

         }

     }

-  /* Remove all files with possible harmful names */

+  /* Remove all files with possible harmful names or invalid entries. */

   f = start;

   while (f)

     {

-      if (has_insecure_name_p (f->name))

+      if (has_insecure_name_p (f->name) || is_invalid_entry (f))

         {

           logprintf (LOG_VERBOSE, _("Rejecting %s.\n"),

                      quote (f->name));

-- 

2.1.0