From: reed%reedloden.com <>
Date: Mon, 30 Mar 2009 21:03:43 +0000 (+0000)
Subject: Bug 476603 - "[SECURITY] Editing attachments doesn't have any CSRF protection" [p... 
X-Git-Tag: bugzilla-3.2.3~1
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=44ebde00f3f48f91d0aae31f6d9b89838edaf79d;p=thirdparty%2Fbugzilla.git

Bug 476603 - "[SECURITY] Editing attachments doesn't have any CSRF protection" [p=reed r=LpSolit a=LpSolit]
---

diff --git a/attachment.cgi b/attachment.cgi
index a0f14c185c..951e995eac 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -585,6 +585,9 @@ sub update {
         ($vars->{'operations'}) =
             Bugzilla::Bug::GetBugActivity($bug->id, $attachment->id, $cgi->param('delta_ts'));
 
+        # The token contains the old modification_time. We need a new one.
+        $cgi->param('token', issue_hash_token([$attachment->id, $attachment->modification_time]));
+
         # If the modification date changed but there is no entry in
         # the activity table, this means someone commented only.
         # In this case, there is no reason to midair.
@@ -599,6 +602,12 @@ sub update {
             exit;
         }
     }
+
+    # We couldn't do this check earlier as we first had to validate attachment ID
+    # and display the mid-air collision page if modification_time changed.
+    my $token = $cgi->param('token');
+    check_hash_token($token, [$attachment->id, $attachment->modification_time]);
+
     # If the submitter of the attachment is not in the insidergroup,
     # be sure that he cannot overwrite the private bit.
     # This check must be done before calling Bugzilla::Flag*::validate(),
diff --git a/template/en/default/attachment/edit.html.tmpl b/template/en/default/attachment/edit.html.tmpl
index ca0a8bc6e1..1b00df9994 100644
--- a/template/en/default/attachment/edit.html.tmpl
+++ b/template/en/default/attachment/edit.html.tmpl
@@ -171,6 +171,9 @@
   <input type="hidden" name="action" value="update">
   <input type="hidden" name="contenttypemethod" value="manual">
   <input type="hidden" name="delta_ts" value="[% attachment.modification_time FILTER html %]">
+  [% IF user.id %]
+    <input type="hidden" name="token" value="[% issue_hash_token([attachment.id, attachment.modification_time]) FILTER html %]">
+  [% END %]
 
   <table class="attachment_info" width="100%">
 
diff --git a/template/en/default/bug/show.xml.tmpl b/template/en/default/bug/show.xml.tmpl
index bd74592714..c59b2bed03 100644
--- a/template/en/default/bug/show.xml.tmpl
+++ b/template/en/default/bug/show.xml.tmpl
@@ -99,9 +99,13 @@
             <type>[% a.contenttype FILTER xml %]</type>
             <size>[% a.datasize FILTER xml %]</size>
             <attacher>[% a.attacher.email FILTER xml %]</attacher>
-        [% IF displayfields.attachmentdata %]
-            <data encoding="base64">[% a.data FILTER base64 %]</data>
-        [% END %]        
+            [%# This is here so automated clients can still use attachment.cgi %]
+            [% IF displayfields.token && user.id %]
+              <token>[% issue_hash_token([a.id, a.modification_time]) FILTER xml %]</token>
+            [% END %]
+            [% IF displayfields.attachmentdata %]
+              <data encoding="base64">[% a.data FILTER base64 %]</data>
+            [% END %]        
 
             [% FOREACH flag = a.flags %]
               <flag name="[% flag.type.name FILTER xml %]"