Re: [Bug-wget] Wget - acess list bypass / race condition PoC

[Giuseppe Scrivano]

Hi,

"Misra, Deapesh" <address@hidden> 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?= <address@hidden>
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" <address@hidden>
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 <errno.h>
 #include <time.h>
 #include <locale.h>
+#include <fcntl.h>
 
 #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 <address@hidden>
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" <address@hidden>
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 <address@hidden>.
 
+* 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 = 22;
           *fp = fopen (hs->local_file, "wb", FOPEN_OPT_ARGS);
 #else /* def __VMS */
-          if (opt.delete_after
-            || opt.spider /* opt.recursive is implicitely true */
-            || !acceptable (hs->local_file))
+          if (hs->temporary)
             {
               *fp = fdopen (open (hs->local_file, O_CREAT | O_TRUNC | 
O_WRONLY, S_IRUSR | S_IWUSR), "wb");
             }
-- 
2.7.4

Regards,
Giuseppe