Bug 472206: [SECURITY] Bugzilla should optionally not allow the user to view possibly harmful attachments - Patch by Frédéric Buclin <LpSolit@gmail.com> r=mkanat a=LpSolit

5 files changed, 69 insertions, 33 deletions

diff --git a/Bugzilla/Config/Attachment.pm b/Bugzilla/Config/Attachment.pm
index d498157f9..15ba2672a 100644
--- a/Bugzilla/Config/Attachment.pm
+++ b/Bugzilla/Config/Attachment.pm
@@ -40,7 +40,13 @@ $Bugzilla::Config::Attachment::sortkey = "025";
 sub get_param_list {
   my $class = shift;
   my @param_list = (
-   {
+  {
+   name => 'allow_attachment_display',
+   type => 'b',
+   default => 0
+  },
+
+  {
    name => 'attachment_base',
    type => 't',
    default => '',
diff --git a/attachment.cgi b/attachment.cgi
index 25b73828e..53fe4a6d0 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -390,8 +390,10 @@ sub view {
     $filename =~ s/\\/\\\\/g; # escape backslashes
     $filename =~ s/"/\\"/g; # escape quotes
 
+    my $disposition = Bugzilla->params->{'allow_attachment_display'} ? 'inline' : 'attachment';
+
     print $cgi->header(-type=>"$contenttype; name=\"$filename\"",
-                       -content_disposition=> "inline; filename=\"$filename\"",
+                       -content_disposition=> "$disposition; filename=\"$filename\"",
                        -content_length => $attachment->datasize);
 
     print $attachment->data;
diff --git a/template/en/default/admin/params/attachment.html.tmpl b/template/en/default/admin/params/attachment.html.tmpl
index 721177429..50c6ad606 100644
--- a/template/en/default/admin/params/attachment.html.tmpl
+++ b/template/en/default/admin/params/attachment.html.tmpl
@@ -25,23 +25,38 @@
 %]
 
 [% param_descs = {
-  attachment_base => "It is possible for a malicious attachment to steal your " _
-                     "cookies or access other attachments to perform an attack " _
-                     "on the user.<p>" _
-                     "If you would like additional security on attachments " _
-                     "to avoid this, set this parameter to an alternate URL " _
-                     "for your $terms.Bugzilla that is not the same as " _
-                     "<tt>urlbase</tt> or <tt>sslbase</tt>. That is, a different " _
-                     "domain name that resolves to this exact same $terms.Bugzilla " _
-                     "installation.<p>" _
-                     "For added security, you can insert <tt>%bugid%</tt> into " _
-                     "the URL, which will be replaced with the ID of the current " _
-                     "$terms.bug that the attachment is on, when you access " _
-                     "an attachment. This will limit attachments to accessing " _
-                     "only other attachments on the same ${terms.bug}. " _
-                     "Remember, though, that all those possible domain names " _
-                     "(such as <tt>1234.your.domain.com</tt>) must point to " _
-                     "this same $terms.Bugzilla instance."
+  allow_attachment_display =>
+    "If this option is on, users will be able to view attachments from"
+    _ " their browser, if their browser supports the attachment's MIME type."
+    _ " If this option is off, users are forced to download attachments,"
+    _ " even if the browser is able to display them."
+    _ "<p>This is a security restriction for installations where untrusted"
+    _ " users may upload attachments that could be potentially damaging if"
+    _ " viewed directly in the browser.</p>"
+    _ "<p>It is highly recommended that you set the <tt>attachment_base</tt>"
+    _ " parameter if you turn this parameter on.",
+
+  attachment_base => 
+    "When the <tt>allow_attachment_display</tt> parameter is on, it is "
+    _ " possible for a malicious attachment to steal your cookies or"
+    _ " perform an attack on $terms.Bugzilla using your credentials."
+    _ "<p>If you would like additional security on attachments to avoid"
+    _ " this, set this parameter to an alternate URL for your $terms.Bugzilla"
+    _ " that is not the same as <tt>urlbase</tt> or <tt>sslbase</tt>."
+    _ " That is, a different domain name that resolves to this exact"
+    _ " same $terms.Bugzilla installation.</p>"
+    _ "<p>Note that if you have set the"
+    _ " <a href=\"editparams.cgi?section=core#cookiedomain\"><tt>cookiedomain</tt>"
+    _" parameter</a>, you should set <tt>attachment_base</tt> to use a"
+    _ " domain that would <em>not</em> be matched by"
+    _ " <tt>cookiedomain</tt>.</p>"
+    _ "<p>For added security, you can insert <tt>%bugid%</tt> into the URL,"
+    _ " which will be replaced with the ID of the current $terms.bug that"
+    _ " the attachment is on, when you access an attachment. This will limit"
+    _ " attachments to accessing only other attachments on the same"
+    _ " ${terms.bug}. Remember, though, that all those possible domain names "
+    _ " (such as <tt>1234.your.domain.com</tt>) must point to this same"
+    _ " $terms.Bugzilla instance.",
 
   allow_attachment_deletion => "If this option is on, administrators will be able to delete " _
                                "the content of attachments.",
diff --git a/template/en/default/attachment/edit.html.tmpl b/template/en/default/attachment/edit.html.tmpl
index a48cd2e1d..5606504f9 100644
--- a/template/en/default/attachment/edit.html.tmpl
+++ b/template/en/default/attachment/edit.html.tmpl
@@ -254,6 +254,29 @@
 
       [% IF !attachment.datasize %]
         <td width="75%"><b>The content of this attachment has been deleted.</b></td>
+      [% ELSIF attachment.isurl %]
+        <td width="75%">
+          <a href="[% attachment.data FILTER html %]">
+            [% IF attachment.datasize < 120 %]
+              [% attachment.data FILTER html %]
+            [% ELSE %]
+              [% attachment.data FILTER truncate(80) FILTER html %]
+              &nbsp;...
+              [% attachment.data.match(".*(.{20})$").0 FILTER html %]
+            [% END %]
+          </a>
+        </td>
+      [% ELSIF !Param("allow_attachment_display") %]
+        <td id="view_disabled" width="50%">
+          <p><b>
+            The attachment is not viewable in your browser due to security
+            restrictions enabled by [% terms.Bugzilla %].
+          </b></p>
+          <p><b>
+            In order to view the attachment, you first have to
+            <a href="attachment.cgi?id=[% attachment.id %]">download it</a>.
+          </b></p>
+        </td>
       [% ELSIF isviewable %]
         <td width="75%">
           [% INCLUDE global/textarea.html.tmpl
@@ -287,18 +310,6 @@
             //-->
           </script>
         </td>
-      [% ELSIF attachment.isurl %]
-        <td width="75%">
-          <a href="[% attachment.data FILTER html %]">
-            [% IF attachment.datasize < 120 %]
-              [% attachment.data FILTER html %]
-            [% ELSE %]
-              [% attachment.data FILTER truncate(80) FILTER html %]
-              &nbsp;...
-              [% attachment.data.match(".*(.{20})$").0 FILTER html %]
-            [% END %]
-          </a>
-        </td>
       [% ELSE %]
         <td id="noview" width="50%">
           <p><b>
diff --git a/template/en/default/attachment/list.html.tmpl b/template/en/default/attachment/list.html.tmpl
index 99f51064d..6849b8ad5 100644
--- a/template/en/default/attachment/list.html.tmpl
+++ b/template/en/default/attachment/list.html.tmpl
@@ -135,9 +135,11 @@
       [% IF attachments.size %]
         <span class="bz_attach_view_hide">
           [% IF obsolete_attachments %]
-            <a href="#a0" onClick="return toggle_display(this);">Hide Obsolete</a> ([% obsolete_attachments %]) |
+            <a href="#a0" onClick="return toggle_display(this);">Hide Obsolete</a> ([% obsolete_attachments %])
+          [% END %]
+          [% IF Param("allow_attachment_display") %]
+            <a href="attachment.cgi?bugid=[% bugid %]&amp;action=viewall">View All</a>
           [% END %]
-          <a href="attachment.cgi?bugid=[% bugid %]&amp;action=viewall">View All</a>
         </span>
       [% END %]
       <a href="attachment.cgi?bugid=[% bugid %]&amp;action=enter">Add an attachment</a>