From: lpsolit%gmail.com <>
Date: Mon, 2 Feb 2009 18:26:22 +0000 (+0000)
Subject: Bug 38862: [SECURITY] attachments should be at a different hostname - Patch by Byron... 
X-Git-Tag: bugzilla-3.0.7~7
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c848088119c015e520aad0e8870fb3fac41194f8;p=thirdparty%2Fbugzilla.git

Bug 38862: [SECURITY] attachments should be at a different hostname - Patch by Byron Jones <bugzilla@glob.com.au> and Frédéric Buclin <LpSolit@gmail.com> r=mkanat a=LpSolit
---

diff --git a/Bugzilla/CGI.pm b/Bugzilla/CGI.pm
index 7fe02049e4..4cf5e57e54 100644
--- a/Bugzilla/CGI.pm
+++ b/Bugzilla/CGI.pm
@@ -78,6 +78,19 @@ sub new {
     # Send appropriate charset
     $self->charset(Bugzilla->params->{'utf8'} ? 'UTF-8' : '');
 
+    # Redirect to urlbase/sslbase if we are not viewing an attachment.
+    # We do this check before the SSL redirect below to avoid two redirects.
+    if (use_attachbase() && i_am_cgi()) {
+        my $cgi_file = $self->url('-path_info' => 0, '-query' => 0, '-relative' => 1);
+        $cgi_file =~ s/\?$//;
+        my $urlbase = Bugzilla->params->{'urlbase'};
+        my $sslbase = Bugzilla->params->{'sslbase'};
+        my $path_regexp = $sslbase ? qr/^(\Q$urlbase\E|\Q$sslbase\E)/ : qr/^\Q$urlbase\E/;
+        if ($cgi_file ne 'attachment.cgi' && $self->self_url !~ /$path_regexp/) {
+            $self->redirect_to_urlbase;
+        }
+    }
+
     # Redirect to SSL if required
     if (Bugzilla->params->{'sslbase'} ne ''
         && Bugzilla->params->{'ssl'} eq 'always'
@@ -297,6 +310,14 @@ sub require_https {
     }
 }
 
+# Redirect to the urlbase version of the current URL.
+sub redirect_to_urlbase {
+    my $self = shift;
+    my $path = $self->url('-path_info' => 1, '-query' => 1, '-relative' => 1);
+    print $self->redirect('-location' => correct_urlbase() . $path);
+    exit;
+}
+
 1;
 
 __END__
@@ -367,6 +388,10 @@ redirects to the https protocol if required, retaining QUERY_STRING.
 It takes an option argument which will be used as the base URL.  If $baseurl
 is not provided, the current URL is used.
 
+=item C<redirect_to_urlbase>
+
+Redirects from the current URL to one prefixed by the urlbase parameter.
+
 =back
 
 =head1 SEE ALSO
diff --git a/Bugzilla/Config/Attachment.pm b/Bugzilla/Config/Attachment.pm
index bbaaaa63d4..d498157f9b 100644
--- a/Bugzilla/Config/Attachment.pm
+++ b/Bugzilla/Config/Attachment.pm
@@ -40,6 +40,13 @@ $Bugzilla::Config::Attachment::sortkey = "025";
 sub get_param_list {
   my $class = shift;
   my @param_list = (
+   {
+   name => 'attachment_base',
+   type => 't',
+   default => '',
+   checker => \&check_urlbase
+  },
+
   {
   name => 'allow_attachment_deletion',
   type => 'b',
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index e97bb11d0c..efc3220f92 100644
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -36,7 +36,7 @@ use base qw(Exporter);
                              html_quote url_quote value_quote xml_quote
                              css_class_quote html_light_quote url_decode
                              i_am_cgi get_netaddr correct_urlbase
-                             lsearch
+                             lsearch use_attachbase
                              diff_arrays diff_strings
                              trim wrap_comment find_wrap_point
                              perform_substs
@@ -251,6 +251,13 @@ sub correct_urlbase {
     return Bugzilla->params->{'urlbase'};
 }
 
+sub use_attachbase {
+    my $attachbase = Bugzilla->params->{'attachment_base'};
+    return ($attachbase ne ''
+            && $attachbase ne Bugzilla->params->{'urlbase'}
+            && $attachbase ne Bugzilla->params->{'sslbase'}) ? 1 : 0;
+}
+
 sub lsearch {
     my ($list,$item) = (@_);
     my $count = 0;
@@ -700,6 +707,11 @@ cookies) to only some addresses.
 Returns either the C<sslbase> or C<urlbase> parameter, depending on the
 current setting for the C<ssl> parameter.
 
+=item C<use_attachbase()>
+
+Returns true if an alternate host is used to display attachments; false
+otherwise.
+
 =back
 
 =head2 Searching
diff --git a/attachment.cgi b/attachment.cgi
index 66e2fdd015..25b73828e5 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -27,6 +27,7 @@
 #                 Greg Hendricks <ghendricks@novell.com>
 #                 Frédéric Buclin <LpSolit@gmail.com>
 #                 Marc Schumann <wurblzap@gmail.com>
+#                 Byron Jones <bugzilla@glob.com.au>
 
 ################################################################################
 # Script Initialization
@@ -50,8 +51,6 @@ use Bugzilla::Attachment;
 use Bugzilla::Attachment::PatchReader;
 use Bugzilla::Token;
 
-Bugzilla->login();
-
 # For most scripts we don't make $cgi and $template global variables. But
 # when preparing Bugzilla for mod_perl, this script used these
 # variables in so many subroutines that it was easier to just
@@ -72,7 +71,21 @@ local our $vars = {};
 # Determine whether to use the action specified by the user or the default.
 my $action = $cgi->param('action') || 'view';
 
-if ($action eq "view")  
+# You must use the appropriate urlbase/sslbase param when doing anything
+# but viewing an attachment.
+if ($action ne 'view') {
+    my $urlbase = Bugzilla->params->{'urlbase'};
+    my $sslbase = Bugzilla->params->{'sslbase'};
+    my $path_regexp = $sslbase ? qr/^(\Q$urlbase\E|\Q$sslbase\E)/ : qr/^\Q$urlbase\E/;
+    if (use_attachbase() && $cgi->self_url !~ /$path_regexp/) {
+        $cgi->redirect_to_urlbase;
+    }
+    Bugzilla->login();
+}
+
+# When viewing an attachment, do not request credentials if we are on
+# the alternate host. Let view() decide when to call Bugzilla->login.
+if ($action eq "view")
 {
     view();
 }
@@ -124,7 +137,8 @@ exit;
 # Validates an attachment ID. Optionally takes a parameter of a form
 # variable name that contains the ID to be validated. If not specified,
 # uses 'id'.
-# 
+# If the second parameter is true, the attachment ID will be validated,
+# however the current user's access to the attachment will not be checked.
 # Will throw an error if 1) attachment ID is not a valid number,
 # 2) attachment does not exist, or 3) user isn't allowed to access the
 # attachment.
@@ -133,17 +147,14 @@ exit;
 # attachment id, and the 2nd item is the bug id corresponding to the
 # attachment.
 # 
-sub validateID
-{
-    my $param = @_ ? $_[0] : 'id';
-    my $dbh = Bugzilla->dbh;
-    my $user = Bugzilla->user;
+sub validateID {
+    my($param, $dont_validate_access) = @_;
+    $param ||= 'id';
 
     # If we're not doing interdiffs, check if id wasn't specified and
     # prompt them with a page that allows them to choose an attachment.
     # Happens when calling plain attachment.cgi from the urlbar directly
     if ($param eq 'id' && !$cgi->param('id')) {
-
         print $cgi->header();
         $template->process("attachment/choose.html.tmpl", $vars) ||
             ThrowTemplateError($template->error());
@@ -159,22 +170,37 @@ sub validateID
      || ThrowUserError("invalid_attach_id", { attach_id => $cgi->param($param) });
   
     # Make sure the attachment exists in the database.
-    my ($bugid, $isprivate, $submitter_id) = $dbh->selectrow_array(
-                                    "SELECT bug_id, isprivate, submitter_id
-                                     FROM attachments 
-                                     WHERE attach_id = ?",
-                                     undef, $attach_id);
-    ThrowUserError("invalid_attach_id", { attach_id => $attach_id }) 
-        unless $bugid;
+    my $attachment = Bugzilla::Attachment->get($attach_id)
+      || ThrowUserError("invalid_attach_id", { attach_id => $attach_id });
+
+    return $attachment if ($dont_validate_access || check_can_access($attachment));
+}
+
+# Make sure the current user has access to the specified attachment.
+sub check_can_access {
+    my $attachment = shift;
+    my $user = Bugzilla->user;
 
     # Make sure the user is authorized to access this attachment's bug.
-    ValidateBugID($bugid);
-    if ($isprivate && $user->id != $submitter_id && !$user->is_insider) {
+    ValidateBugID($attachment->bug_id);
+    if ($attachment->isprivate && $user->id != $attachment->attacher->id
+        && !$user->is_insider) {
         ThrowUserError('auth_failure', {action => 'access',
                                         object => 'attachment'});
     }
+    return 1;
+}
+
+# Determines if the attachment is public -- that is, if users who are
+# not logged in have access to the attachment
+sub attachmentIsPublic {
+    my $attachment = shift;
 
-    return ($attach_id, $bugid);
+    return 0 if Bugzilla->params->{'requirelogin'};
+    return 0 if $attachment->isprivate;
+
+    my $anon_user = new Bugzilla::User;
+    return $anon_user->can_see_bug($attachment->bug_id);
 }
 
 # Validates format of a diff/interdiff. Takes a list as an parameter, which
@@ -288,17 +314,63 @@ sub isViewable
 ################################################################################
 
 # Display an attachment.
-sub view
-{
-    # Retrieve and validate parameters
-    my ($attach_id) = validateID();
-    my $dbh = Bugzilla->dbh;
-    
-    # Retrieve the attachment content and its content type from the database.
-    my ($contenttype, $filename, $thedata) = $dbh->selectrow_array(
-            "SELECT mimetype, filename, thedata FROM attachments " .
-            "INNER JOIN attach_data ON id = attach_id " .
-            "WHERE attach_id = ?", undef, $attach_id);
+sub view {
+    my $attachment;
+
+    if (use_attachbase()) {
+        $attachment = validateID(undef, 1);
+        # Replace %bugid% by the ID of the bug the attachment belongs to, if present.
+        my $attachbase = Bugzilla->params->{'attachment_base'};
+        my $bug_id = $attachment->bug_id;
+        $attachbase =~ s/%bugid%/$bug_id/;
+        my $path = 'attachment.cgi?id=' . $attachment->id;
+
+        # Make sure the attachment is served from the correct server.
+        if ($cgi->self_url !~ /^\Q$attachbase\E/) {
+            # We couldn't call Bugzilla->login earlier as we first had to make sure
+            # we were not going to request credentials on the alternate host.
+            Bugzilla->login();
+            if (attachmentIsPublic($attachment)) {
+                # No need for a token; redirect to attachment base.
+                print $cgi->redirect(-location => $attachbase . $path);
+                exit;
+            } else {
+                # Make sure the user can view the attachment.
+                check_can_access($attachment);
+                # Create a token and redirect.
+                my $token = url_quote(issue_session_token($attachment->id));
+                print $cgi->redirect(-location => $attachbase . "$path&t=$token");
+                exit;
+            }
+        } else {
+            # No need to validate the token for public attachments. We cannot request
+            # credentials as we are on the alternate host.
+            if (!attachmentIsPublic($attachment)) {
+                my $token = $cgi->param('t');
+                my ($userid, undef, $token_attach_id) = Bugzilla::Token::GetTokenData($token);
+                unless ($userid
+                        && detaint_natural($token_attach_id)
+                        && ($token_attach_id == $attachment->id))
+                {
+                    # Not a valid token.
+                    print $cgi->redirect('-location' => correct_urlbase() . $path);
+                    exit;
+                }
+                # Change current user without creating cookies.
+                Bugzilla->set_user(new Bugzilla::User($userid));
+                # Tokens are single use only, delete it.
+                delete_token($token);
+            }
+        }
+    } else {
+        # No alternate host is used. Request credentials if required.
+        Bugzilla->login();
+        $attachment = validateID();
+    }
+
+    # At this point, Bugzilla->login has been called if it had to.
+    my $contenttype = $attachment->contenttype;
+    my $filename = $attachment->filename;
    
     # Bug 111522: allow overriding content-type manually in the posted form
     # params.
@@ -311,69 +383,37 @@ sub view
     }
 
     # Return the appropriate HTTP response headers.
-    $filename =~ s/^.*[\/\\]//;
-    my $filesize = length($thedata);
-    # A zero length attachment in the database means the attachment is 
-    # stored in a local file
-    if ($filesize == 0)
-    {
-        my $hash = ($attach_id % 100) + 100;
-        $hash =~ s/.*(\d\d)$/group.$1/;
-        if (open(AH, bz_locations()->{'attachdir'} . "/$hash/attachment.$attach_id")) {
-            binmode AH;
-            $filesize = (stat(AH))[7];
-        }
-    }
-    if ($filesize == 0)
-    {
-        ThrowUserError("attachment_removed");
-    }
-
+    $attachment->datasize || ThrowUserError("attachment_removed");
 
+    $filename =~ s/^.*[\/\\]//;
     # escape quotes and backslashes in the filename, per RFCs 2045/822
     $filename =~ s/\\/\\\\/g; # escape backslashes
     $filename =~ s/"/\\"/g; # escape quotes
 
     print $cgi->header(-type=>"$contenttype; name=\"$filename\"",
                        -content_disposition=> "inline; filename=\"$filename\"",
-                       -content_length => $filesize);
-
-    if ($thedata) {
-        print $thedata;
-    } else {
-        while (<AH>) {
-            print $_;
-        }
-        close(AH);
-    }
+                       -content_length => $attachment->datasize);
 
+    print $attachment->data;
 }
 
 sub interdiff {
     # Retrieve and validate parameters
-    my ($old_id) = validateID('oldid');
-    my ($new_id) = validateID('newid');
+    my $old_attachment = validateID('oldid');
+    my $new_attachment = validateID('newid');
     my $format = validateFormat('html', 'raw');
     my $context = validateContext();
 
-    # XXX - validateID should be replaced by Attachment::check_attachment()
-    # and should return an attachment object. This would save us a lot of
-    # trouble.
-    my $old_attachment = Bugzilla::Attachment->get($old_id);
-    my $new_attachment = Bugzilla::Attachment->get($new_id);
-
     Bugzilla::Attachment::PatchReader::process_interdiff(
         $old_attachment, $new_attachment, $format, $context);
 }
 
 sub diff {
     # Retrieve and validate parameters
-    my ($attach_id) = validateID();
+    my $attachment = validateID();
     my $format = validateFormat('html', 'raw');
     my $context = validateContext();
 
-    my $attachment = Bugzilla::Attachment->get($attach_id);
-
     # If it is not a patch, view normally.
     if (!$attachment->ispatch) {
         view();
@@ -538,10 +578,8 @@ sub insert
 # is private and the user does not belong to the insider group.
 # Validations are done later when the user submits changes.
 sub edit {
-  my ($attach_id) = validateID();
-  my $dbh = Bugzilla->dbh;
+  my $attachment = validateID();
 
-  my $attachment = Bugzilla::Attachment->get($attach_id);
   my $isviewable = !$attachment->isurl && isViewable($attachment->contenttype);
 
   # Retrieve a list of attachments for this bug as well as a summary of the bug
@@ -551,6 +589,7 @@ sub edit {
   # We only want attachment IDs.
   @$bugattachments = map { $_->id } @$bugattachments;
 
+  my $dbh = Bugzilla->dbh;
   my ($bugsummary, $product_id, $component_id) =
       $dbh->selectrow_array('SELECT short_desc, product_id, component_id
                                FROM bugs
@@ -596,9 +635,11 @@ sub update
 
     # Retrieve and validate parameters
     ValidateComment(scalar $cgi->param('comment'));
-    my ($attach_id, $bugid) = validateID();
+    my $attachment = validateID();
+    my $attach_id = $attachment->id;
+    my $bugid = $attachment->bug_id;
     my $bug = new Bugzilla::Bug($bugid);
-    my $attachment = Bugzilla::Attachment->get($attach_id);
+
     $attachment->validate_can_edit($bug->product_id);
     validateCanChangeAttachment($attach_id);
     Bugzilla::Attachment->validate_description(THROW_ERROR);
@@ -774,8 +815,9 @@ sub delete_attachment {
       || ThrowUserError('attachment_deletion_disabled');
 
     # Make sure the administrator is allowed to edit this attachment.
-    my ($attach_id, $bug_id) = validateID();
-    my $attachment = Bugzilla::Attachment->get($attach_id);
+    my $attachment = validateID();
+    my $attach_id = $attachment->id;
+    my $bug_id = $attachment->bug_id;
     validateCanChangeAttachment($attach_id);
 
     $attachment->datasize || ThrowUserError('attachment_removed');
diff --git a/template/en/default/admin/params/attachment.html.tmpl b/template/en/default/admin/params/attachment.html.tmpl
index 785d91822e..721177429e 100644
--- a/template/en/default/admin/params/attachment.html.tmpl
+++ b/template/en/default/admin/params/attachment.html.tmpl
@@ -25,6 +25,24 @@
 %]
 
 [% 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_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 1a65068261..a48cd2e1de 100644
--- a/template/en/default/attachment/edit.html.tmpl
+++ b/template/en/default/attachment/edit.html.tmpl
@@ -38,6 +38,9 @@
   subheader = subheader
 %]
 
+[%# No need to display the Diff button and iframe if the attachment is not a patch. %]
+[% patchviewerinstalled = (patchviewerinstalled && attachment.ispatch) %]
+
 <script type="text/javascript">
   <!--
   var prev_mode = 'raw';
@@ -46,37 +49,7 @@
   var has_viewed_as_diff = 0;
   function editAsComment()
     {
-      // Get the content of the document as a string.
-      var viewFrame = document.getElementById('viewFrame');
-      var aSerializer = new XMLSerializer();
-      var contentDocument = viewFrame.contentDocument;
-      var theContent = aSerializer.serializeToString(contentDocument);
-
-      // If this is a plaintext document, remove cruft that Mozilla adds
-      // because it treats it as an HTML document with a big PRE section.
-      // http://bugzilla.mozilla.org/show_bug.cgi?id=86012
-      var contentType = '[% attachment.contenttype FILTER js %]';
-      if ( contentType == 'text/plain' )
-        {
-          theContent = theContent.replace( /^<html><head\/?><body><pre>/i , "" );
-          theContent = theContent.replace( /<\/pre><\/body><\/html>$/i , "" );
-          theContent = theContent.replace( /&lt;/gi , "<" );
-          theContent = theContent.replace( /&gt;/gi , ">" );
-          theContent = theContent.replace( /&amp;/gi , "&" );
-        }
-
-      // Add mail-style quote indicators (>) to the beginning of each line.
-      // ".*\n" matches lines that end with a newline, while ".+" matches
-      // the rare situation in which the last line of a file does not end
-      // with a newline.
-      theContent = theContent.replace( /(.*\n|.+)/g , ">$1" );
-
       switchToMode('edit');
-
-      // Copy the contents of the diff into the textarea
-      var editFrame = document.getElementById('editFrame');
-      editFrame.value = theContent + "\n\n";
-
       has_edited = 1;
     }
   function undoEditAsComment()
@@ -290,6 +263,8 @@
             minrows = 10
             cols    = 80
             wrap    = 'soft'
+            defaultcontent = (attachment.contenttype.match('^text\/')) ?
+                               attachment.data.replace('(.*\n|.+)', '>$1') : undef
           %]
           <iframe id="viewFrame" src="attachment.cgi?id=[% attachment.id %]" style="height: 400px; width: 100%;">
             <b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.