From e4600575bb4e2d2a6b9e2c543d0920b969d98e55 Mon Sep 17 00:00:00 2001 From: micah Date: Sun, 29 Jul 2007 17:29:05 -0700 Subject: [PATCH] [svn] Fix for bug #20299: Basic auth creds sent before challenge --- NEWS | 8 ++ src/ChangeLog | 18 ++++ src/http.c | 127 +++++++++++++++++--------- tests/ChangeLog | 16 ++++ tests/HTTPServer.pm | 190 ++++++++++++++++++++++++++++----------- tests/Makefile.in | 1 + tests/Test-auth-basic.px | 48 ++++++++++ 7 files changed, 315 insertions(+), 93 deletions(-) create mode 100755 tests/Test-auth-basic.px diff --git a/NEWS b/NEWS index 4d3a51b0..e5f6878d 100644 --- a/NEWS +++ b/NEWS @@ -7,6 +7,14 @@ Please send GNU Wget bug reports to . * 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. diff --git a/src/ChangeLog b/src/ChangeLog index 4dc66b1f..c576f117 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -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 + + * 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) : 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 * options.h: added maxredirect to options struct diff --git a/src/http.c b/src/http.c index 4813460f..af024bb9 100644 --- a/src/http.c +++ b/src/http.c @@ -40,6 +40,7 @@ so, delete this exception statement from your version. */ #include #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 */ diff --git a/tests/ChangeLog b/tests/ChangeLog index ed6a0e76..085f45be 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,19 @@ +2007-07-25 Micah Cowan + + * 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 * Makefile.in: diff --git a/tests/HTTPServer.pm b/tests/HTTPServer.pm index d29ea307..97e91396 100644 --- a/tests/HTTPServer.pm +++ b/tests/HTTPServer.pm @@ -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 diff --git a/tests/Makefile.in b/tests/Makefile.in index 3f7d3b62..ad244434 100644 --- a/tests/Makefile.in +++ b/tests/Makefile.in @@ -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 index 00000000..527d4200 --- /dev/null +++ b/tests/Test-auth-basic.px @@ -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 +