781850 - (CVE-2012-4198) [SECURITY] Do not leak the existence of groups when using User.get()

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Frédéric Buclin]

I just realized that User.get becomes way too verbose in Bugzilla 4.4. With a powerless account, I can access personal information, some of which power users shouldn't have access to either (even with admin or editusers privs):

- (confidential) groups a user belongs to, bug 548198 (Bugzilla 4.0!)
- unshared saved searches, bug 697224 (Bugzilla 4.4)

This is totally unacceptable and this information must only be disclosed if you are querying your own account, i.e. the user ID matches Bugzilla->user->id.

I know I approved bug 697224, but I didn't realize that all users could have access to this information. I clearly withdraw my approval here.

Said differently, saved searches should never be disclosed to anyone but you. From the UI, admins have no way to get this information, except by impersonating the user account, in which case the user is informed by email that someone is looking at his account. So there is no reason that someone can have access to this information using User.get, even for admins.

About groups, User.get is disclosing if a group exists or not, defeating CVE-2011-2380, see bug 653477. Bugzilla::Group->check_no_disclose() must be used instead of Bugzilla::Group->check().

Comment 1

[Frédéric Buclin]

I backed out bug 697224. Only Bugzilla 4.3.2 had this code, so we are fine here. So this bug now focuses on the disclose of confidential groups only.

Comment 2

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #0)
> Bugzilla::Group->check_no_disclose() must be used instead of Bugzilla::Group->check().

Actually, I simply had to fix the error ID passed to check(). check_no_disclose() must only be used for bugs.

Comment 3

[Frédéric Buclin]

Attachment: patch for 4.4 + trunk, v1

It's now illegal to pass a group name if you don't belong to this group (to not disclose if this group exists or not). It's still fine to pass a group ID you don't belong to, though, because its name is not disclosed.

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 669185 [details] [diff] [review]
patch for 4.4 + trunk, v1

Review of attachment 669185 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good and works as expected. r=dkl

::: Bugzilla/WebService/User.pm
@@ +889,5 @@
>  
> +=item 804 (Invalid Group Name)
> +
> +You passed a group name which doesn't exist or which you don't belong to
> +in the C<groups> argument.

Nit: may sound better this way

You passed a group name in the C<groups> argument which either does not exist or you do not belong to it.

I am fine either way.

Comment 5

[Frédéric Buclin]

I need to backport it to 4.2 and 4.0 as the patch doesn't apply cleanly to these older branches.

Comment 6

[Frédéric Buclin]

Attachment: patch for 4.2, v1

Patch for 4.2.x only.

Comment 7

[Frédéric Buclin]

Attachment: patch for 4.0, v1

Patch for 4.0.x only. The only difference compared to the patch for 4.2 is in Constants.pm.

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 671395 [details] [diff] [review]
patch for 4.2, v1

Review of attachment 671395 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 671396 [details] [diff] [review]
patch for 4.0, v1

Review of attachment 671396 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 10

[Frédéric Buclin]

No need for another CVE number for this bug (I think), it's another form of CVE-2011-2380.

Comment 11

[Daniel Veditz [:dveditz]]

You can't re-use year-old CVEs that way, use CVE-2012-4198 instead.

Comment 12

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/WebService/Constants.pm
modified Bugzilla/WebService/User.pm
Committed revision 8468.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified Bugzilla/WebService/Constants.pm
modified Bugzilla/WebService/User.pm
Committed revision 8453.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified Bugzilla/WebService/Constants.pm
modified Bugzilla/WebService/User.pm
Committed revision 8167.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/
modified Bugzilla/WebService/Constants.pm
modified Bugzilla/WebService/User.pm
Committed revision 7733.

Comment 13

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.4/
modified t/webservice_user_get.t
Committed revision 235.

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.2/
modified t/webservice_user_get.t
Committed revision 227.

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.0/
modified t/webservice_user_get.t
Committed revision 208.

Comment 14

[Frédéric Buclin]

Security advisory sent. Removing the security flag.