When viewing a public attachment in the "diff" mode, i.e. an attachment not marked as "private", a user who is not in the insidergroup (the group required to view private attachments) can read *all* descriptions of attachments, including those being "private". Note that the attachment itself is correctly protected, only its description is visible. My descriptions are rather unexploitable, because they are always of the form "patch v2 for tip", but timeless told me today that sometimes people put more useful descriptions than me. :) In this case, there is a risk (probably a low one) that someone could try to use this information to exploit the security bug (we know it's a security bug, else why would this attachment be protected?). dveditz asked justdave in bug 330892 to set the 'insidergroup' parameter to 'security', so I think this hole should be addressed before bug 330892 is fixed, else anyone knowing this "trick" could try to use it. I can reproduce this bug on both 2.18.5+ and tip, so all supported versions of Bugzilla are affected. Note that the fix is trivial and non-invasive. I mark the severity of this bug as "minor" as reading the description is probably much less critical than reading the patch itself.

LpSolit: do you have a patch for this bug? Your original description implies that you might. You are right - it would be good to apply this to b.m.o. before turning on insidergroup. Gerv

gerv, not yet. I saw this problem when working on bug 343814 this morning. But I can have it ready today or tomorrow.

Oops, while scanning attachment.cgi to make sure there are no other security holes, I found a bigger one as I reached to get *all* attachment descriptions of *all* bugs, including security bugs. Note that you cannot do that using the UI. You have to write the URL yourself and pass well choosen parameters, but at least I have them all.

About my previous comment, you need editbugs privs to exploit it, but that's pretty common, at least on b.m.o. And I think several installations use the default regexp used for the canconfirm and editbugs groups, i.e. .*, meaning that on some installations, everybody can exploit it by default.

Now I found a 3rd way to get descriptions of private attachments, including flags set on them: use the "Format for printing" (aka "Show multiple bugs at once"). Note that you need to have access to the bug to exploit it. This 3rd way doesn't require any privilege though. More important: this only affects Bugzilla 2.24. 2.22 and older are not affected. Yay!

This patch fixes all 3 ways I found to get descriptions of private attachments without having the required privs. We are currently pretty inconsistent about what to do with private attachments when the insidergroup parameter is reset. Some templates display private attachments, some other still hide/exclude them. For comparison, private comments are still hidden. I think this is the right way to go.

It seems to me that the insider functionality needs a data leak audit. How many of the Bugzilla developers regularly use installations where it's enabled, when the developer themselves is outside the group? Not many, I'll bet. Gerv

Two small improvements over v1: - Now attachments the user tries to mark as obsolete are checked before inserting a new attachment into the DB. Else you can get an error when a first insertion has already been made, meaning that you only have partial data in the DB; - I now hide the ID of attachments the user cannot access when editing another attachment of the same bug. Now the UI is fully consistent with what you are really allowed to do/view.

Only list patches in diff mode as non-patches won't be accepted anyway. The patch for 2.22 already contains this fix.

unbitrotten patch

Comment on attachment 231409 [details] [diff] [review] patch for 2.22, v1 >Index: Bugzilla/Attachment.pm >+ my $user = Bugzilla->user; >+ my $dbh = Bugzilla->dbh; >+ >+ # By default, private attachments are not accessible... >+ my $and_restriction = 'AND isprivate = 0'; >+ my $insider_group = Param('insidergroup'); >+ >+ # ... unless the user is in the insider group. >+ if ($insider_group && $user->in_group($insider_group)) { >+ $and_restriction = ''; >+ } Nit: it seems like you repeat this logic several times (albeit in slightly modified forms), so it might make sense to factor it out into a separate method, f.e. $user->is_insider, which returns true if there's an insider group and the user is in it, false otherwise. Then you could write this code more simply as: my $and_restriction = $user->is_insider ? "" : "AND isprivate = 0"; And later in validateID you could say simply: if ($isprivate && !$user->is_insider) { ThrowUserError("auth_failure", {action => "access", object => "attachment"}); } And in validateCanEdit something like: if ($attachment->isprivate && !Bugzilla->user->is_insider) { ThrowUserError('illegal_attachment_edit', {attach_id => $attachment->id}); } >Index: attachment.cgi >+ my $insider_group = Param('insidergroup'); >+ if ($isprivate && (!$insider_group || !$user->in_group($insider_group))) { >+ ThrowUserError("auth_failure", {action => "access", >+ object => "attachment"}); > } So if $isprivate is set, but there's no insider group, then no one can access the attachment? Seems like $isprivate should only apply when there is an insider group defined, no? Perhaps I misunderstand. > sub validateCanEdit > { > my ($attach_id) = (@_); >+ my $user = Bugzilla->user; >+ >+ my $attachment = Bugzilla::Attachment->get($attach_id); >+ # Only people in the insider group can view private attachments. >+ my $insider_group = Param('insidergroup'); >+ if ($attachment->isprivate >+ && (!$insider_group || !$user->in_group($insider_group))) >+ { >+ ThrowUserError('illegal_attachment_edit', {attach_id => $attachment->id}); >+ } > > # People in editbugs can edit all attachments > return if UserInGroup("editbugs"); > > # Bug 97729 - the submitter can edit their attachments >- SendSQL("SELECT attach_id FROM attachments WHERE " . >- "attach_id = $attach_id AND submitter_id = " . Bugzilla->user->id); >+ return if ($attachment->attacher->id == $user->id); Shouldn't submitters be able to edit their attachments, even if those attachments get moved to the insider group? That would be consistent with how we treat bug reporters, i.e. by default they have access to their bugs, even if the bugs get restricted to some group, unless someone explicitly removes their access by unchecking the "Reporter can always view this bug" checkbox. Otherwise this looks fine, and assuming the answers to my two questions above do not necessitate any code changes, then r=myk. But those questions should be explicitly addressed (and probably documented somewhere) before this goes in.

> So if $isprivate is set, but there's no insider group, then no one can access > the attachment? Seems like $isprivate should only apply when there is an > insider group defined, no? Perhaps I misunderstand. Ok, I see now that you address this in comment 6. Seems like we should be consistent with the way comments work, and if we want to change the way they work, we should do so in a separate bug, so this is fine.

Comment on attachment 231454 [details] [diff] [review] patch for 2.20, v1 r=myk modulo the "edit your own attachments, even if they're private" issue.

Comment on attachment 231490 [details] [diff] [review] patch for 2.18, v1 r=myk modulo the "edit your own attachments, even if they're private" issue.

Comment on attachment 232006 [details] [diff] [review] patch for tip, v1.3 >+ Only users being in the insider group can view private attachments. ... >+ # Only people in the insider group can view private attachments. Nit: both of these should read: # Only users in the insider group can view private attachments. Otherwise this looks fine. r=myk modulo the "edit your own attachments, even if they're private" issue.

I now let the submitter view his own attachments, even if they have been added to the insider group. I also refactored the code a bit as suggested by myk, and I implemented $user->is_insider. There are many places all over the code where the way to consider private attachments when no insider group is set is not consistent. This is definitely a edge case which I don't want to fix in this security bug. In other words, all other places do not care if you are the submitter of the private attachment. Also, some places will let you view some data about attachments if the insidergroup is empty, some other places won't. I don't consider this as part of the security bug.

Comment on attachment 235390 [details] [diff] [review] patch for tip, v2 This looks fine. But we should get confirmation that allowing submitters to see and modify their own attachments, even when those attachments have been moved to the insider group, is what we want. Gerv, dveditz, Dave, what think you?

(In reply to comment #20) It makes sense to me to allow the submitter to see their own attachments regardless... it's consistent with how we deal with reporters vs bugs, and what people would expect I think.

By "submitter" you mean the person who added the attachment, not the bug reporter, right? If so it should be fine. For bugs we have the option of hiding them from the reporter if we add comments we don't want to share (though now with insider-comments that should be even less likely), but we can't really add anything to an attachment the submitter doesn't already know.

This patch lets submitters to view their attachments, even if they are marked as private, and I also implemented User::is_insider() as suggested by myk. This patch also fixes request.cgi which let requests made on private attachments to be visible by everyone.

Fixes request.cgi too.

Addresses myk's comments about letting submitters view their own attachments and refactoring code inside User::is_insider(). This patch also prevents submitters not being in the insidergroup from overwriting the private bit on their attachments.

The UI doesn't let you turn on/off the private bit on attachments if you are not in the insidergroup. Consequently, attachment.cgi won't see the 'isprivate' form and will assume that you want it to be off, which is wrong in this case. So I force attachment.cgi to ignore isprivate unless you have enough privs to change it. This rule applies to the submitter too.

Note only have we to make sure a user not being in the insidergroup cannot change the private bit, but we have to make this check at the right time, i.e. *before* calling Flag*::validate() which look at the private bit to determine whether the requestee is allowed to view the attachment or not. I did an ugly hack, but this is the less risky change for branches.

Check the private bit on time, see my comment above.

OK, I think I got them all. This patch fixes the "check the private bit soon enough" issue.

I added UserIsInsider() in globals.pl which is a nice shortcut to avoid checking all the time whether the user is logged in or not. Indeed, Bugzilla->user->foo() fails if the user is logged out on 2.18 (2.18 doesn't allow empty user object).

Comment on attachment 235842 [details] [diff] [review] patch for tip, v2.3 Looks good, and staring at it longer won't add any more value. r=myk

Thanks for the reviews, myk. :)

bitrotten because UserInGroup() has been replaced by Bugzilla->user->in_group()

I'm assuming this bug dates back to the original introduction of insidergroup, which was in 2.17 (according to bonsai).

tip: Checking in attachment.cgi; /cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v <-- attachment.cgi new revision: 1.123; previous revision: 1.122 done Checking in request.cgi; /cvsroot/mozilla/webtools/bugzilla/request.cgi,v <-- request.cgi new revision: 1.41; previous revision: 1.40 done Checking in Bugzilla/Attachment.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v <-- Attachment.pm new revision: 1.40; previous revision: 1.39 done Checking in Bugzilla/User.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v <-- User.pm new revision: 1.134; previous revision: 1.133 done Checking in Bugzilla/Attachment/PatchReader.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment/PatchReader.pm,v <-- PatchReader.pm new revision: 1.3; previous revision: 1.2 done Checking in template/en/default/filterexceptions.pl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v <-- filterexceptions.pl new revision: 1.81; previous revision: 1.80 done Checking in template/en/default/attachment/list.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/list.html.tmpl,v <-- list.html.tmpl new revision: 1.32; previous revision: 1.31 done Checking in template/en/default/attachment/show-multiple.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/show-multiple.html.tmpl,v <-- show-multiple.html.tmpl new revision: 1.20; previous revision: 1.19 done 2.22: Checking in attachment.cgi; /cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v <-- attachment.cgi new revision: 1.103.2.2; previous revision: 1.103.2.1 done Checking in request.cgi; /cvsroot/mozilla/webtools/bugzilla/request.cgi,v <-- request.cgi new revision: 1.29.2.2; previous revision: 1.29.2.1 done Checking in Bugzilla/Attachment.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v <-- Attachment.pm new revision: 1.30.2.1; previous revision: 1.30 done Checking in Bugzilla/User.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v <-- User.pm new revision: 1.101.2.5; previous revision: 1.101.2.4 done Checking in template/en/default/filterexceptions.pl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v <-- filterexceptions.pl new revision: 1.61.2.3; previous revision: 1.61.2.2 done Checking in template/en/default/attachment/list.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/list.html.tmpl,v <-- list.html.tmpl new revision: 1.25.2.2; previous revision: 1.25.2.1 done Checking in template/en/default/attachment/show-multiple.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/show-multiple.html.tmpl,v <-- show-multiple.html.tmpl new revision: 1.15.12.2; previous revision: 1.15.12.1 done 2.20.2: Checking in attachment.cgi; /cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v <-- attachment.cgi new revision: 1.89.2.5; previous revision: 1.89.2.4 done Checking in request.cgi; /cvsroot/mozilla/webtools/bugzilla/request.cgi,v <-- request.cgi new revision: 1.23.2.3; previous revision: 1.23.2.2 done Checking in Bugzilla/Attachment.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v <-- Attachment.pm new revision: 1.21.4.1; previous revision: 1.21 done Checking in Bugzilla/User.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v <-- User.pm new revision: 1.61.2.21; previous revision: 1.61.2.20 done Checking in template/en/default/attachment/list.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/list.html.tmpl,v <-- list.html.tmpl new revision: 1.21.2.1; previous revision: 1.21 done 2.18.5: Checking in attachment.cgi; /cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v <-- attachment.cgi new revision: 1.58.2.7; previous revision: 1.58.2.6 done Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/Attic/globals.pl,v <-- globals.pl new revision: 1.270.2.14; previous revision: 1.270.2.13 done Checking in request.cgi; /cvsroot/mozilla/webtools/bugzilla/request.cgi,v <-- request.cgi new revision: 1.14.2.3; previous revision: 1.14.2.2 done Checking in Bugzilla/Attachment.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v <-- Attachment.pm new revision: 1.17.2.1; previous revision: 1.17 done Checking in Bugzilla/User.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v <-- User.pm new revision: 1.20.2.4; previous revision: 1.20.2.3 done Checking in template/en/default/attachment/list.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/list.html.tmpl,v <-- list.html.tmpl new revision: 1.17.2.2; previous revision: 1.17.2.1 done

Security Advisory has been sent, so this bug is no longer private.