Bug 268146: mod_security complain: Invalid cookie format: Cookie value is…

Bugzilla/Auth/Login/WWW/CGI.pm

@@ -232,12 +232,8 @@ sub logout {
 
 sub clear_browser_cookies {
     my $cgi = Bugzilla->cgi;
-    $cgi->send_cookie(-name => "Bugzilla_login",
-                      -value => "",
-                      -expires => "Tue, 15-Sep-1998 21:49:00 GMT");
-    $cgi->send_cookie(-name => "Bugzilla_logincookie",
-                      -value => "",
-                      -expires => "Tue, 15-Sep-1998 21:49:00 GMT");
+    $cgi->remove_cookie('Bugzilla_login');
+    $cgi->remove_cookie('Bugzilla_logincookie');
 }
 
 1;

Bugzilla/CGI.pm

@@ -19,6 +19,7 @@
 #
 # Contributor(s): Bradley Baetz <bbaetz@student.usyd.edu.au>
 #                 Byron Jones <bugzilla@glob.com.au>
+#                 Marc Schumann <wurblzap@gmail.com>
 
 use strict;
 
@@ -28,6 +29,7 @@ use CGI qw(-no_xhtml -oldstyle_urls :private_tempfiles :unique_headers SERVER_PU
 
 use base qw(CGI);
 
+use Bugzilla::Error;
 use Bugzilla::Util;
 use Bugzilla::Config;
 
@@ -177,21 +179,42 @@ sub multipart_start {
 sub send_cookie {
     my $self = shift;
 
-    # Add the default path in
-    unshift(@_, '-path' => Param('cookiepath'));
-    if (Param('cookiedomain'))
-    {
-        unshift(@_, '-domain' => Param('cookiedomain'));
+    # Move the param list into a hash for easier handling.
+    my %paramhash;
+    my @paramlist;
+    my ($key, $value);
+    while ($key = shift) {
+        $value = shift;
+        $paramhash{$key} = $value;
     }
 
-    # Use CGI::Cookie directly, because CGI.pm's |cookie| method gives the
-    # current value if there isn't a -value attribute, which happens when
-    # we're expiring an entry.
-    require CGI::Cookie;
-    my $cookie = CGI::Cookie->new(@_);
-    push @{$self->{Bugzilla_cookie_list}}, $cookie;
+    # Complain if -value is not given or empty (bug 268146).
+    if (!exists($paramhash{'-value'}) || !$paramhash{'-value'}) {
+        ThrowCodeError('cookies_need_value');
+    }
+
+    # Add the default path and the domain in.
+    $paramhash{'-path'} = Param('cookiepath');
+    $paramhash{'-domain'} = Param('cookiedomain') if Param('cookiedomain');
+
+    # Move the param list back into an array for the call to cookie().
+    foreach (keys(%paramhash)) {
+        unshift(@paramlist, $_ => $paramhash{$_});
+    }
+
+    push(@{$self->{'Bugzilla_cookie_list'}}, $self->cookie(@paramlist));
+}
 
-    return;
+# Cookies are removed by setting an expiry date in the past.
+# This method is a send_cookie wrapper doing exactly this.
+sub remove_cookie {
+    my $self = shift;
+    my ($cookiename) = (@_);
+
+    # Expire the cookie, giving a non-empty dummy value (bug 268146).
+    $self->send_cookie('-name'    => $cookiename,
+                       '-expires' => 'Tue, 15-Sep-1998 21:49:00 GMT',
+                       '-value'   => 'X');
 }
 
 # Redirect to https if required
@@ -256,11 +279,21 @@ Values in C<@exclude> are not included in the result.
 
 =item C<send_cookie>
 
-This routine is identical to CGI.pm's C<cookie> routine, except that the cookie
-is sent to the browser, rather than returned. This should be used by all
-Bugzilla code (instead of C<cookie> or the C<-cookie> argument to C<header>),
-so that under mod_perl the headers can be sent correctly, using C<print> or
-the mod_perl APIs as appropriate.
+This routine is identical to the cookie generation part of CGI.pm's C<cookie>
+routine, except that it knows about Bugzilla's cookie_path and cookie_domain
+parameters and takes them into account if necessary.
+This should be used by all Bugzilla code (instead of C<cookie> or the C<-cookie>
+argument to C<header>), so that under mod_perl the headers can be sent
+correctly, using C<print> or the mod_perl APIs as appropriate.
+
+To remove (expire) a cookie, use C<remove_cookie>.
+
+=item C<remove_cookie>
+
+This is a wrapper around send_cookie, setting an expiry date in the past,
+effectively removing the cookie.
+
+As its only argument, it takes the name of the cookie to expire.
 
 =item C<require_https($baseurl)>
 

buglist.cgi

@@ -707,8 +707,7 @@ if ($order) {
                 else {
                     my $vars = { fragment => $fragment };
                     if ($order_from_cookie) {
-                        $cgi->send_cookie(-name => 'LASTORDER',
-                                          -expires => 'Tue, 15-Sep-1998 21:49:00 GMT');
+                        $cgi->remove_cookie('LASTORDER');
                         ThrowCodeError("invalid_column_name_cookie", $vars);
                     }
                     else {
@@ -1020,8 +1019,7 @@ if ($format->{'extension'} eq "html") {
                           -expires => 'Fri, 01-Jan-2038 00:00:00 GMT');
     }
     else {
-        $cgi->send_cookie(-name => 'BUGLIST',
-                          -expires => 'Tue, 15-Sep-1998 21:49:00 GMT');
+        $cgi->remove_cookie('BUGLIST');
         $vars->{'toolong'} = 1;
     }
 

colchange.cgi

@@ -97,7 +97,7 @@ if (defined $cgi->param('rememberedquery')) {
             }
         }
         if (defined $cgi->param('splitheader')) {
-            $splitheader = $cgi->param('splitheader');
+            $splitheader = $cgi->param('splitheader')? 1: 0;
         }
     }
     my $list = join(" ", @collist);
@@ -106,9 +106,14 @@ if (defined $cgi->param('rememberedquery')) {
     $cgi->send_cookie(-name => 'COLUMNLIST',
                       -value => $list,
                       -expires => 'Fri, 01-Jan-2038 00:00:00 GMT');
-    $cgi->send_cookie(-name => 'SPLITHEADER',
-                      -value => $cgi->param('splitheader'),
-                      -expires => 'Fri, 01-Jan-2038 00:00:00 GMT');
+    if ($splitheader) {
+        $cgi->send_cookie(-name => 'SPLITHEADER',
+                          -value => $splitheader,
+                          -expires => 'Fri, 01-Jan-2038 00:00:00 GMT');
+    }
+    else {
+        $cgi->remove_cookie('SPLITHEADER');
+    }
 
     $vars->{'message'} = "change_columns";
     $vars->{'redirect_url'} = "buglist.cgi?".$cgi->param('rememberedquery');

query.cgi

@@ -100,8 +100,7 @@ if ($userid) {
                 }
                 $dbh->bz_unlock_tables();
             }
-            $cgi->send_cookie(-name => $cookiename,
-                              -expires => "Fri, 01-Jan-2038 00:00:00 GMT");
+            $cgi->remove_cookie($cookiename);
         }
     }
 }

template/en/default/global/code-error.html.tmpl

@@ -86,6 +86,9 @@
     Charts will not work without the Chart::Lines Perl module being installed.
     Run checksetup.pl for installation instructions.
 
+  [% ELSIF error == "cookies_need_value" %]
+    Every cookie must have a value.
+
   [% ELSIF error == "field_type_mismatch" %]
     Cannot seem to handle <code>[% field FILTER html %]</code>
     and <code>[% type FILTER html %]</code> together.