Bug 428659 – Setting SSL param to 'authenticated sessions' only protects logins and param

doesn't protect WebService calls at all
Patch by David Lawrence <dkl@redhat.com> - r/a=LpSolit/mkanat

4 files changed, 84 insertions, 27 deletions

diff --git a/Bugzilla/Auth/Login/CGI.pm b/Bugzilla/Auth/Login/CGI.pm
index 980e27123..9e008be82 100644
--- a/Bugzilla/Auth/Login/CGI.pm
+++ b/Bugzilla/Auth/Login/CGI.pm
@@ -65,12 +65,17 @@ sub fail_nodata {
             ->faultstring('Login Required');
     }
 
-    # Redirect to SSL if required
-    if (Bugzilla->params->{'sslbase'} ne '' 
-        and Bugzilla->params->{'ssl'} ne 'never') 
+    # If system is not configured to never require SSL connections
+    # we want to always redirect to SSL since passing usernames and
+    # passwords over an unprotected connection is a bad idea. If we 
+    # get here then a login form will be provided to the user so we
+    # want this to be protected if possible.
+    if ($cgi->protocol ne 'https' && Bugzilla->params->{'sslbase'} ne ''
+        && Bugzilla->params->{'ssl'} ne 'never')
     {
         $cgi->require_https(Bugzilla->params->{'sslbase'});
     }
+
     print $cgi->header();
     $template->process("account/auth/login.html.tmpl",
                        { 'target' => $cgi->url(-relative=>1) }) 
diff --git a/Bugzilla/CGI.pm b/Bugzilla/CGI.pm
index aeb8419ca..e6238f334 100644
--- a/Bugzilla/CGI.pm
+++ b/Bugzilla/CGI.pm
@@ -71,14 +71,6 @@ sub new {
     # Send appropriate charset
     $self->charset(Bugzilla->params->{'utf8'} ? 'UTF-8' : '');
 
-    # Redirect to SSL if required
-    if (Bugzilla->params->{'sslbase'} ne ''
-        && Bugzilla->params->{'ssl'} eq 'always'
-        && i_am_cgi())
-    {
-        $self->require_https(Bugzilla->params->{'sslbase'});
-    }
-
     # Check for errors
     # All of the Bugzilla code wants to do this, so do it here instead of
     # in each script
@@ -297,18 +289,23 @@ sub remove_cookie {
 
 # Redirect to https if required
 sub require_https {
-    my $self = shift;
-    if ($self->protocol ne 'https') {
-        my $url = shift;
-        if (defined $url) {
-            $url .= $self->url('-path_info' => 1, '-query' => 1, '-relative' => 1);
-        } else {
-            $url = $self->self_url;
-            $url =~ s/^http:/https:/i;
-        }
-        print $self->redirect(-location => $url);
-        exit;
+    my ($self, $url) = @_;
+    # Do not create query string if data submitted via XMLRPC
+    # since we want the data to be resubmitted over POST method.
+    my $query = Bugzilla->usage_mode == USAGE_MODE_WEBSERVICE ? 0 : 1;
+    # XMLRPC clients (SOAP::Lite at least) requires 301 to redirect properly
+    # and do not work with 302.
+    my $status = Bugzilla->usage_mode == USAGE_MODE_WEBSERVICE ? 301 : 302;
+    if (defined $url) {
+        $url .= $self->url('-path_info' => 1, '-query' => $query, '-relative' => 1);
+    } else {
+        $url = $self->self_url;
+        $url =~ s/^http:/https:/i;
     }
+    print $self->redirect(-location => $url, -status => $status);
+    # When using XML-RPC with mod_perl, we need the headers sent immediately.
+    $self->r->rflush if $ENV{MOD_PERL};
+    exit;
 }
 
 1;
@@ -375,10 +372,10 @@ As its only argument, it takes the name of the cookie to expire.
 
 =item C<require_https($baseurl)>
 
-This routine checks if the current page is being served over https, and
-redirects to the https protocol if required, retaining QUERY_STRING.
+This routine redirects the client to a different location using the https protocol. 
+If the client is using XMLRPC, it will not retain the QUERY_STRING since XMLRPC uses POST.
 
-It takes an option argument which will be used as the base URL.  If $baseurl
+It takes an optional argument which will be used as the base URL.  If $baseurl
 is not provided, the current URL is used.
 
 =back
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index e7a76e21d..1e7dbf8d1 100644
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -36,7 +36,7 @@ use base qw(Exporter);
                              html_quote url_quote xml_quote
                              css_class_quote html_light_quote url_decode
                              i_am_cgi get_netaddr correct_urlbase
-                             lsearch
+                             lsearch ssl_require_redirect
                              diff_arrays diff_strings
                              trim wrap_hard wrap_comment find_wrap_point
                              format_time format_time_decimal validate_date
@@ -218,6 +218,46 @@ sub i_am_cgi {
     return exists $ENV{'SERVER_SOFTWARE'} ? 1 : 0;
 }
 
+sub ssl_require_redirect {
+    my $method = shift;
+
+    # If currently not in a protected SSL 
+    # connection, determine if a redirection is 
+    # needed based on value in Bugzilla->params->{ssl}.
+    # If we are already in a protected connection or
+    # sslbase is not set then no action is required.
+    if (uc($ENV{'HTTPS'}) ne 'ON' 
+        && $ENV{'SERVER_PORT'} != 443 
+        && Bugzilla->params->{'sslbase'} ne '')
+    {
+        # System is configured to never require SSL 
+        # so no redirection is needed.
+        return 0 
+            if Bugzilla->params->{'ssl'} eq 'never';
+            
+        # System is configured to always require a SSL
+        # connection so we need to redirect.
+        return 1
+            if Bugzilla->params->{'ssl'} eq 'always';
+
+        # System is configured such that if we are inside
+        # of an authenticated session, then we need to make
+        # sure that all of the connections are over SSL. Non
+        # authenticated sessions SSL is not mandatory.
+        # For XMLRPC requests, if the method is User.login
+        # then we always want the connection to be over SSL
+        # if the system is configured for authenticated
+        # sessions since the user's username and password
+        # will be passed before the user is logged in.
+        return 1 
+            if Bugzilla->params->{'ssl'} eq 'authenticated sessions'
+                && (Bugzilla->user->id 
+                    || (defined $method && $method eq 'User.login'));
+    }
+
+    return 0;
+}
+
 sub correct_urlbase {
     my $ssl = Bugzilla->params->{'ssl'};
     return Bugzilla->params->{'urlbase'} if $ssl eq 'never';
diff --git a/Bugzilla/WebService.pm b/Bugzilla/WebService.pm
index 7812a237b..d1502468d 100755
--- a/Bugzilla/WebService.pm
+++ b/Bugzilla/WebService.pm
@@ -19,6 +19,7 @@ package Bugzilla::WebService;
 
 use strict;
 use Bugzilla::WebService::Constants;
+use Bugzilla::Util;
 use Date::Parse;
 use XMLRPC::Lite;
 
@@ -49,7 +50,21 @@ sub handle_login {
     eval "require $class";
 
     return if $class->login_exempt($method);
-    Bugzilla->login;
+    Bugzilla->login();
+
+    # Even though we check for the need to redirect in
+    # Bugzilla->login() we check here again since Bugzilla->login()
+    # does not know what the current XMLRPC method is. Therefore
+    # ssl_require_redirect in Bugzilla->login() will have returned 
+    # false if system was configured to redirect for authenticated 
+    # sessions and the user was not yet logged in.
+    # So here we pass in the method name to ssl_require_redirect so
+    # it can then check for the extra case where the method equals
+    # User.login, which we would then need to redirect if not
+    # over a secure connection. 
+    my $full_method = $uri . "." . $method;
+    Bugzilla->cgi->require_https(Bugzilla->params->{'sslbase'})
+        if ssl_require_redirect($full_method);
 
     return;
 }