You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
151 lines
6.0 KiB
151 lines
6.0 KiB
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 |
|
|
|
|