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]};

moving to real milestones...

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?

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.

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.

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...

setting owner to match patch author

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

OK, it's in.

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.

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.

Moving to Bugzilla product