449931 - [SECURITY] Unprivileged users can approve/unapprove all the quips (including bypassing moderation)

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Alexander Færøy]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; da; rv:1.9) Gecko/2008052912 Firefox/3.0
Build Identifier: Bugzilla 3.0.4

After one of our developers were playing around with the quip system in our Bugzilla setup he figured out that he could unapprove all our quips by visiting a specially crafted URL. 

For example: http://bugs.example.org/quips.cgi?action=approve

Our current quip setup is set to be 'moderated', but that doesn't change the outcome of the above URL.

Reproducible: Always

Steps to Reproduce:
1. Visit http://bugs.example.org/quips.cgi?action=approve
2. Wait for a Bugzilla admin to kill you.
Actual Results:  
Unapproving all the quips in the database

Expected Results:  
Show some access denied error or at least not unapprove all the quips :)

Perhaps it would be nice to create a group where the Bugzilla admins can put users in so that those users are allowed to approve bugs. But that's probably a feature request.

Comment 1

[Frédéric Buclin]

I can confirm this. The code in quips.cgi is crazy. If it gets action=approve and nothing else, it assumes you want to disable all quips, with no check at all about who you are.

I would say, the other way is more critical: when a powerless user adds a new quip with the param set to moderated, his quip is in the moderated queue till an admin approves it, and so the quip doesn't appear yet at the top of buglists. But with the trick above, and appending the ID of your quip to the URL, it gets approved by the attacker and now is visible at the top of buglists. And there is no log of who approved quips. As an attacker, I could put insults and have them approved.

We should leave this bug in the security group to prevent someone trying this on installations which use quips... and also because you can perform actions which are restricted to admins, which is generally bad.

Not blocking 3.2 RC1, but we should fix it in our next releases.

Comment 2

[Alexander Færøy]

Adding Robin. He has developed a patch for the issue.

Comment 3

[Robin H. Johnson [:robbat2]]

Attachment: Patch for quips security, against 2.22.x tree

This patch is against the Bugzilla 2.22 branch used by Gentoo.
Should apply to other 2.x versions with a little bit of fuzz. 3.x might need a bit more template work.

quips.cgi rev 1.32
template/en/default/list/quips.html.tmpl 1.16

Comment 4

[Frédéric Buclin]

Comment on attachment 333096 [details] [diff] [review]
Patch for quips security, against 2.22.x tree

>Quips usage must check for suitable privileges, and also ensure that just
>loading the action=approve page without any other parameters does not affect
>the state of the database.
>
>X-Mozilla-Bug: 449931
>Signed-off-by: Robin H. Johnson <[e-mail]>
>
>====
>This patch is against the Bugzilla 2.22 branch used by Gentoo. 
>Should apply to other versions with a little bit of fuzz.
>
>quips.cgi rev 1.32
>template/en/default/list/quips.html.tmpl 1.16
>

Please remove this part from the patch. That's not required. :)



>+++ quips.cgi	2008-08-09 17:04:45.000000000 +0000

> if ($action eq 'approve') {
>+    UserInGroup("admin")

Please write $user->in_group('admin') instead. UserInGroup is deprecated, even on 2.22.


>+        my $quip = $cgi->param('quipid_'.$quipid);

Write: my $quip = $cgi->param('quipid_'.$quipid) ? 1 : 0 so that you don't need to define $form below.

>+        my $quipe = $cgi->param('quipid_existing_'.$quipid);

Nit: the param should be named 'defined_quip_$quipid' to better match what we do it in Bugzilla 3.2. Also, you don't need to define $quipe if you do the check directly inside if (...).


>+        if(defined($quipe)) {
>+            my $form = $quip ? 1 : 0;

$form is now obsolete with my suggestion above.


>+                if($form) { push(@approved, $quipid); }
>+                else { push(@unapproved, $quipid); }

Please modify this to match what we do now:

 if (...) {
    push();
 }
 else {
    push();
 }

I know the code was already like this, but as we are here... :)



>+++ template/en/default/list/quips.html.tmpl	9 Aug 2008 17:06:18 -0000

>+              <input type="hidden" name="quipid_existing_[% quipid FILTER uri%]"
>+                     id="quipid_existing_[% quipid FILTER uri%]"
>+                     value="[%- quips.$quipid.approved+0 %]">

Its value should be 1, always.


I didn't test your patch yet, but it looks great. Fix the comments above and we are all good.

Comment 5

[Robin H. Johnson [:robbat2]]

Attachment: second take at quips security fix

"====" is the separator used between patch description and extra information, per LKML style. Most automatic tools should just strip it out.

This patch is against the 2.22.x branch used by Gentoo. Should apply to original 2.22 with a little bit of fuzz.

Against:
quips.cgi 1.32
template/en/default/list/quips.html.tmpl 1.16

The rest of your suggested changes are in place, and the fix does work on my production bugzilla.

Comment 6

[Frédéric Buclin]

Bug 184309, which introduced the moderation queue, forgot to implement this check. Affected versions: Bugzilla 2.17.4 and above.

Comment 7

[Frédéric Buclin]

Comment on attachment 333106 [details] [diff] [review]
second take at quips security fix

>+++ quips.cgi	2008-08-09 18:53:53.000000000 +0000

>+      || ThrowUserError("auth_failure", {group  => "admin",
>+                                         action => "approve",
>+                                         object => "quips"});

You still have to define action == "approve" in user-error.html.tmpl, else you get:

 "Sorry, you aren't a member of the 'admin' group, and so you are not authorized to quips."

Obviously, a verb is missing before 'quips'.


>+        my $quip = $cgi->param('quipid_'.$quipid) ? 1 : 0;
>+        if(defined($cgi->param('defined_quipid_'.$quipid))) {

Nit: 'foo_'.$bar could be written as "foo_$bar". It's a bit nicer.


>+            if($quips{$quipid} ne $quip) {

Both are integers. Should be != (ne is for strings).



>Index: template/en/default/list/quips.html.tmpl

>+              <input type="hidden" name="defined_quipid_[% quipid FILTER uri%]"
>+                     id="defined_quipid_[% quipid FILTER uri%]"
>+                     value="1">
>               <input type="checkbox" name="quipid_[% quipid FILTER uri%]"
>                      id="quipid_[% quipid FILTER uri%]"

Filtering is wrong at four places. Should be FILTER html (this has been fixed in Bugzilla 3.0 and 3.2). It doesn't harm as we pass an integer, but let's do it right anyway.


I tested your patch and it's working fine. It even applies on 3.2 RC1 without much changes (I just had to replace FILTER uri by FILTER html in the template above). We are very close to r+ now. :)

Comment 8

[Frédéric Buclin]

Robin, do you still have the motivation to update your patch? :) Remaining issues are very easy to fix.

Comment 9

[Robin H. Johnson [:robbat2]]

Attachment: quips-v3.patch (for 2.22)

LpSolit:
I'm busy with real life too.

I think your demands of fixing the style of the quips code at the same time are not really what security issues are about. It should fix the security bug, and nothing but the security bug - which I did with the original submission, to the coding standards already present in that file.

A second patch submission to clean up the file to the nitpicking modern BugZilla standards that you've had me do gets put on top of it then. Patch isolation is a good thing, not a bad thing.

Regardless, I've attached a third take of the patch that fixes your other list of nitpicks.

Comment 10

[Frédéric Buclin]

Comment on attachment 333583 [details] [diff] [review]
quips-v3.patch (for 2.22)

Robin, sorry to bother you with my nits. We try to keep our code as clean as possible, even when fixing security bugs.

Anyway, this patch is working great, and I have no other nit to report. Thanks a lot for your contribution. r=LpSolit

Comment 11

[Frédéric Buclin]

Attachment: patch for 2.20

On 2.20, $user is not defined in quips.cgi, so I had to replace it by Bugzilla->user. No other changes.

Comment 12

[Frédéric Buclin]

Attachment: patch for 3.x

This one applies on all 3.x installations. The only difference is that the template already uses FILTER html, so there was a small bitrot.

Comment 13

[Frédéric Buclin]

We are releasing Bugzilla 3.2 RC2 today. Time to check in this patch.

Comment 14

[Gervase Markham [:gerv]]

Do we normally open security bugs as soon as the new version is released? If so, we should not do so with this one. Someone could cause havoc. We need to give people time to update.

Gerv

Comment 15

[Frédéric Buclin]

tip:

Checking in quips.cgi;
/cvsroot/mozilla/webtools/bugzilla/quips.cgi,v  <--  quips.cgi
new revision: 1.39; previous revision: 1.38
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <-- user-error.html.tmpl
new revision: 1.265; previous revision: 1.264
done
Checking in template/en/default/list/quips.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/quips.html.tmpl,v  <--  quips.html.tmpl
new revision: 1.24; previous revision: 1.23
done


3.2rc1+:

Checking in quips.cgi;
/cvsroot/mozilla/webtools/bugzilla/quips.cgi,v  <--  quips.cgi
new revision: 1.38.2.1; previous revision: 1.38
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <-- user-error.html.tmpl
new revision: 1.249.2.2; previous revision: 1.249.2.1
done
Checking in template/en/default/list/quips.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/quips.html.tmpl,v  <--  quips.html.tmpl
new revision: 1.22.2.1; previous revision: 1.22
done


3.0.5+:

Checking in quips.cgi;
/cvsroot/mozilla/webtools/bugzilla/quips.cgi,v  <--  quips.cgi
new revision: 1.37.2.1; previous revision: 1.37
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <-- user-error.html.tmpl
new revision: 1.204.2.15; previous revision: 1.204.2.14
done
Checking in template/en/default/list/quips.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/quips.html.tmpl,v  <--  quips.html.tmpl
new revision: 1.19.2.2; previous revision: 1.19.2.1
done


2.22.5+:

Checking in quips.cgi;
/cvsroot/mozilla/webtools/bugzilla/quips.cgi,v  <--  quips.cgi
new revision: 1.32.2.1; previous revision: 1.32
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <-- user-error.html.tmpl
new revision: 1.145.2.20; previous revision: 1.145.2.19
done
Checking in template/en/default/list/quips.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/quips.html.tmpl,v  <--  quips.html.tmpl
new revision: 1.16.4.1; previous revision: 1.16
done


2.20.6+:

Checking in quips.cgi;
/cvsroot/mozilla/webtools/bugzilla/quips.cgi,v  <--  quips.cgi
new revision: 1.28.2.1; previous revision: 1.28
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <-- user-error.html.tmpl
new revision: 1.115.2.13; previous revision: 1.115.2.12
done
Checking in template/en/default/list/quips.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/quips.html.tmpl,v  <--  quips.html.tmpl
new revision: 1.16.2.1; previous revision: 1.16
done

Comment 16

[Frédéric Buclin]

(In reply to comment #14)
> Do we normally open security bugs as soon as the new version is released?

Yes we do. It doesn't make sense to keep this one secret as the security advisory already contains details of the issue.

Comment 17

[Gervase Markham [:gerv]]

Does the security advisory give people enough information to exploit the issue?

If so, I would note that the Bugzilla team's approach to security bugs is different to the rest of the Mozilla project's. Normally, as I understand it, MFSAs do not include sufficient detail to exploit the issue, and bugs are not opened immediately upon the release of the patched version. 

I think it might be a good idea to coordinate with Dan Veditz and ask him about project best practice, to make sure we are not putting Bugzilla admins at unnecessary risk.

Gerv

Comment 18

[Frédéric Buclin]

There are much more critical security bugs than this one. To know what changed, all an attacker needs to do is to do a diff of the file, and it will appear very obviously how to build the URL to bypass checks. The fact that we (the Bugzilla team) do things differently from the rest of Mozilla projects is really not new. But this discussion has nothing to do in this bug.

Comment 19

[Max Kanat-Alexander]

Security advisory sent, unlocking this bug.

Comment 20

[Reed Loden [:reed]]

As a side note, somebody used this vulnerability against BMO and unapproved all the quips. ;)

Comment 21

[Frédéric Buclin]

That's why we send security advisories. There are supposed to be read, not ignored. :-p

Comment 22

[Denis Roy]

<rant>

This bug really, really saddens me, for two reasons.

1. Robin's concerns in comment 9 are perfectly valid. The BZ team is literally begging for contributions (I get asked every time I go to editparams), but when someone does contribute, they get needlessly nitpicked.

It would have taken you less time to clean up the patch than to enumerate all the nits.


2. As the admin of a large Bugzilla installation, I'm really glad I disabled quips long ago, because this afternoon's announcement would have had me scrambling to update. Of course "There are much more critical security bugs than this one" LpSolit, but this exploit is so trivially easy to action that you just know someone will try it..

(In reply to comment #18)
> The fact that we (the
> Bugzilla team) do things differently from the rest of Mozilla projects is
> really not new.

That doesn't necessarily mean it's a good thing.

</rant>

Comment 23

[Frédéric Buclin]

(In reply to comment #22)
> 1. Robin's concerns in comment 9 are perfectly valid. The BZ team is literally
> begging for contributions (I get asked every time I go to editparams), but when
> someone does contribute, they get needlessly nitpicked.

FYI, I backported the patch to all other branches myself. And in many cases, patch authors are happy to fix their own patches themselves (as I was when I started contributing, for instance). Also, I asked in comment 8 if he wanted to update his patch himself. He could have answered 'no' and we would have done it ourselves.


> than this one" LpSolit, but this exploit is so trivially easy to action that
> you just know someone will try it..

So what?



> That doesn't necessarily mean it's a good thing.

Neither that's it's a bad thing. But we are completely out of the topic of this bug. There are forums for that.

Comment 24

[Denis Roy]

> So what?

My lord... From your comment 1, you seem to understand the severity of this issue. Have you no consideration for the admins of large sites who have responsibilities beyond patching Bugzilla the minute an advisory goes out? An advisory that reveals every detail about the exploit, no less, and even a way to inject insults, profanity and who knows what.

For some reason, comment 17 made a lot of common sense to me.

BTW: I'm not so sure we're off-topic here.  This is a good indication of how the Bugzilla team goes about handling security advisories. I must say, this approach is far from great.

Comment 25

[Dave Miller [:justdave]]

This is primarily a cosmetic feature, a successful exploitation will have no effect on your actual bug data.  Regardless of that, figuring out how to exploit this bug is trivial.  There is no way to announce that we fixed it without giving enough information for someone to figure out how to exploit.  Yes, the fact that it exists is a black eye for us, as is any other security issue.  In this case, because of the lack of sensitive data there, nobody ever really bothered to audit it (most folks seem to consider the quips feature to be a toy) aside from making sure you couldn't damage anything else from there.

The concern about the nitpicking has been discussed a number of times, and we're trying to work on it.  We really do want to encourage outside contributions, and sometimes we need a good reminder like this.  I do know that historically, the idea that a contributor should fix his/her own patches has come from the idea that we don't like to steal a contributor's thunder by winding up with people taking dual credit for a patch if someone else fixes it for them, and not by any means from a "we don't feel like doing the extra work" angle.  It's often hard to convey that, or forgotten to be conveyed.

In defense of Fréderic, his responses often hint to me of language barrier issues.  Although his command of English is quite good, it's not his primary language, and the connotation of some phrases he chooses to use often fail to convey the "I'm just making some suggestions here" and come off as a "you need to do this" more often than it should.  I know him well enough to understand that's all he means, and usually don't even notice it, because when he really does mean "you need to do this" it's pretty darn explicit.  But to someone coming in from the outside that hasn't dealt with him before it's certainly hard to know.  Comment #8, on review, does sound a bit condescending.  It comes off with a feel of "We'll look badly upon you if you're not motivated anymore" which I'm sure isn't what he meant.

Comment 26

[Dave Miller [:justdave]]

(and just for reference, comments 23 and 24 hadn't been posted yet when I posted that, I midaired with them posting it)

Comment 27

[Gervase Markham [:gerv]]

Regardless of the triviality of exploit of this particular case, I continue to assert that opening security bugs the moment we release patches and advisories is bad practice. Dave: where is the appropriate forum to have that discussion? The mailing list?

Gerv