Re: [Bug-wget] Wget - acess list bypass / race condition PoC
couldn't you just use whatever secure mkstmp is appropriate for the language wget is written in http://rosettacode.org/wiki/Secure_temporary_file so much easier and secure then faffing about. On Wed, Aug 17, 2016 at 7:40 AM, 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 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 leas
[Bug-wget] How remove all after ? or @ (REAL PROBLEM)
file on server http://site.com/style.css?v1000 downloaded file style.css@v1000 How remove @v1000 I want result: style.css (without @v1000) My OS win10, wget version 1.18
Re: [Bug-wget] Wget tests
wor...@alum.mit.edu (Dale R. Worley) writes: > Tim Rühsen writes: >> Somethin went wrong... try again: > > I will investigate why this happened. Ugh. I had checked out a very old version of wget that didn't have Makefile.am, etc. After I fixed that, I discovered that older versions of Git do not handle submodules correctly if the path in $PWD uses symbolic links. So I've retrieved a newer version of Git to be able to work on wget. And there is at least one significant error in testenv/Test-Proto.py... Dale
Re: [Bug-wget] Wget - acess list bypass / race condition PoC
Hi Eli,
Eli Zaretskii writes:
>> From: Giuseppe Scrivano
>> Date: Sun, 21 Aug 2016 15:26:58 +0200
>> Cc: "bug-wget@gnu.org" ,
>> Dawid Golunski ,
>> "kseifr...@redhat.com"
>>
>> #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 = fdopen (open (hs->local_file, O_CREAT | O_TRUNC |
>> O_WRONLY, S_IRUSR | S_IWUSR), "wb");
>> +}
>
> For this to work on MS-Windows, the 'open' call should use O_BINARY,
> in addition to the other flags. Otherwise, the "b" in "wb" of
> 'fdopen' will be ignored by the MS runtime.
thanks for the review, I am going to amend it to the patch.
Giuseppe
[Bug-wget] buildbot success in OpenCSW Buildbot on wget-solaris10-sparc
The Buildbot has detected a restored build on builder wget-solaris10-sparc while building wget. Full details are available at: https://buildfarm.opencsw.org/buildbot/builders/wget-solaris10-sparc/builds/174 Buildbot URL: https://buildfarm.opencsw.org/buildbot/ Buildslave for this Build: unstable10s Build Reason: The web-page 'rebuild' button was pressed by 'dam ': Build Source Stamp: [branch master] 0787d7253edff7d808c47d36e10766e8adda2100 Blamelist: Tim Rühsen Build succeeded! sincerely, -The Buildbot
Re: [Bug-wget] buildbot failure in OpenCSW Buildbot on wget-solaris10-i386
Hi Tim, Am 17.08.2016 um 21:52 schrieb Tim Rühsen : > Please update libpsl. Looks like you have a rather outdated version. Sure, this lead to a pull request ragrding libicu setup :-) New libpsl is pushed to OpenCSW unstable, same for new public-suffix-list (I take https://publicsuffix.org/list/public_suffix_list.dat instead of the one from Mozilla, is there a difference?) The new package is installed on the buildfarm and the wget build looks better. Best regards — Dago > On Mittwoch, 17. August 2016 21:20:45 CEST build...@opencsw.org wrote: >> The Buildbot has detected a new failure on builder wget-solaris10-i386 while >> building wget. Full details are available at: >> https://buildfarm.opencsw.org/buildbot/builders/wget-solaris10-i386/builds/ >> 167 >> >> Buildbot URL: https://buildfarm.opencsw.org/buildbot/ >> >> Buildslave for this Build: unstable10x >> >> Build Reason: scheduler >> Build Source Stamp: [branch master] 262baeb11379a2765507455569d16abfa94947c0 >> Blamelist: Tim Rühsen >> >> BUILD FAILED: failed shell_2 shell_3 >> >> sincerely, >> -The Buildbot > -- "You don't become great by trying to be great, you become great by wanting to do something, and then doing it so hard that you become great in the process." - xkcd #896 signature.asc Description: Message signed with OpenPGP using GPGMail
Re: [Bug-wget] Wget - acess list bypass / race condition PoC
> From: Giuseppe Scrivano
> Date: Sun, 21 Aug 2016 15:26:58 +0200
> Cc: "bug-wget@gnu.org" ,
> Dawid Golunski ,
> "kseifr...@redhat.com"
>
> #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 = fdopen (open (hs->local_file, O_CREAT | O_TRUNC |
> O_WRONLY, S_IRUSR | S_IWUSR), "wb");
> +}
For this to work on MS-Windows, the 'open' call should use O_BINARY,
in addition to the other flags. Otherwise, the "b" in "wb" of
'fdopen' will be ignored by the MS runtime.
Thanks.
[Bug-wget] buildbot success in OpenCSW Buildbot on wget-solaris10-i386
The Buildbot has detected a restored build on builder wget-solaris10-i386 while building wget. Full details are available at: https://buildfarm.opencsw.org/buildbot/builders/wget-solaris10-i386/builds/169 Buildbot URL: https://buildfarm.opencsw.org/buildbot/ Buildslave for this Build: unstable10x Build Reason: The web-page 'rebuild' button was pressed by 'dam ': Build Source Stamp: [branch master] 0787d7253edff7d808c47d36e10766e8adda2100 Blamelist: Tim Rühsen Build succeeded! sincerely, -The Buildbot
Re: [Bug-wget] Wget - acess list bypass / race condition PoC
Hi,
"Misra, Deapesh" writes:
> Yes - I whole heartedly agree with the following:
>
>>
>> To cite myself :)
>> "But there is also non-obvious wget behavior in creating those (temp) files
>> in
>> the filesystem."
>>
>> The Wget docs just don't make clear that these files come into existence for
>> a
>> while. Of course we could amend the docs and lean back... but it still is
>> not
>> an intuitive behavior and I fear people might trap into that pit. And we
>> could
>> easily prevent it with some lines of code...
>>
>> Regards, Tim
>
> Although I am late to this thread, I would like to elucidate the basic issue
> I had with the current scenario with an analogy:
>
> If I assign a guard to a room and order the guard not to allow (say)
> people wearing yellow shirts, I intuitively expect that the people
> with yellow shirts will be prevented from entering the room and not
> that everyone will be allowed into the room and then the yellow
> shirted people will be asked to leave.
>
> When I had thought about the possible solutions, I had thought of storing the
> files in a temporary location. But you guys (developers) are on the right
> track with all your solutions and the ensuing discussion, IMHO.
If nobody is going to complain, I will push these patches shortly:
>From 1904fbfb40d817075a809124cb03640b9b7234df Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20R=C3=BChsen?=
Date: Sun, 14 Aug 2016 21:04:58 +0200
Subject: [PATCH 1/2] Limit file mode to u=rw on temp. downloaded files
* bootstrap.conf: Add gnulib modules fopen, open.
* src/http.c (open_output_stream): Limit file mode to u=rw
on temporary downloaded files.
Reported-by: "Misra, Deapesh"
Discovered by: Dawid Golunski (http://legalhackers.com)
---
bootstrap.conf | 2 ++
src/http.c | 13 -
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/bootstrap.conf b/bootstrap.conf
index 2b225b7..d9a5f90 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -40,6 +40,7 @@ dirname
fcntl
flock
fnmatch
+fopen
futimens
ftello
getaddrinfo
@@ -71,6 +72,7 @@ crypto/md5
crypto/sha1
crypto/sha256
crypto/sha512
+open
quote
quotearg
recv
diff --git a/src/http.c b/src/http.c
index 56b8669..d463f29 100644
--- a/src/http.c
+++ b/src/http.c
@@ -39,6 +39,7 @@ as that of the covered work. */
#include
#include
#include
+#include
#include "hash.h"
#include "http.h"
@@ -2471,7 +2472,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 = fdopen (open (hs->local_file, O_CREAT | O_TRUNC | O_WRONLY, S_IRUSR | S_IWUSR), "wb");
+}
+ else
+{
+ *fp = fopen (hs->local_file, "wb");
+}
+
#endif /* def __VMS [else] */
}
else
--
2.7.4
>From 7013f8260fc0a2b71f2414593aedb45c77972d98 Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano
Date: Sun, 21 Aug 2016 15:21:44 +0200
Subject: [PATCH 2/2] Append .tmp to temporary files
* src/http.c (struct http_stat): Add `temporary` flag.
(check_file_output): Append .tmp to temporary files.
(open_output_stream): Refactor condition to use hs->temporary instead.
Reported-by: "Misra, Deapesh"
Discovered by: Dawid Golunski (http://legalhackers.com)
---
NEWS | 6 ++
src/http.c | 14 +++---
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/NEWS b/NEWS
index 5073d7e..56c21a5 100644
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,12 @@ See the end for copying conditions.
Please send GNU Wget bug reports to .
+* Changes in Wget X.Y.Z
+
+* On a recursive download, append a .tmp suffix to temporary files
+ that will be deleted after being parsed, and create them
+ readable/writable only by the owner.
+
* Changes in Wget 1.18
* By default, on server redirects to a FTP resource, use the original
diff --git a/src/http.c b/src/http.c
index d463f29..43bc2cb 100644
--- a/src/http.c
+++ b/src/http.c
@@ -1569,6 +1569,7 @@ struct http_stat
#ifdef HAVE_METALINK
metalink_t *metalink;
#endif
+ bool temporary; /* downloading a temporary file */
};
static void
@@ -2259,6 +2260,15 @@ check_file_output (struct url *u, struct http_stat *hs,
xfree (local_file);
}
+ hs->temporary = opt.delete_after || opt.spider || !acceptable (hs->local_file);
+ if (hs->temporary)
+{
+ char *tmp = NULL;
+ asprintf (&tmp, "%s.tmp", hs->local_file);
+ xfree (hs->local_file);
+ hs->local_file = tmp;
+}
+
/* TODO: perform this check only once. */
if (!hs->existence_checked && file_exists_p (hs->local_file))
{
@@ -2472,9 +2482,7 @@ open_output_stream (struct http_stat *hs, int count, FILE **fp)
open_id
