Attachment 936905 Details for Bug 1139181 – updated fix

[patch] updated fix

wget_fix.patch (text/plain), 7.49 KB, created by Giuseppe Scrivano on 2014-09-12 10:40:43 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Giuseppe Scrivano

Created: 2014-09-12 10:40:43 UTC

Size: 7.49 KB

patch

obsolete

>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/ChangeLog |  6 ++++++
> doc/wget.texi | 23 ++++++++++++-----------
> src/ChangeLog |  5 +++++
> src/init.c    | 16 ++++++++++++++++
> 4 files changed, 39 insertions(+), 11 deletions(-)
>
>diff --git a/doc/ChangeLog b/doc/ChangeLog
>index f055fa5..41118f4 100644
>--- a/doc/ChangeLog
>+++ b/doc/ChangeLog
>@@ -1,3 +1,9 @@
>+2014-09-08  Darshit Shah  <darnir@gmail.com>
>+
>+	* wget.texi (symbolic links): Update documentation of retr-symlinks to
>+	reflect the new default. Add warning about potential security issues with
>+	--retr-symlinks=yes.
>+
> 2014-08-03  Giuseppe Scrivano  <gscrivano@gnu.org>
> 
> 	* wget.texi (Download Options): Fix texinfo warning.
>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/ChangeLog b/src/ChangeLog
>index f677f12..fdc41fa 100644
>--- a/src/ChangeLog
>+++ b/src/ChangeLog
>@@ -1,3 +1,8 @@
>+2014-09-08  Darshit Shah  <darnir@gmail.com>
>+
>+	* init.c (defaults): Set retr-symlinks to true by default. This changes a
>+	default setting of Wget. Fixes security bug R7-2014-15.
>+
> 2014-07-23  Darshit Shah  <darnir@gmail.com>
> 
> 	* http.c (gethttp): Fix a memory leak when retrying authorization
>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/ChangeLog |  7 +++++++
> src/ftp.c     | 27 +++++++++++++++++++++++++--
> 2 files changed, 32 insertions(+), 2 deletions(-)
>
>diff --git a/src/ChangeLog b/src/ChangeLog
>index fdc41fa..207a839 100644
>--- a/src/ChangeLog
>+++ b/src/ChangeLog
>@@ -1,5 +1,12 @@
> 2014-09-08  Darshit Shah  <darnir@gmail.com>
> 
>+	* ftp.c (ftp_retrieve_glob): Also check for invalid entries along with
>+	harmful filenames
>+	(is_valid_entry): New function. Check if the provided node is a valid entry
>+	in a listing file.
>+
>+2014-09-08  Darshit Shah  <darnir@gmail.com>
>+
> 	* init.c (defaults): Set retr-symlinks to true by default. This changes a
> 	default setting of Wget. Fixes security bug R7-2014-15.
> 
>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
>

Actions: View | Diff

Attachments on bug 1139181: 935576 | 936905