70189 - showattachment.cgi doesn't check viewing permissions

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Jacob Steenhagen]

One of the reasons listed for having bug security is the possibility that
proprietary code could be associated w/that bug.  There's a good chance that
this code could be in an attachment.  showattachment.cgi should thus check the
viewing permissions on a bug before spitting out the data.  If the idea I listed
in bug 39816 is used, this would be fixed by changing the SQL query to:

select mimetype, bug_id, thedata from attachments where attach_id =" .
SqlQuote($::FORM{'attach_id'})

Then adding something along the lines of:
if (!UserCanSeeBug($row[1]) {
    # Error message about not being able to view the attachment here
}

And, of course, changing the print line to:

print qq{Content-type: $row[0]\n\n$row[2]};

Comment 1

[Dave Miller [:justdave]]

moving to real milestones...

Comment 2

[Joe Robins]

As an alternative solution, what if we simply did the check in the select 
statement?  We could either do:

------
select mimetype, thedata
from attachments
left join bugs on bugs.bug_id = attachments.bug_id
where attach_id =".SqlQuote($::FORM{'attach_id'})."
and (bugs.groupset & $::usergroupset) = bugs.groupset
------

This would not retrieve any attachment which the user does not have permissions 
for.  (It also assumes the user is already logged in...  We'd probably need a 
confirm_login() or at least a quietly_check_login() in there to have the right 
value in $::usergroupset, right?)

Alternatively, if we want to be able to print a separate message for denied 
attachments, we could still do the whole check in one sql query:

------
select mimetype, thedata, ((bugs.groupset & $::usergroupset) = bugs.groupset)
from attachments
left join bugs on bugs.bug_id = attachments.bug_id
where attach_id =".SqlQuote($::FORM{'attach_id'})."

...
if($row[2] eq "0") {
  # Error message for permission denied here
}
------

Not a particularly ugly query, really, and it saves us having to make another 
database call to check the permissions afterwards.

Whatcha think?

Comment 3

[Jacob Steenhagen]

Attachment: validate info

Comment 4

[Jacob Steenhagen]

In order to maintain parity with other files in bugzilla, I opted to use the
ValidateBugID() routine.  The biggest downside I see with this method is that if
you're trying to see an attachment you don't have permission to view, it tells
in the error message what bug the file is attached to.  I don't think that's a
big deal being that attemping to view the bug it tells you about will result in
the same error, but thought I'd mention it.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

I suggest removing the call to &SqlQuote since the attachment ID is a number and
does not need quotes around it (and probably can't have them in some non-mysql
database servers).

I agree that &ValidateBugID gives a confusing error message in this situation,
and most of that function is also unnecessary since in this case we know that
the bug ID is valid and represents an existing bug.  I suggest copying into
showattachment.cgi just the part of that function that checks authorization and
then displaying a customized error message if the user is not authorized to
access the attachment.

Really minor nit: error messages that are complete sentences should have periods
at their end.

Comment 6

[Jacob Steenhagen]

The periods and SqlQuote make sence and will be in the next patch.  Where I keep
going in circles is ValidateBugID().  I originally opted to use that routine to
make things such as bug 39816 and bug 68022 easier to impliment (less places to
change the permission checking).  OTOH, Joe's suggestion about what query to use
so the permission and data retrieval is done with the same query also makes
sence (and, of course, would allow the error message to be specific to this file.

I think the next patch I upload will again use ValidateBugID().  It won't have
anything from bug 38862, but I think the changes in showattachment.cgi in that
patch will be easy enough to merge in.  If there's another reason I'm missing
that supports one method over the other, it may help solidify this in my mind...

Comment 7

[Jacob Steenhagen]

Attachment: Patch v2

Comment 8

[Dave Miller [:justdave]]

setting owner to match patch author

Comment 9

[Tara Hernandez]

this seems reasonable to me, and i also support keeping the consistancy of
validatebugid for the tiome being.  also,we should probably go ahead and get it
in so myk can merge to it for his next pass at 38862.

r=tara

Comment 10

[Jacob Steenhagen]

OK, it's in.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

A quick note for the record in case my suggestion about not putting quotes
around the attachment ID starts to propagate through the rest of the code:

Non-validated non-quoted values in SQL statements present a theoretical security
risk in situations where the database interface allows multiple SQL statements
within a single statement string.  In this situation, a malicious user could
submit a value like "1; DELETE FROM bugs;" that, when added to a query (f.e.
"SELECT * FROM bugs WHERE bug_id = $id") results in the deletion of all records
from one of the database tables.

The solution to this problem (which is what happens in the patch for this bug)
is to validate all values that will be inserted into SQL statements without
quotes.  Note that at this point this is just a theoretical risk.  As far as I
can tell DBI does not accept SQL strings with multiple semi-colon separated
statements within them.

Comment 12

[Jesse Ruderman]

See also bug 38862, where we're trying to figure out how to not let bugzilla 
attachments do things as the bugzilla user (through javascript) while at the 
same time preventing users from reading attachments on bugs they're not allowed 
to read.

Comment 13

[Dave Miller [:justdave]]

Moving to Bugzilla product