Closed Bug 346086 Opened 13 years ago Closed 13 years ago

[SECURITY] attachment.cgi lets you view descriptions of private attachments even when you are not in the insidergroup

Categories

(Bugzilla :: Attachments & Requests, defect, major)

2.17
defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: LpSolit, Assigned: LpSolit)

References

Details

(Whiteboard: [ready for 2.18.6][ready for 2.20.3][ready for 2.22.1][ready for 2.23.3])

Attachments

(4 files, 13 obsolete files)

17.95 KB, patch
myk
: review+
Details | Diff | Splinter Review
13.20 KB, patch
myk
: review+
Details | Diff | Splinter Review
11.77 KB, patch
myk
: review+
Details | Diff | Splinter Review
23.68 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
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.
Flags: blocking2.22.1?
Flags: blocking2.20.3?
Flags: blocking2.18.6?
Flags: blocking2.22.1?
Flags: blocking2.22.1+
Flags: blocking2.20.3?
Flags: blocking2.20.3+
Flags: blocking2.18.6?
Flags: blocking2.18.6+
Assignee: attach-and-request → LpSolit
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.
Status: NEW → ASSIGNED
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!
Severity: minor → major
Attached patch patch for tip, v1 (obsolete) — Splinter Review
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.
Attachment #231012 - Flags: review?(myk)
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

Attached patch patch for tip, v1.1 (obsolete) — Splinter Review
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.
Attachment #231012 - Attachment is obsolete: true
Attachment #231037 - Flags: review?(myk)
Attachment #231012 - Flags: review?(myk)
Blocks: 346524
Blocks: 346525
No longer blocks: 346524
Attached patch patch for 2.22, v1 (obsolete) — Splinter Review
Attachment #231409 - Flags: review?(myk)
Attached patch patch for tip, v1.2 (obsolete) — Splinter Review
Only list patches in diff mode as non-patches won't be accepted anyway. The patch for 2.22 already contains this fix.
Attachment #231037 - Attachment is obsolete: true
Attachment #231411 - Flags: review?(myk)
Attachment #231037 - Flags: review?(myk)
Attached patch patch for 2.20, v1 (obsolete) — Splinter Review
Attachment #231454 - Flags: review?(myk)
Attached patch patch for 2.18, v1 (obsolete) — Splinter Review
Attachment #231490 - Flags: review?(myk)
Attached patch patch for tip, v1.3 (obsolete) — Splinter Review
unbitrotten patch
Attachment #231411 - Attachment is obsolete: true
Attachment #232006 - Flags: review?(myk)
Attachment #231411 - Flags: review?(myk)
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.
Attachment #231409 - Flags: review?(myk) → review+
> 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.
Attachment #231454 - Flags: review?(myk) → review+
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.
Attachment #231490 - Flags: review?(myk) → review+
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.
Attachment #232006 - Flags: review?(myk) → review+
Attached patch patch for tip, v2 (obsolete) — Splinter Review
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.
Attachment #232006 - Attachment is obsolete: true
Attachment #235390 - Flags: review?(myk)
Blocks: 294021
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?
Attachment #235390 - Flags: review?(myk) → review+
(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.
Attached patch patch for 2.22, v2 (obsolete) — Splinter Review
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.
Attachment #231409 - Attachment is obsolete: true
Attachment #235789 - Flags: review?(myk)
Attached patch patch for tip, v2.1 (obsolete) — Splinter Review
Fixes request.cgi too.
Attachment #235390 - Attachment is obsolete: true
Attachment #235792 - Flags: review?(myk)
Attached patch patch for 2.20, v2 (obsolete) — Splinter Review
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.
Attachment #231454 - Attachment is obsolete: true
Attachment #235824 - Flags: review?(myk)
Attached patch patch for tip, v2.2 (obsolete) — Splinter Review
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.
Attachment #235792 - Attachment is obsolete: true
Attachment #235828 - Flags: review?(myk)
Attachment #235792 - Flags: review?(myk)
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.
Attachment #235789 - Attachment is obsolete: true
Attachment #235840 - Flags: review?(myk)
Attachment #235789 - Flags: review?(myk)
Attached patch patch for tip, v2.3 (obsolete) — Splinter Review
Check the private bit on time, see my comment above.
Attachment #235828 - Attachment is obsolete: true
Attachment #235842 - Flags: review?(myk)
Attachment #235828 - Flags: review?(myk)
OK, I think I got them all. This patch fixes the "check the private bit soon enough" issue.
Attachment #235824 - Attachment is obsolete: true
Attachment #235845 - Flags: review?(myk)
Attachment #235824 - Flags: review?(myk)
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).
Attachment #231490 - Attachment is obsolete: true
Attachment #236536 - Flags: review?(myk)
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
Attachment #235842 - Flags: review?(myk) → review+
Whiteboard: [ready for 2.23.3] patches awaiting review from myk
Attachment #235840 - Flags: review?(myk) → review+
Attachment #235845 - Flags: review?(myk) → review+
Attachment #236536 - Flags: review?(myk) → review+
Thanks for the reviews, myk. :)
Flags: approval?
Flags: approval2.22?
Flags: approval2.20?
Flags: approval2.18?
Whiteboard: [ready for 2.23.3] patches awaiting review from myk → [ready for 2.18.6][ready for 2.20.3][ready for 2.22.1][ready for 2.23.3]
bitrotten because UserInGroup() has been replaced by Bugzilla->user->in_group()
Attachment #235842 - Attachment is obsolete: true
Attachment #239857 - Flags: review+
I'm assuming this bug dates back to the original introduction of insidergroup, which was in 2.17 (according to bonsai).
Version: 2.18.5 → 2.17
Flags: approval?
Flags: approval2.22?
Flags: approval2.22+
Flags: approval2.20?
Flags: approval2.20+
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Summary: attachment.cgi lets you view descriptions of private attachments even when you are not in the insidergroup → [SECURITY] attachment.cgi lets you view descriptions of private attachments even when you are not in the insidergroup
Security Advisory has been sent, so this bug is no longer private.
Group: webtools-security
Blocks: 364748
You need to log in before you can comment on or make changes to this bug.