]> sjero.net Git - wget/commitdiff
[svn] Fix for bug #20299: Basic auth creds sent before challenge
authormicah <devnull@localhost>
Mon, 30 Jul 2007 00:29:05 +0000 (17:29 -0700)
committermicah <devnull@localhost>
Mon, 30 Jul 2007 00:29:05 +0000 (17:29 -0700)
NEWS
src/ChangeLog
src/http.c
tests/ChangeLog
tests/HTTPServer.pm
tests/Makefile.in
tests/Test-auth-basic.px [new file with mode: 0755]

diff --git a/NEWS b/NEWS
index 4d3a51b0785507cccf583ef303867a72014d6f11..e5f6878d563b90cee01c2ea7a1db02d21c082459 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,14 @@ Please send GNU Wget bug reports to <bug-wget@gnu.org>.
 \f
 * Changes in Wget 1.11.
 
+** No authentication credentials are sent until a challenge is issued,
+for improved security. Authentication handling is still not
+RFC-compliant, as once a Basic challenge has been received, it will
+assume it can send credentials to any URL at that same host, and not
+just the ones at or below the original authenticated location.
+Credentials for Digest authentication are still never saved or issued
+automatically, and continue to require a challenge for each resource.
+
 ** Wget now saves HTTP downloads using file names specified by the
 `Content-Disposition' header.  This is a standard way of specifying
 the file name used by many web dynamically generated pages.
index 4dc66b1f4f5c3eb130022c69a95ac9f9c48e7f4f..c576f1170a12ed43eb5fed6b3ce2e34d33ed0d05 100644 (file)
@@ -8,6 +8,24 @@
        opt.max_redirect's value should be checked a bit differently in
        retr.c, to allow for the "infinite" meaning of zero.
 
+2007-07-25  Micah Cowan  <micah@cowan.name>
+
+       * http.c (create_authorization_line)
+       (basic_authentication_encode, known_authentication_scheme_p)
+       (load_cookies): Moved declarations up.
+       (basic_authed_hosts): Added. Tracks what hosts have issued Basic
+       challenge and been given the global username, password.
+       (maybe_send_basic_creds): Added. Sends Basic creds for hosts that
+       have issued Basic challenges.
+       (register_basic_auth_host): Added. Instantiates
+       basic_authed_hosts if necessary, then registers the host that
+       has issued a challenge.
+       (gethttp) <auth>: Only send authentication credentials after
+       we've received a challenge from that host. This is a stop-gap
+       fix until a proper fix can be implemented; still isn't quite
+       right, as we should only be sending credentials automatically
+       for authenticated paths and below, and not for the entire host.
+
 2007-07-16  Joshua David Williams  <yurimxpxman@gmail.com>
 
        * options.h: added maxredirect to options struct
index 4813460f61baf36527a78083ddf36ea6e0c120eb..af024bb92a51d83429b4e5a73c1e388a08a376d9 100644 (file)
@@ -40,6 +40,7 @@ so, delete this exception statement from your version.  */
 #include <locale.h>
 
 #include "wget.h"
+#include "hash.h"
 #include "http.h"
 #include "utils.h"
 #include "url.h"
@@ -66,6 +67,14 @@ so, delete this exception statement from your version.  */
 
 extern char *version_string;
 
+/* Forward decls. */
+static char *create_authorization_line (const char *, const char *,
+                                        const char *, const char *,
+                                        const char *, bool *);
+static char *basic_authentication_encode (const char *, const char *);
+static bool known_authentication_scheme_p (const char *, const char *);
+static void load_cookies (void);
+
 #ifndef MIN
 # define MIN(x, y) ((x) > (y) ? (y) : (x))
 #endif
@@ -373,6 +382,50 @@ request_free (struct request *req)
   xfree (req);
 }
 
+static struct hash_table *basic_authed_hosts;
+
+/* Find out if this host has issued a Basic challenge yet; if so, give
+ * it the username, password. A temporary measure until we can get
+ * proper authentication in place. */
+
+static int
+maybe_send_basic_creds (const char *hostname, const char *user,
+                        const char *passwd, struct request *req)
+{
+  int did_challenge = 0;
+
+  if (basic_authed_hosts
+      && hash_table_contains(basic_authed_hosts, hostname))
+    {
+      DEBUGP(("Found `%s' in basic_authed_hosts.\n", hostname));
+      request_set_header (req, "Authorization",
+                          basic_authentication_encode (user, passwd),
+                          rel_value);
+      did_challenge = 1;
+    }
+  else
+    {
+      DEBUGP(("Host `%s' has not issued a general basic challenge.\n",
+              hostname));
+    }
+  return did_challenge;
+}
+
+static void
+register_basic_auth_host (const char *hostname)
+{
+  if (!basic_authed_hosts)
+    {
+      basic_authed_hosts = make_nocase_string_hash_table (1);
+    }
+  if (!hash_table_contains(basic_authed_hosts, hostname))
+    {
+      hash_table_put (basic_authed_hosts, xstrdup(hostname), NULL);
+      DEBUGP(("Inserted `%s' into basic_authed_hosts\n", hostname));
+    }
+}
+
+
 /* Send the contents of FILE_NAME to SOCK.  Make sure that exactly
    PROMISED_SIZE bytes are sent over the wire -- if the file is
    longer, read only that much; if the file is shorter, report an error.  */
@@ -1259,13 +1312,6 @@ free_hstat (struct http_stat *hs)
   hs->error = NULL;
 }
 
-static char *create_authorization_line (const char *, const char *,
-                                        const char *, const char *,
-                                        const char *, bool *);
-static char *basic_authentication_encode (const char *, const char *);
-static bool known_authentication_scheme_p (const char *, const char *);
-static void load_cookies (void);
-
 #define BEGINS_WITH(line, string_constant)                               \
   (!strncasecmp (line, string_constant, sizeof (string_constant) - 1)    \
    && (ISSPACE (line[sizeof (string_constant) - 1])                      \
@@ -1312,10 +1358,15 @@ gethttp (struct url *u, struct http_stat *hs, int *dt, struct url *proxy)
   int sock = -1;
   int flags;
 
-  /* Set to 1 when the authorization has failed permanently and should
+  /* Set to 1 when the authorization has already been sent and should
      not be tried again. */
   bool auth_finished = false;
 
+  /* Set to 1 when just globally-set Basic authorization has been sent;
+   * should prevent further Basic negotiations, but not other
+   * mechanisms. */
+  bool basic_auth_finished = false;
+
   /* Whether NTLM authentication is used for this request. */
   bool ntlm_seen = false;
 
@@ -1421,31 +1472,13 @@ gethttp (struct url *u, struct http_stat *hs, int *dt, struct url *proxy)
   user = user ? user : (opt.http_user ? opt.http_user : opt.user);
   passwd = passwd ? passwd : (opt.http_passwd ? opt.http_passwd : opt.passwd);
 
-  if (user && passwd)
+  if (user && passwd
+      && !u->user) /* We only do "site-wide" authentication with "global"
+                      user/password values; URL user/password info overrides. */
     {
-      /* We have the username and the password, but haven't tried
-         any authorization yet.  Let's see if the "Basic" method
-         works.  If not, we'll come back here and construct a
-         proper authorization method with the right challenges.
-
-         If we didn't employ this kind of logic, every URL that
-         requires authorization would have to be processed twice,
-         which is very suboptimal and generates a bunch of false
-         "unauthorized" errors in the server log.
-
-         #### But this logic also has a serious problem when used
-         with stronger authentications: we *first* transmit the
-         username and the password in clear text, and *then* attempt a
-         stronger authentication scheme.  That cannot be right!  We
-         are only fortunate that almost everyone still uses the
-         `Basic' scheme anyway.
-
-         There should be an option to prevent this from happening, for
-         those who use strong authentication schemes and value their
-         passwords.  */
-      request_set_header (req, "Authorization",
-                          basic_authentication_encode (user, passwd),
-                          rel_value);
+      /* If this is a host for which we've already received a Basic
+       * challenge, we'll go ahead and send Basic authentication creds. */
+      basic_auth_finished = maybe_send_basic_creds(u->host, user, passwd, req);
     }
 
   proxyauth = NULL;
@@ -1919,16 +1952,13 @@ File `%s' already there; not retrieving.\n\n"), hs->local_file);
               }
 
           if (!www_authenticate)
-            /* If the authentication header is missing or
-               unrecognized, there's no sense in retrying.  */
-            logputs (LOG_NOTQUIET, _("Unknown authentication scheme.\n"));
-          else if (BEGINS_WITH (www_authenticate, "Basic"))
-            /* If the authentication scheme is "Basic", which we send
-               by default, there's no sense in retrying either.  (This
-               should be changed when we stop sending "Basic" data by
-               default.)  */
-            ;
-          else
+            {
+              /* If the authentication header is missing or
+                 unrecognized, there's no sense in retrying.  */
+              logputs (LOG_NOTQUIET, _("Unknown authentication scheme.\n"));
+            }
+          else if (!basic_auth_finished
+                   || !BEGINS_WITH (www_authenticate, "Basic"))
             {
               char *pth;
               pth = url_full_path (u);
@@ -1941,9 +1971,20 @@ File `%s' already there; not retrieving.\n\n"), hs->local_file);
                                   rel_value);
               if (BEGINS_WITH (www_authenticate, "NTLM"))
                 ntlm_seen = true;
+              else if (!u->user && BEGINS_WITH (www_authenticate, "Basic"))
+                {
+                  /* Need to register this host as using basic auth,
+                   * so we automatically send creds next time. */
+                  register_basic_auth_host (u->host);
+                }
               xfree (pth);
               goto retry_with_auth;
             }
+          else
+            {
+              /* We already did Basic auth, and it failed. Gotta
+               * give up. */
+            }
         }
       logputs (LOG_NOTQUIET, _("Authorization failed.\n"));
       request_free (req);
@@ -3090,6 +3131,6 @@ test_parse_content_disposition()
 #endif /* TESTING */
 
 /*
- * vim: et ts=2 sw=2
+ * vim: et sts=2 sw=2 cino+={s
  */
 
index ed6a0e761dc54b40d3c751a0f32b0be36fad5a54..085f45be345bc36aad3036c9adbbe2fd6d9efea4 100644 (file)
@@ -1,3 +1,19 @@
+2007-07-25  Micah Cowan  <micah@cowan.name>
+
+       * HTTPServer.pm (run, send_response): Farmed out some logic from
+       the run method into a separate one named send_response, which
+       was then modified to handle simple authentication testing.
+       (handle_auth): Added to handle simple authentication testing.
+       (verify_auth_basic): Checks to make sure Basic credentials are
+       valid.
+       (verify_auth_digest): Stub added; always fails test.
+       * Makefile.in: Added Test-auth-basic.px to list of automatically
+       run tests.
+       * Test-auth-basic: Simple basic authentication test; mainly just
+       lets the server do its testing. Its current purpose is just to
+       ensure that correct basic creds are sent, but never until a
+       challenge has been sent.
+
 2007-07-05  Micah Cowan  <micah@cowan.name>
 
        * Makefile.in:
index d29ea307f5dcbc80cbdb1389be5cb6811d8efa16..97e91396dcf6e5b6f5ed0105e8cf1cd2fc1ba5ff 100644 (file)
@@ -24,7 +24,6 @@ sub run {
             $synch_callback->();
             $initialized = 1;
         }        
-                                
         my $con = $self->accept();
         print STDERR "Accepted a new connection\n" if $log;
         while (my $req = $con->get_request) {
@@ -47,55 +46,8 @@ sub run {
                 print STDERR "Serving requested URL: ", $url_path, "\n" if $log;
                 next unless ($req->method eq "HEAD" || $req->method eq "GET");
                 
-                # create response
-                my $tmp = $urls->{$url_path};
-                my $resp = HTTP::Response->new ($tmp->{code}, $tmp->{msg});
-                print STDERR "HTTP::Response: \n", $resp->as_string if $log;
-                
-                #if (is_dynamic_url) { # dynamic resource
-                #} else { # static resource
-                    # fill in headers
-                    while (my ($name, $value) = each %{$tmp->{headers}}) {
-                        # print STDERR "setting header: $name = $value\n";
-                        $resp->header($name => $value);
-                    }
-                    print STDERR "HTTP::Response with headers: \n", $resp->as_string if $log;
-                    
-                    if ($req->method eq "GET") {
-                        if (exists($tmp->{headers}{"Content-Length"})) {
-                            # Content-Length and length($tmp->{content}) don't match
-                            # manually prepare the HTTP response
-                            $con->send_basic_header($tmp->{code}, $resp->message, $resp->protocol);
-                            print $con $resp->headers_as_string($CRLF);
-                            print $con $CRLF;
-                            print $con $tmp->{content};
-                            next;
-                        }
-                        if ($req->header("Range")) {
-                            $req->header("Range") =~ m/bytes=(\d*)-(\d*)/;
-                            my $content_len = length($tmp->{content});
-                            my $start = $1 ? $1 : 0;
-                            my $end = $2 ? $2 : ($content_len - 1);
-                            my $len = $2 ? ($2 - $start) : ($content_len - $start);
-                            $resp->header("Accept-Ranges" => "bytes");
-                            $resp->header("Content-Length" => $len);
-                            $resp->header("Content-Range" => "bytes $start-$end/$content_len");
-                            $resp->header("Keep-Alive" => "timeout=15, max=100");
-                            $resp->header("Connection" => "Keep-Alive");
-                            $con->send_basic_header(206, "Partial Content", $resp->protocol);
-                            print $con $resp->headers_as_string($CRLF);
-                            print $con $CRLF;
-                            print $con substr($tmp->{content}, $start, $len);
-                            next;
-                        }
-                        # fill in content
-                        $resp->content($tmp->{content});
-                        print STDERR "HTTP::Response with content: \n", $resp->as_string if $log;
-                    }
-                #}
-                
-                $con->send_response($resp);
-                print STDERR "HTTP::Response sent: \n", $resp->as_string if $log;
+                my $url_rec = $urls->{$url_path};
+                $self->send_response($req, $url_rec, $con);
             } else {
                 print STDERR "Requested wrong URL: ", $url_path, "\n" if $log;
                 $con->send_error($HTTP::Status::RC_FORBIDDEN);
@@ -107,6 +59,144 @@ sub run {
     }
 }
 
+sub send_response {
+    my ($self, $req, $url_rec, $con) = @_;
+
+    # create response
+    my ($code, $msg, $headers);
+    my $send_content = ($req->method eq "GET");
+    if (exists $url_rec->{'auth_method'}) {
+        ($send_content, $code, $msg, $headers) =
+            $self->handle_auth($req, $url_rec);
+    } else {
+        ($code, $msg) = @{$url_rec}{'code', 'msg'};
+        $headers = $url_rec->{headers};
+    }
+    my $resp = HTTP::Response->new ($code, $msg);
+    print STDERR "HTTP::Response: \n", $resp->as_string if $log;
+
+    while (my ($name, $value) = each %{$headers}) {
+        # print STDERR "setting header: $name = $value\n";
+        $resp->header($name => $value);
+    }
+    print STDERR "HTTP::Response with headers: \n", $resp->as_string if $log;
+
+    if ($send_content) {
+        my $content = $url_rec->{content};
+        if (exists($url_rec->{headers}{"Content-Length"})) {
+            # Content-Length and length($content) don't match
+            # manually prepare the HTTP response
+            $con->send_basic_header($url_rec->{code}, $resp->message, $resp->protocol);
+            print $con $resp->headers_as_string($CRLF);
+            print $con $CRLF;
+            print $con $content;
+            next;
+        }
+        if ($req->header("Range")) {
+            $req->header("Range") =~ m/bytes=(\d*)-(\d*)/;
+            my $content_len = length($content);
+            my $start = $1 ? $1 : 0;
+            my $end = $2 ? $2 : ($content_len - 1);
+            my $len = $2 ? ($2 - $start) : ($content_len - $start);
+            $resp->header("Accept-Ranges" => "bytes");
+            $resp->header("Content-Length" => $len);
+            $resp->header("Content-Range" => "bytes $start-$end/$content_len");
+            $resp->header("Keep-Alive" => "timeout=15, max=100");
+            $resp->header("Connection" => "Keep-Alive");
+            $con->send_basic_header(206, "Partial Content", $resp->protocol);
+            print $con $resp->headers_as_string($CRLF);
+            print $con $CRLF;
+            print $con substr($content, $start, $len);
+            next;
+        }
+        # fill in content
+        $resp->content($content);
+        print STDERR "HTTP::Response with content: \n", $resp->as_string if $log;
+    }
+
+    $con->send_response($resp);
+    print STDERR "HTTP::Response sent: \n", $resp->as_string if $log;
+}
+
+# Generates appropriate response content based on the authentication
+# status of the URL.
+sub handle_auth {
+    my ($self, $req, $url_rec) = @_;
+    my ($send_content, $code, $msg, $headers);
+    # Catch failure to set code, msg:
+    $code = 500;
+    $msg  = "Didn't set response code in handle_auth";
+    # Most cases, we don't want to send content.
+    $send_content = 0;
+    # Initialize headers
+    $headers = {};
+    my $authhdr = $req->header('Authorization');
+
+    # Have we sent the challenge yet?
+    unless (defined $url_rec->{auth_challenged}
+        && $url_rec->{auth_challenged}) {
+        # Since we haven't challenged yet, we'd better not
+        # have received authentication (for our testing purposes).
+        if ($authhdr) {
+            $code = 400;
+            $msg  = "You sent auth before I sent challenge";
+        } else {
+            # Send challenge
+            $code = 401;
+            $msg  = "Authorization Required";
+            $headers->{'WWW-Authenticate'} = $url_rec->{'auth_method'}
+                . " realm=\"wget-test\"";
+            $url_rec->{auth_challenged} = 1;
+        }
+    } elsif (!defined($authhdr)) {
+        # We've sent the challenge; we should have received valid
+        # authentication with this one. A normal server would just
+        # resend the challenge; but since this is a test, wget just
+        # failed it.
+        $code = 400;
+        $msg  = "You didn't send auth after I sent challenge";
+    } else {
+        my ($sent_method) = ($authhdr =~ /^(\S+)/g);
+        unless ($sent_method eq $url_rec->{'auth_method'}) {
+            # Not the authorization type we were expecting.
+            $code = 400;
+            $msg = "Expected auth type $url_rec->{'auth_method'} but got "
+                . "$sent_method";
+        } elsif (($sent_method eq 'Digest'
+                  && &verify_auth_digest($authhdr, $url_rec, \$msg))
+                 ||
+                 ($sent_method eq 'Basic'
+                  && &verify_auth_basic($authhdr, $url_rec, \$msg))) {
+            # SUCCESSFUL AUTH: send expected message, headers, content.
+            ($code, $msg) = @{$url_rec}{'code', 'msg'};
+            $headers = $url_rec->{headers};
+            $send_content = 1;
+        } else {
+            $code = 400;
+        }
+    }
+
+    return ($send_content, $code, $msg, $headers);
+}
+
+sub verify_auth_digest {
+    return undef; # Not yet implemented.
+}
+
+sub verify_auth_basic {
+    require MIME::Base64;
+    my ($authhdr, $url_rec, $msgref) = @_;
+    my $expected = MIME::Base64::encode_base64($url_rec->{'user'} . ':'
+        . $url_rec->{'passwd'}, '');
+    my ($got) = $authhdr =~ /^Basic (.*)$/;
+    if ($got eq $expected) {
+        return 1;
+    } else {
+        $$msgref = "Wanted ${expected} got ${got}";
+        return undef;
+    }
+}
+
 1;
 
 # vim: et ts=4 sw=4
index 3f7d3b626e33643a5491fabc571ef4b552513efa..ad2444347c8999537ccf10bc02d427d54b2b8eac 100644 (file)
@@ -106,6 +106,7 @@ run-px-tests: WgetTest.pm
        ./Test--spider-fail.px && echo && echo
        ./Test--spider.px && echo && echo
        ./Test--spider-r.px && echo && echo
+       ./Test-auth-basic.px && echo && echo
 
 WgetTest.pm: WgetTest.pm.in @top_srcdir@/config.status
        cd @top_srcdir@ && ./config.status
diff --git a/tests/Test-auth-basic.px b/tests/Test-auth-basic.px
new file mode 100755 (executable)
index 0000000..527d420
--- /dev/null
@@ -0,0 +1,48 @@
+#!/usr/bin/perl -w
+
+use strict;
+
+use HTTPTest;
+
+
+###############################################################################
+
+my $wholefile = "You're all authenticated.\n";
+
+# code, msg, headers, content
+my %urls = (
+    '/needs-auth.txt' => {
+        auth_method => 'Basic',
+        user => 'fiddle-dee-dee',
+        passwd => 'Dodgson',
+        code => "200",
+        msg => "You want fries with that?",
+        headers => {
+            "Content-type" => "text/plain",
+        },
+        content => $wholefile,
+    },
+);
+
+my $cmdline = $WgetTest::WGETPATH . " --user=fiddle-dee-dee --password=Dodgson"
+    . " http://localhost:8080/needs-auth.txt";
+
+my $expected_error_code = 0;
+
+my %expected_downloaded_files = (
+    'needs-auth.txt' => {
+        content => $wholefile,
+    },
+);
+
+###############################################################################
+
+my $the_test = HTTPTest->new (name => "Test-auth-basic",
+                              input => \%urls, 
+                              cmdline => $cmdline, 
+                              errcode => $expected_error_code, 
+                              output => \%expected_downloaded_files);
+exit $the_test->run();
+
+# vim: et ts=4 sw=4
+