request for review of wget patch (CVE-2010-2252)

[Tim Zingelman <tez%netbsd.org@localhost>]

I've made an attempt at patching the CVE-2010-2252 issue in wget
v1.12.  Currently the only published patches are against v1.11.4
rather than v1.12 which is in pkgsrc.

I'd very much appreciate a review... in particular of the patches to
retr.c which had changed significantly between 1.11.4 and 1.12, and
where the proposed patch includes what seems to me to be a mis-worded
comment.
The original 1.11.4 patch is at
http://www.openwall.com/lists/oss-security/2010/05/17/2  Tested this
build against the first testcase in
https://bugzilla.redhat.com/show_bug.cgi?id=602797 and got good
results.

Thanks,  - Tim

File:patches/patch-aa
$NetBSD$
fix for CVE-2010-2252 - patch for v1.12 based on v1.11.4 patch found at
http://www.openwall.com/lists/oss-security/2010/05/17/2

--- doc/wget.texi.orig  2010-08-09 18:18:44.042737600 -0500
+++ doc/wget.texi
@@ -1487,6 +1487,17 @@ This option is useful for some file-down
 @code{Content-Disposition} headers to describe what the name of a
 downloaded file should be.

+@...dex redirects
+@...dex HTTP redirects
+@...dex file name generation
+@...m --use-server-file-name
+
+If this is set to on, the file name provided from the server is used.
+(The server might return a different name using HTTP redirects.)  It is
+recommended to use this option for backwards compatibility only because
+server-provided file names can be unpredictable and lead to unexpected
+results.
+
 @cindex authentication
 @item --auth-no-challenge



File:patches/patch-ab
$NetBSD$
fix for CVE-2010-2252 - patch for v1.12 based on v1.11.4 patch found at
http://www.openwall.com/lists/oss-security/2010/05/17/2
(also fix for apparently harmless unterminated string from same source)

--- src/http.c.orig     2010-08-09 17:12:00.761007000 -0500
+++ src/http.c
@@ -1385,7 +1385,7 @@ free_hstat (struct http_stat *hs)
    server, and u->url will be requested.  */
 static uerr_t
 gethttp (struct url *u, struct http_stat *hs, int *dt, struct url *proxy,
-         struct iri *iri)
+         struct iri *iri, struct url *original_u)
 {
   struct request *req;

@@ -1444,6 +1444,8 @@ gethttp (struct url *u, struct http_stat

   bool host_lookup_failed = false;

+  assert(original_u != 0);
+
 #ifdef HAVE_SSL
   if (u->scheme == SCHEME_HTTPS)
     {
@@ -1989,7 +1991,7 @@ gethttp (struct url *u, struct http_stat
         {
           /* The Content-Disposition header is missing or broken.
            * Choose unique file name according to given URL. */
-          hs->local_file = url_file_name (u);
+          hs->local_file = url_file_name (original_u);
         }
     }

@@ -2411,7 +2413,7 @@ File %s already there; not retrieving.\n
    retried, and retried, and retried, and...  */
 uerr_t
 http_loop (struct url *u, char **newloc, char **local_file, const
char *referer,
-           int *dt, struct url *proxy, struct iri *iri)
+           int *dt, struct url *proxy, struct iri *iri, struct url *original_u)
 {
   int count;
   bool got_head = false;         /* used for time-stamping and
filename detection */
@@ -2429,6 +2431,8 @@ http_loop (struct url *u, char **newloc,
   /* Assert that no value for *LOCAL_FILE was passed. */
   assert (local_file == NULL || *local_file == NULL);

+  assert(original_u != 0);
+
   /* Set LOCAL_FILE parameter. */
   if (local_file && opt.output_document)
     *local_file = HYPHENP (opt.output_document) ? NULL : xstrdup
(opt.output_document);
@@ -2457,7 +2461,7 @@ http_loop (struct url *u, char **newloc,
     }
   else if (!opt.content_disposition)
     {
-      hstat.local_file = url_file_name (u);
+      hstat.local_file = url_file_name (original_u);
       got_name = true;
     }

@@ -2497,7 +2501,7 @@ File %s already there; not retrieving.\n

   /* Send preliminary HEAD request if -N is given and we have an existing
    * destination file. */
-  file_name = url_file_name (u);
+  file_name = url_file_name (original_u);
   if (opt.timestamping
       && !opt.content_disposition
       && file_exists_p (file_name))
@@ -2578,7 +2582,7 @@ Spider mode enabled. Check if remote fil
         *dt &= ~SEND_NOCACHE;

       /* Try fetching the document, or at least its head.  */
-      err = gethttp (u, &hstat, dt, proxy, iri);
+      err = gethttp (u, &hstat, dt, proxy, iri, original_u);

       /* Time?  */
       tms = datetime_str (time (NULL));
@@ -3031,7 +3035,7 @@ http_atotm (const char *time_string)
       if (l >= sizeof savedlocale)
         savedlocale[0] = '\0';
       else
-        memcpy (savedlocale, oldlocale, l);
+        memcpy (savedlocale, oldlocale, l+1);
     }
   else savedlocale[0] = '\0';



File:patches/patch-ac
$NetBSD$
fix for CVE-2010-2252 - patch for v1.12 based on v1.11.4 patch found at
http://www.openwall.com/lists/oss-security/2010/05/17/2

--- src/http.h.orig     2010-08-09 17:53:19.021265600 -0500
+++ src/http.h
@@ -34,7 +34,7 @@ as that of the covered work.  */
 struct url;

 uerr_t http_loop (struct url *, char **, char **, const char *, int *,
-                 struct url *, struct iri *);
+                 struct url *, struct iri *, struct url *);
 void save_cookies (void);
 void http_cleanup (void);
 time_t http_atotm (const char *);


File:patches/patch-ad
$NetBSD$
fix for CVE-2010-2252 - patch for v1.12 based on v1.11.4 patch found at
http://www.openwall.com/lists/oss-security/2010/05/17/2

--- src/init.c.orig     2010-08-09 17:22:02.962052000 -0500
+++ src/init.c
@@ -246,6 +246,7 @@ static const struct {
   { "useproxy",         &opt.use_proxy,         cmd_boolean },
   { "user",             &opt.user,              cmd_string },
   { "useragent",        NULL,                   cmd_spec_useragent },
+  { "useserverfilename", &opt.use_server_file_name, cmd_boolean },
   { "verbose",          NULL,                   cmd_spec_verbose },
   { "wait",             &opt.wait,              cmd_time },
   { "waitretry",        &opt.waitretry,         cmd_time },


File:patches/patch-ae
$NetBSD$
fix for CVE-2010-2252 - patch for v1.12 based on v1.11.4 patch found at
http://www.openwall.com/lists/oss-security/2010/05/17/2

--- src/main.c.orig     2010-08-09 17:23:29.817819000 -0500
+++ src/main.c
@@ -266,6 +266,7 @@ static struct cmdline_option option_data
     { "timeout", 'T', OPT_VALUE, "timeout", -1 },
     { "timestamping", 'N', OPT_BOOLEAN, "timestamping", -1 },
     { "tries", 't', OPT_VALUE, "tries", -1 },
+    { "use-server-file-name", 0, OPT_BOOLEAN, "useserverfilename", -1 },
     { "user", 0, OPT_VALUE, "user", -1 },
     { "user-agent", 'U', OPT_VALUE, "useragent", -1 },
     { "verbose", 'v', OPT_BOOLEAN, "verbose", -1 },


File:patches/patch-af
$NetBSD$
fix for CVE-2010-2252 - patch for v1.12 based on v1.11.4 patch found at
http://www.openwall.com/lists/oss-security/2010/05/17/2

--- src/options.h.orig  2010-08-09 17:24:17.174302100 -0500
+++ src/options.h
@@ -238,6 +238,7 @@ struct options
   bool auth_without_challenge;  /* Issue Basic authentication creds without
                                    waiting for a challenge. */

+  bool use_server_file_name;   /* Use server-provided file name. */
   bool enable_iri;
   char *encoding_remote;
   char *locale;


File:patches/patch-ag
$NetBSD$
fix for CVE-2010-2252 - patch for v1.12 based on v1.11.4 patch found at
http://www.openwall.com/lists/oss-security/2010/05/17/2

--- src/retr.c.orig     2010-08-09 17:54:43.594894300 -0500
+++ src/retr.c
@@ -689,7 +689,11 @@ retrieve_url (struct url * orig_parsed,
 #endif
       || (proxy_url && proxy_url->scheme == SCHEME_HTTP))
     {
-      result = http_loop (u, &mynewloc, &local_file, refurl, dt,
proxy_url, iri);
+      /* Pass original URL as final argument, used to generate local file name
+         unless useserverfilename has been enabled.  Redirection
might otherwise
+         lead to unexpected file names.  See CVE-2010-2252 */
+      result = http_loop (u, &mynewloc, &local_file, refurl, dt,
proxy_url, iri,
+       opt.use_server_file_name ? u : orig_parsed);
     }
   else if (u->scheme == SCHEME_FTP)
     {


File: Makefile
@@ -1,6 +1,7 @@
 # $NetBSD: Makefile,v 1.100 2009/09/14 12:06:12 tron Exp $

 DISTNAME=      wget-1.12
+PKGREVISION=   1
 CATEGORIES=    net
 MASTER_SITES=  ${MASTER_SITE_GNU:=wget/}


File: distinfo
@@ -3,3 +3,10 @@
 SHA1 (wget-1.12.tar.gz) = 50d4ed2441e67db7aa5061d8a4dde41ee0e94248
 RMD160 (wget-1.12.tar.gz) = 232d0aa6fb36731c162d2b7374aa9ab59e671b7d
 Size (wget-1.12.tar.gz) = 2464747 bytes
+SHA1 (patch-aa) = f9fb3615b8509ecbfb584cda09d4711840dba845
+SHA1 (patch-ab) = 71363689d9ab4153a2e1e7ba95910438ee3f6925
+SHA1 (patch-ac) = a72e5e3a67d2681207eb507352049328957792f6
+SHA1 (patch-ad) = 21c7886bad4fd2b479c639636834cc9b5ab420b4
+SHA1 (patch-ae) = c9cd08de86ab4b52b624aae0de09b0b892270791
+SHA1 (patch-af) = 2b71f7e84b43d90ec9f76be0ed90569e8fc6d8ae
+SHA1 (patch-ag) = 87897b53bcc9747176919c4def43622c90270f44

###