Bug 466692: [SECURITY] keywords and unused flag types can be deleted by bypassing the token check - Patch by Frédéric Buclin <LpSolit@gmail.com> r=mkanat a=LpSolit

5 files changed, 23 insertions, 32 deletions

diff --git a/editflagtypes.cgi b/editflagtypes.cgi
index d77c6b8a3..4dbaae573 100755
--- a/editflagtypes.cgi
+++ b/editflagtypes.cgi
@@ -80,7 +80,7 @@ elsif ($action eq 'edit')           { edit($action);    }
 elsif ($action eq 'insert')         { insert($token);   }
 elsif ($action eq 'update')         { update($token);   }
 elsif ($action eq 'confirmdelete')  { confirmDelete();  } 
-elsif ($action eq 'delete')         { deleteType(undef, $token); }
+elsif ($action eq 'delete')         { deleteType($token); }
 elsif ($action eq 'deactivate')     { deactivate($token); }
 else { 
     ThrowCodeError("action_unrecognized", { action => $action });
@@ -460,9 +460,8 @@ sub update {
 
 
 sub confirmDelete {
-  my $flag_type = validateID();
+    my $flag_type = validateID();
 
-  if ($flag_type->flag_count) {
     $vars->{'flag_type'} = $flag_type;
     $vars->{'token'} = issue_session_token('delete_flagtype');
     # Return the appropriate HTTP response headers.
@@ -471,20 +470,13 @@ sub confirmDelete {
     # Generate and return the UI (HTML page) from the appropriate template.
     $template->process("admin/flag-type/confirm-delete.html.tmpl", $vars)
       || ThrowTemplateError($template->error());
-  } 
-  else {
-    # We should *always* ask if the admin really wants to delete
-    # a flagtype, even if there is no flag belonging to this type.
-    my $token = issue_session_token('delete_flagtype');
-    deleteType($flag_type, $token);
-  }
 }
 
 
 sub deleteType {
-    my $flag_type = shift || validateID();
     my $token = shift;
     check_token_data($token, 'delete_flagtype');
+    my $flag_type = validateID();
     my $id = $flag_type->id;
     my $dbh = Bugzilla->dbh;
 
diff --git a/editkeywords.cgi b/editkeywords.cgi
index dbc92971d..76bba4817 100755
--- a/editkeywords.cgi
+++ b/editkeywords.cgi
@@ -151,26 +151,23 @@ if ($action eq 'update') {
     exit;
 }
 
-
-if ($action eq 'delete') {
+if ($action eq 'del') {
     my $keyword =  new Bugzilla::Keyword($key_id)
         || ThrowCodeError('invalid_keyword_id', { id => $key_id });
 
     $vars->{'keyword'} = $keyword;
+    $vars->{'token'} = issue_session_token('delete_keyword');
 
-    # We need this token even if there is no bug using this keyword.
-    $token = issue_session_token('delete_keyword');
-
-    if (!$cgi->param('reallydelete') && $keyword->bug_count) {
-        $vars->{'token'} = $token;
+    print $cgi->header();
+    $template->process("admin/keywords/confirm-delete.html.tmpl", $vars)
+      || ThrowTemplateError($template->error());
+    exit;
+}
 
-        print $cgi->header();
-        $template->process("admin/keywords/confirm-delete.html.tmpl", $vars)
-            || ThrowTemplateError($template->error());
-        exit;
-    }
-    # We cannot do this check earlier as we have to check 'reallydelete' first.
+if ($action eq 'delete') {
     check_token_data($token, 'delete_keyword');
+    my $keyword =  new Bugzilla::Keyword($key_id)
+        || ThrowCodeError('invalid_keyword_id', { id => $key_id });
 
     $dbh->do('DELETE FROM keywords WHERE keywordid = ?', undef, $keyword->id);
     $dbh->do('DELETE FROM keyworddefs WHERE id = ?', undef, $keyword->id);
diff --git a/template/en/default/admin/flag-type/confirm-delete.html.tmpl b/template/en/default/admin/flag-type/confirm-delete.html.tmpl
index cc6a064a9..ed909417d 100644
--- a/template/en/default/admin/flag-type/confirm-delete.html.tmpl
+++ b/template/en/default/admin/flag-type/confirm-delete.html.tmpl
@@ -28,13 +28,16 @@
 %]
 
 <p>
-   There are [% flag_type.flag_count %] flags of type [% flag_type.name FILTER html %].
-   If you delete this type, those flags will also be deleted.  Note that
-   instead of deleting the type you can
+  [% IF flag_type.flag_count %]
+    There are [% flag_type.flag_count %] flags of type [% flag_type.name FILTER html %].
+    If you delete this type, those flags will also be deleted.
+  [% END %]
+
+  Note that instead of deleting the type you can
    <a href="editflagtypes.cgi?action=deactivate&amp;id=[% flag_type.id %]&amp;token=
            [%- token FILTER html %]">deactivate it</a>,
-   in which case the type and its flags will remain in the database
-   but will not appear in the [% terms.Bugzilla %] UI.
+   in which case the type [% IF flag_type.flag_count %] and its flags [% END %] will remain
+   in the database but will not appear in the [% terms.Bugzilla %] UI.
 </p>
 
 <table>
diff --git a/template/en/default/admin/keywords/confirm-delete.html.tmpl b/template/en/default/admin/keywords/confirm-delete.html.tmpl
index 6bde05abf..20a6deee7 100755..100644
--- a/template/en/default/admin/keywords/confirm-delete.html.tmpl
+++ b/template/en/default/admin/keywords/confirm-delete.html.tmpl
@@ -31,7 +31,7 @@
 <p>
   [% IF keyword.bug_count == 1 %]
     There is one [% terms.bug %] with this keyword set.
-  [% ELSE %]
+  [% ELSIF keyword.bug_count > 1 %]
     There are [% keyword.bug_count FILTER html %] [%+ terms.bugs %] with
     this keyword set.
   [% END %]
@@ -43,7 +43,6 @@
 <form method="post" action="editkeywords.cgi">
   <input type="hidden" name="id" value="[% keyword.id FILTER html %]">
   <input type="hidden" name="action" value="delete">
-  <input type="hidden" name="reallydelete" value="1">
   <input type="hidden" name="token" value="[% token FILTER html %]">
   <input type="submit" id="delete"
          value="Yes, really delete the keyword">
diff --git a/template/en/default/admin/keywords/list.html.tmpl b/template/en/default/admin/keywords/list.html.tmpl
index 5fb6b3aa6..c400a2362 100755..100644
--- a/template/en/default/admin/keywords/list.html.tmpl
+++ b/template/en/default/admin/keywords/list.html.tmpl
@@ -54,7 +54,7 @@
      { 
        heading => "Action" 
        content => "Delete"
-       contentlink => "editkeywords.cgi?action=delete&amp;id=%%id%%"
+       contentlink => "editkeywords.cgi?action=del&amp;id=%%id%%"
      }
    ]
 %]