824399 - (CVE-2013-0786) [SECURITY] build_subselect() leaks the existence of products and components you cannot access

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Frédéric Buclin]

In Bugzilla 2.17.1 (bug 179960), Bugzilla::Search::build_subselect() was introduced to improve performance when querying products and components. Its job is to run the subselect itself and return the corresponding product/component IDs. But by doing this, it leaks the existence of products and components you cannot see.

If you try to directly access a product you cannot see, Bugzilla will throw an error of the form "either this product doesn't exist, or you don't have access to it". This means you have no way to know if this product exists, as expected. But when running queries, build_subselect() will return 1=2 if the product doesn't exist at all, else it will return the product ID. So you can use this trick to determine if a product doesn't exist at all, or exists but you are not allowed to access it.

Bugzilla 2.17.1 to 4.0.x are affected (due to bug 179960), then 4.1.1 to 4.4rc1 are not affected (due to bug 579568), and 4.4rc1+ to 4.5 are affected again (due to bug 780820). As we didn't release 4.4rc2 or 4.5.1 yet, only the 3.6.x and 4.0.x releases are affected by this issue.

There is a clear performance by running the subselects in build_subselect() itself and returning the IDs only, so we certainly don't want to revert that. So I have no idea how to fix this issue. The only way would be to disable the debug mode entirely, but that's too useful to be disabled.

Comment 1

[Frédéric Buclin]

f****

Comment 2

[Simon Green]

Given bug 824262 is affected by the same issue (albeit not a security one) maybe the best option (for fixing both bugs) is have a new column in fielddefs (or hardcoded in Search.pm) whether to pre-compute sub selects or not) 

e.g. change the if statement in https://bug824262.bugzilla.mozilla.org/attachment.cgi?id=695242

Comment 3

[Frédéric Buclin]

No, because if we return subselects for products and components instead of returning the hardcoded list of IDs, the perf penalty is huge again. So from a performance point of view, it makes total sense to execute the subselects ourselves for products and components.

As I said in comment 0, maybe disabling the debug mode for non-admins (or some other group) is the only viable way to fix this issue.

Comment 4

[Dave Miller [:justdave]]

Would it be possible to have the context of "visible products" specified as a condition to the subselect when generating queries?  That would solve the problem, too.

Comment 5

[Frédéric Buclin]

(In reply to Dave Miller from comment #4)
> Would it be possible to have the context of "visible products" specified as
> a condition to the subselect when generating queries?  That would solve the
> problem, too.

We cannot rely on the caller to think about security when calling Bugzilla::Search. As discussed on IRC, I'm not a fan to have build_subselect() add additional criteria itself when talking about products or components, because this means joining the group_control_map and the user_group_map tables, making the whole thing slower again. As we already filter bugs the user can see, this would mean doing security checks twice (more or less, because a product can be visible while a bug in this product is restricted to a group you don't belong to).

Honestly, 99% of users don't even know about this debug mode in buglist.cgi, and the remaining 1% is mostly developers. Developers usually test stuff on their own local test installation, where they are admins. So I would be in favor to restrict the debug mode to members of the admin group only. I don't want to regress performance of Bugzilla::Search because a few users may want to play with the debug mode to try to collect some info about products or components + other fields we may not think about (I guess group names have the same problem, ditto for target milestones and versions).

Moreover, it's reasonable to assume that on some installations which have their own (additional) code to get bugs, the content of the generated SQL query may reveal confidential data that admins wouldn't want to make public.

Comment 6

[:glob ✱]

> So I would be in favor to restrict the debug mode to members of the admin group only.

this sounds like the best approach to me.

instead of hardcoding the admin group, it would be useful to have the group configurable so sites can choose to bless debug abilities onto trusted users without giving them full admin.

Comment 7

[Simon Green]

(In reply to Byron Jones ‹:glob› from comment #6)
> instead of hardcoding the admin group, it would be useful to have the group
> configurable so sites can choose to bless debug abilities onto trusted users
> without giving them full admin.

+1

Is anyone working on fixing this? If not, I'm happy to give this a crack on Monday (AEST).

One question though, should the group name by configured in the data/params file, like the insidergroup value is (default value: admin), or should I automatically create a new group?

I can imagine some installations may want to set this value as blank as either (a) they are not affected by this issue (no private products) or (b) the requiredlogin value is set.

  -- simon

Comment 8

[Frédéric Buclin]

(In reply to Simon Green from comment #7)
> Is anyone working on fixing this? If not, I'm happy to give this a crack on
> Monday (AEST).

Feel free to work on it.


> One question though, should the group name by configured in the data/params
> file, like the insidergroup value is (default value: admin), or should I
> automatically create a new group?

No need for a hardcoded group. Simply add a new param named e.g. "debugger_group", and set it to admin by default.


> I can imagine some installations may want to set this value as blank as
> either (a) they are not affected by this issue (no private products) or (b)
> the requiredlogin value is set.

Blank means nobody can access the feature, because nobody belongs to the group. That's the desired behavior.

Comment 9

[Simon Green]

(In reply to Frédéric Buclin from comment #8)
> Feel free to work on it.

Will do. I'll try and get this done on Monday.

> No need for a hardcoded group. Simply add a new param named e.g.
> "debugger_group", and set it to admin by default.

Sounds good to me.
 
> > I can imagine some installations may want to set this value as blank as
> > either (a) they are not affected by this issue (no private products) or (b)
> > the requiredlogin value is set.
> 
> Blank means nobody can access the feature, because nobody belongs to the
> group. That's the desired behavior.

For the reasons stated above, I would take blank to mean debug is available to everyone (most installations don't have a everybody group like bmo does)

  -- simon

Comment 10

[Frédéric Buclin]

(In reply to Simon Green from comment #9)
> For the reasons stated above, I would take blank to mean debug is available
> to everyone (most installations don't have a everybody group like bmo does)

No, else this param would behave in the exact opposite way of other group parameters. Leaving a group param blank always meant to disable the feature. I don't want to make an exception here.

Comment 11

[Simon Green]

(In reply to Frédéric Buclin from comment #10)
> I don't want to make an exception here.

Fair point.

Comment 12

[Simon Green]

Attachment: v1 patch

First crack at this. Tested against trunk, patches cleanly against 4.0 (with offsets). I'm not entirely sure about the wording of the message on the parameters page. If you want it changed, please suggest an alternative :)

I also decided to silently ignore a user that tried to view debugging information without being in the appropriate group. If you want an error message to appear, let me know.

Comment 13

[Frédéric Buclin]

Comment on attachment 698242 [details] [diff] [review]
v1 patch

>=== modified file 'Bugzilla/Config/GroupSecurity.pm'

>+   name => 'debuggroup',

Let's name it debug_group, to follow our guidelines (old param names were using the foobar syntax instead of the more readable foo_bar).



>=== modified file 'buglist.cgi'

>+if ($cgi->param('debug')
>+    and $user->in_group(Bugzilla->params->{debuggroup})

Before calling $user->in_group(), you must make sure that debug_group is set. Also, let's use && instead of and, which is what we use 99.5% of the time.


You must also patch report.cgi and/or reports/report.html.tmpl which also have a debug mode.



>=== modified file 'template/en/default/admin/params/groupsecurity.html.tmpl'

>+  debuggroup => "The name of the group of users who can view the raw " _
>+                "SQL queries in buglist.cgi. Note: This can expose " _
>+                "private products and components. See " _
>+                "<a href=\"https://bugzilla.mozilla.org/show_bug.cgi?id=824399\">bug 824399</a> " _
>+                "for more information.",

Do not mention products and components specifically, nor the bug ID (that's not the right place for it anyway). Also, this parameter is not specific to buglist.cgi as report.cgi is involved too.

Comment 14

[Simon Green]

Attachment: v2 patch

Made the suggested changes. This patch is for trunk only. When it is r+, I'll generate patches for 4.4 and 4.0 too.

Comment 15

[Frédéric Buclin]

Comment on attachment 698320 [details] [diff] [review]
v2 patch

>=== modified file 'report.cgi'

>+my $debug = $cgi->param('debug')
>+    && Bugzilla->params->{debug_group}
>+    && Bugzilla->user->in_group(Bugzilla->params->{debug_group});

You must move this code after Bugzilla->login() is called, else Bugzilla->user doesn't point to the current user yet, and it always returns 0.


>-$format->{'ctype'} = "text/html" if $cgi->param('debug');
>+$format->{'ctype'} = "text/html" if $debug;

This change must be reverted. It's fine to display raw data for reports for all users, as it displays the same data as in the table itself, but in a different way.


>-if ($cgi->param('debug')) {
>+if ($debug) {

This change must be reverted too, for the same reason as above.

What must be fixed is this line:

    $vars->{'debug'} = $cgi->param('debug');

else the template still displays debug stuff about executed SQL queries.


Everything else is fine.

Comment 16

[Frédéric Buclin]

To be honest, I don't want this fix for 3.6 and 4.0. Adding a new group so late in these respective branches is not a good idea, IMO. 3.6 is going to reach EOL very soon anyway. 4.2 is not affected so that's not a problem and it doesn't need the fix. This leaves us with 4.4 and trunk only where I'm fine to take this fix.

What do you guys think?

Comment 17

[David Lawrence [:dkl]]

Sounds reasonable.

Comment 18

[Simon Green]

I disagree. It should be fixed for Bugzilla 4.0. We aren't adding a new group, we are adding a new parameter. It's not ideal, but some sites (brc included) wouldn't want the existence of products shown.

Given that this bug was originally public, it is the reason we haven't put a 4.4rc1 preview on a public facing server.

Having said that brc is currently running 4.2, so from that perspective it doesn't bother me if you don't fix Bugzilla 4.0.

Comment 19

[Dave Miller [:justdave]]

Looks like a simple enough patch to be safe in 4.0 to me.

Comment 20

[Simon Green]

Attachment: v3 patch

Comment 21

[Frédéric Buclin]

(In reply to Dave Miller [:justdave] from comment #19)
> Looks like a simple enough patch to be safe in 4.0 to me.

The question is not about the patch being trivial/safe but about adding a new parameter in the 10th minor release of a stable branch. For 3.6, this would be the 13th minor release on that branch. Is that really what we want?

Comment 22

[Frédéric Buclin]

Comment on attachment 703061 [details] [diff] [review]
v3 patch

>=== modified file 'report.cgi'

>+if (Bugzilla->params->{debug_group}
>+    && Bugzilla->user->in_group(Bugzilla->params->{debug_group})
>+) {
>+    $vars->{'debug'} = $cgi->param('debug');
>+}

I would prefer a fix like you did for buglist.cgi, i.e.:

if ($cgi->param('debug')
    && Bugzilla->params->{debug_group}
    && Bugzilla->user->in_group(Bugzilla->params->{debug_group}))
{
    $vars->{'debug'} = 1;
}

This way we don't waste cycles into ->in_group() if the user didn't ask for debug stuff.


r=LpSolit with this comment fixed. Please upload an updated patch which includes this fix.

Comment 23

[Dave Miller [:justdave]]

(In reply to Frédéric Buclin from comment #21)
> (In reply to Dave Miller [:justdave] from comment #19)
> > Looks like a simple enough patch to be safe in 4.0 to me.
> 
> The question is not about the patch being trivial/safe but about adding a
> new parameter in the 10th minor release of a stable branch. For 3.6, this
> would be the 13th minor release on that branch. Is that really what we want?

Most of that argument is irrelevant because it's a security fix.  What we want is the safety and security of our users.

Comment 24

[Simon Green]

Attachment: v4 patch, 4.4 + trunk

Comment 25

[Simon Green]

(In reply to Dave Miller [:justdave] from comment #23)
> Most of that argument is irrelevant because it's a security fix.

+1

> What we want is the safety and security of our users.

and of the company's data.

Comment 26

[Frédéric Buclin]

Comment on attachment 703629 [details] [diff] [review]
v4 patch, 4.4 + trunk

>=== modified file 'template/en/default/admin/params/groupsecurity.html.tmpl'

>+  debug_group => "The name of the group of users who can view the actual " _
>+                 "SQL query generated when viewing bug lists and reports.",

Oops, I just realized that you have "bug" hardcoded in the description. It must be $terms.bug. Could you please attach an updated patch with this fix? Everything else looks good and works fine. r=LpSolit

Comment 27

[Frédéric Buclin]

Comment on attachment 703629 [details] [diff] [review]
v4 patch, 4.4 + trunk

The patch doesn't apply cleanly to 4.2 and older (conflict in report.cgi). It needs to be backported.

Comment 28

[Frédéric Buclin]

@reed, @dveditz: we need a CVE number for this bug.

Comment 29

[Simon Green]

Attachment: v5 patch, 4.4+trunk

Now runtest compliant. I really need to run that before submitting bugs :)

Comment 30

[Simon Green]

Attachment: v5 patch, 4.0 and 4.2

Comment 31

[Frédéric Buclin]

Comment on attachment 703859 [details] [diff] [review]
v5 patch, 4.4+trunk

Thanks! r=LpSolit

Comment 32

[Simon Green]

Attachment: v5 patch, 3.6

Comment 33

[Frédéric Buclin]

Comment on attachment 703860 [details] [diff] [review]
v5 patch, 4.0 and 4.2

r=LpSolit

Comment 34

[Frédéric Buclin]

Comment on attachment 703863 [details] [diff] [review]
v5 patch, 3.6

r=LpSolit

Comment 35

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #28)
> @reed, @dveditz: we need a CVE number for this bug.

dveditz: ping?

Comment 36

[Daniel Veditz [:dveditz]]

CVE-2013-0786

Comment 37

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified buglist.cgi
modified report.cgi
modified Bugzilla/Config/GroupSecurity.pm
modified template/en/default/admin/params/groupsecurity.html.tmpl
Committed revision 8586.


Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified buglist.cgi
modified report.cgi
modified Bugzilla/Config/GroupSecurity.pm
modified template/en/default/admin/params/groupsecurity.html.tmpl
Committed revision 8524.


Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified buglist.cgi
modified report.cgi
modified Bugzilla/Config/GroupSecurity.pm
modified template/en/default/admin/params/groupsecurity.html.tmpl
Committed revision 8190.


Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/
modified buglist.cgi
modified report.cgi
modified Bugzilla/Config/GroupSecurity.pm
modified template/en/default/admin/params/groupsecurity.html.tmpl
Committed revision 7743.


Committing to: bzr+ssh://[e-mail]/bugzilla/3.6/
modified buglist.cgi
modified report.cgi
modified Bugzilla/Config/GroupSecurity.pm
modified template/en/default/admin/params/groupsecurity.html.tmpl
Committed revision 7310.

Comment 38

[Frédéric Buclin]

Security advisory sent. Removing sec flag.