Please review / test this patch. Please check the 'Reported-by' in the commit message and if you got a CVE number, please report for inclusion into the commit message (and/or the code).
Regards, Tim On Mittwoch, 17. August 2016 10:40:35 CEST Dawid Golunski wrote: > Random file name + .part extension on temporary files would already be > good improvement (even if still stored within the same directory) and > help prevent the exploitation. > > Thanks. > > On Wed, Aug 17, 2016 at 9:22 AM, Tim Rühsen <[email protected]> wrote: > > On Mittwoch, 17. August 2016 13:37:33 CEST Ander Juaristi wrote: > >> I was thinking we could rename php extensions to phps, but it's all the > >> same thing in the end, and even better, since the former applies to any > >> kind of file and I've seen some broken servers that actually run phps > >> files.>> > >> So, this is what I would do: > >> 1. Write temporary files with 600 perms, and make sure they're owned > >> > >> by the running user and group. qmail goes even further [1] by not > >> letting root run, but I would not do that here. > >> > >> 2. Use mkostemp() to generate a unique filename and give it a > >> > >> harmless extension (like Mozilla's .part). We already have unique_name() > >> in utils.c, altough it returns the file name untouched if it does not > >> exist. We should do some research on whether we could reuse parts of it. > > > > Giuseppe and I have a working patch that is basically like this. We are > > still discussing the details (mkstemp extension, fixed extension, both or > > maybe a mkdtemp directory where we put all the temp files). > > > > As soon as we agree, we'll post the patch here for further > > discussion/review.> > >> 3. Place them in /tmp, or even better, in ~/.wget-tempfiles, or > >> > >> something like that. > > > > /tmp often is on a separate filesystem (e.g. RAM disk) with limited space. > > This could open another (DOS) attack vector. > > > > You do not always have a home directory when running Wget. > > > >> There's a patch by Tim somewhere in this list that already does 1 (but > >> please, remove the braces ;D). > >> > >> It also comes to my mind, instead of writing each temp file to its own > >> file, we could put them all in the same file (with O_APPEND). But a) we > >> need a way to tell them apart later, and b) it may cause problems in > >> NFS, according to open(2). > >> > >> [1] http://cr.yp.to/qmail/guarantee.html > >> > >> On 15/08/16 18:31, Tim Rühsen wrote: > >> > On Montag, 15. August 2016 10:02:55 CEST moparisthebest wrote: > >> >> Hello, > >> >> > >> >> I find it extremely hard to call this a wget vulnerability when SO > >> >> many > >> >> other things are wrong with that 'vulnerable code' implementation it > >> >> isn't even funny: > >> >> > >> >> 1. The image_importer.php script takes a single argument, why would it > >> >> download with the recursive switch turned on? Isn't that clearly a > >> >> bug > >> >> in the php script? Has a php script like this that downloads all > >> >> files > >> >> from a website of a particular extension ever been observed in the > >> >> wild? > >> >> > >> >> 2. A *well* configured server would have a whitelist of .php files it > >> >> will execute, making it immune to this. A *decently* configured > >> >> server > >> >> would always at a minimum make sure they don't execute code in > >> >> directories with user provided uploads in them. So it's additionally > >> >> a > >> >> bug in the server configuration. (incidentally every php package I've > >> >> downloaded has at minimum a .htaccess in upload directories to prevent > >> >> this kind of thing with apache) > >> >> > >> >> It seems to me like there has always been plenty of ways to shoot > >> >> yourself in the foot with PHP, and this is just another iteration on a > >> >> theme. > >> > > >> > Hi, > >> > > >> > this is absolutely true and your points were the first things that came > >> > to > >> > my mind when reading the original post. > >> > > >> > But there is also non-obvious wget behavior in creating those (temp) > >> > files > >> > in the filesystem. And there is also a long history of attack vectors > >> > introduced by temp files as well. > >> > > >> > Today the maintainers discussed a few possible fixes, all with pros and > >> > cons. I would like to list them here, in case someone likes to comment: > >> > > >> > 1. Rewrite code to keep temp files in memory. > >> > Too complex, needs a redesign of wget. And has been done for wget2... > >> > > >> > 2. Add a harmless extension to the file names. > >> > Possible name collision with wanted files. > >> > Possible name length issues, have to be worked around. > >> > > >> > 3. Using file mode 0 (no flags at all). > >> > Short vulnerability when changing modes to write/read the data. > >> > > >> > 4. Using O_TMPFILE for open(). > >> > Just for Linux, not for every filesystem available. > >> > > >> > 5. Using mkostemp(). > >> > Possible name collision with wanted files (which would be unexpectedly > >> > named as *.1 in case of a collision). At least the chance for a > >> > collision > >> > seems very low. > >> > > >> > Any thoughts or other ideas ? > >> > > >> > Regards, Tim
From 0b2fa64ba2898cc49b28da3b43b658d728448772 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim Rühsen?= <[email protected]> Date: Sun, 14 Aug 2016 21:04:58 +0200 Subject: [PATCH] Use wget_XXXXXX.part for temp. file names * bootstrap.conf: Add gnulib modules fopen, open, mkstemps. * src/http.c: New static function fopen_tmp(), creates temp. files named wget_XXXXXX.part. (open_output_stream): Call fopen_tmp() for temp. files. Reported-by: "Misra, Deapesh" <[email protected]> --- bootstrap.conf | 5 ++++- src/http.c | 36 +++++++++++++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index 2b225b7..eece390 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -40,6 +40,7 @@ dirname fcntl flock fnmatch +fopen futimens ftello getaddrinfo @@ -63,14 +64,16 @@ mbiter mbtowc memrchr mkdir -mkstemp mkostemp +mkstemp +mkstemps crypto/md2 crypto/md4 crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512 +open quote quotearg recv diff --git a/src/http.c b/src/http.c index 56b8669..728ada7 100644 --- a/src/http.c +++ b/src/http.c @@ -39,6 +39,7 @@ as that of the covered work. */ #include <errno.h> #include <time.h> #include <locale.h> +#include <fcntl.h> #include "hash.h" #include "http.h" @@ -1568,6 +1569,8 @@ struct http_stat #ifdef HAVE_METALINK metalink_t *metalink; #endif + FILE *tmpfp; /* set for temp. files, not closed bertween + * download & parsing */ }; static void @@ -2423,6 +2426,27 @@ check_auth (struct url *u, char *user, char *passwd, struct response *resp, return auth_err; } +static FILE * +fopen_tmp (char **fname) +{ + FILE *fp; + char *orig = *fname; + + *fname = xstrdup("wget_XXXXXX.part"); + fp = fdopen (mkstemps (*fname, 5), "wb"); + + if (!fp) + { + xfree(*fname); + *fname = orig; + fp = fopen (*fname, "wb"); + } + else + xfree(orig); + + return fp; +} + static uerr_t open_output_stream (struct http_stat *hs, int count, FILE **fp) { @@ -2471,7 +2495,17 @@ open_output_stream (struct http_stat *hs, int count, FILE **fp) open_id = 22; *fp = fopen (hs->local_file, "wb", FOPEN_OPT_ARGS); #else /* def __VMS */ - *fp = fopen (hs->local_file, "wb"); + if (opt.delete_after + || opt.spider /* opt.recursive is implicitely true */ + || !acceptable (hs->local_file)) + { + *fp = fopen_tmp(&hs->local_file); + } + else + { + *fp = fopen (hs->local_file, "wb"); + } + #endif /* def __VMS [else] */ } else -- 2.9.3
signature.asc
Description: This is a digitally signed message part.
