User.get ignores the "maxusermatches" parameter and allows listing all email addresses

RESOLVED FIXED in Bugzilla 4.4

Status

()

Bugzilla
WebService
--
minor
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: freddyb, Assigned: dkl)

Tracking

({sec-low, wsec-disclosure})

3.3.1
Bugzilla 4.4
sec-low, wsec-disclosure
Bug Flags:
approval +
approval4.4 +

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
As it seems, it is possible to list all email addresses by asking for *.

As the main bugzilla frontend requires three letters until it starts with tab completion, how about requiring the same in the webservice and disallowing wildcards when less than 4 characters are given.
(Reporter)

Comment 1

3 years ago
I understand the disclaimer at https://bugzilla.mozilla.org/createaccount.cgi which explains that bugzilla is a public place and that email addresses are visible to everyone. But that shouldn't make it too easy to list everyone, should it?
Someone else may have a definitive answer, but I believe the view was that no spammer is going to write a specific crawler for our API, and if someone goes to that effort, there are lots of ways of getting all or almost all the email addresses from a Bugzilla.

Gerv
I don't think you need a crawler. A single "wget" suffices, you don't even need multiple requests as far as I can see.
(Reporter)

Comment 4

3 years ago
(In reply to Gervase Markham [:gerv] from comment #2)
> here are lots of ways of getting all or almost all the
> email addresses from a Bugzilla.

There's no point in making it too easy though :)

Comment 5

3 years ago
(In reply to Frederik Braun [:freddyb] from comment #0)
> As it seems, it is possible to list all email addresses by asking for *.

To which method and argument do you pass "*"? User.get({ match => '*' })? You can only call this method with the "match" parameter if you are logged in. Logged out users get an error.
Severity: normal → minor
OS: Linux → All
Hardware: x86_64 → All
(In reply to Frédéric Buclin from comment #5)
> (In reply to Frederik Braun [:freddyb] from comment #0)
> > As it seems, it is possible to list all email addresses by asking for *.
> 
> To which method and argument do you pass "*"? User.get({ match => '*' })?
> You can only call this method with the "match" parameter if you are logged
> in. Logged out users get an error.

That's right. I don't see a big difference between being logged in and out here though because everyone can create an account in seconds, and doesn't need any specific privileges in order to execute the match '*' query.
(Assignee)

Comment 7

3 years ago
I vote that having a 3-4 character requirement for pattern length is not a bad idea and not a difficult change.

+1

dkl

Comment 8

3 years ago
(In reply to Christian Holler (:decoder) from comment #6)
> That's right. I don't see a big difference between being logged in and out
> here though because everyone can create an account in seconds

You have bmo in mind. Some installations do not let you create new accounts yourself, or require some given domain such as @company.com, which prevents this abuse.


(In reply to David Lawrence [:dkl] from comment #7)
> I vote that having a 3-4 character requirement for pattern length is not a
> bad idea and not a difficult change.

I don't see why we should be more restrictive with User.get than what we are from the web UI. From the web UI, you can already get all user accounts by putting "*" in a user field. The difference is that the web UI follows the "maxusermatches" parameter and only returns as many users as specified in this parameter while User.get ignores it and returns all users. So the right fix is to force User.get() to limit the number of accounts it returns, not to enforce some minimum length which is of not help anyway. One could easily pass .com, .net, .org, .au, .fr, etc... to get all addresses they want (the number of possibilities is small), and all these strings are 3-4 characters length.

So I don't consider this bug as a security bug. It's rather a security enhancement. And FYI, I already disclosed this problem publicly in bug 863093.
Summary: Bugzilla API allows listing all email addresses using wildcards → User.get({match => "*"}) ignores the "maxusermatches" parameter and allows listing all email addresses
Target Milestone: --- → Bugzilla 4.4
Version: unspecified → 3.3.1

Comment 9

3 years ago
Oh, and another reason why it's not a security bug: User.get accepts an "ids" parameter, which lets you enumerate all user IDs. So you can loop over all integers till you get undef and know you reached the end of the list. So wildcards just make things a bit easier to get, but is not different.
Group: bugzilla-security
Summary: User.get({match => "*"}) ignores the "maxusermatches" parameter and allows listing all email addresses → User.get ignores the "maxusermatches" parameter and allows listing all email addresses
(Assignee)

Comment 10

3 years ago
Taking
Assignee: webservice → dkl
Status: NEW → ASSIGNED
(Reporter)

Comment 11

3 years ago
(In reply to Frédéric Buclin from comment #9)
> Oh, and another reason why it's not a security bug: User.get accepts an
> "ids" parameter, which lets you enumerate all user IDs. So you can loop over
> all integers till you get undef and know you reached the end of the list. So
> wildcards just make things a bit easier to get, but is not different.

Totally correct, Frédéric! This is not about getting something you wouldn't get otherwise. It's just about raising the bar. It would, for example, be easy to defend against abusive API consumers that have to do hundreds or thousands of requests. But there's no point in that if you can get all email addresses with just one request ;)


Thank you for taking the bug, David! :)
(Assignee)

Comment 12

3 years ago
Created attachment 8363758 [details] [diff] [review]
199516_1.patch
Attachment #8363758 - Flags: review?(LpSolit)

Comment 13

3 years ago
Comment on attachment 8363758 [details] [diff] [review]
199516_1.patch

No idea what this patch is about, but it's unrelated to this bug. :)
Attachment #8363758 - Attachment is obsolete: true
Attachment #8363758 - Flags: review?(LpSolit)
(Assignee)

Comment 14

3 years ago
Created attachment 8363899 [details] [diff] [review]
962060_1.patch

basically we were not honoring Bugzilla->params->{maxusermatch} at all so that is fixes. Also using that value for 'ids' as well as LpSolit pointed out someone could just feed in all numbers 1 to whatever and accomplish the same (as long as they are logged in).

dkl
Attachment #8363899 - Flags: review?(LpSolit)

Comment 15

3 years ago
Comment on attachment 8363899 [details] [diff] [review]
962060_1.patch

>+    if ($params->{ids}) {
>+        my @ids = sort { $a <=> $b } @{$params->{ids}};
>+        splice(@ids, $limit) if $limit;

It doesn't make sense to limit the number of users we return when passing 'ids', for two reasons: the client can easily loop over each interger when calling User.get() till it gets undef. So trying to limit the number of users returned here doesn't prevent any abuse if that's the intention of the caller. And as its name and description say, the 'maxusermatches' parameter is only used for user matching. So the right fix IMO is to simply replace

 $params->{'maxusermatches'}

by

 Bugzilla->params->{'maxusermatches'}

in the existing code.
Attachment #8363899 - Flags: review?(LpSolit) → review-
(In reply to Frédéric Buclin from comment #15)
> So the right fix IMO is to simply replace
>  $params->{'maxusermatches'}
> by
>  Bugzilla->params->{'maxusermatches'}
> in the existing code.

it would be better to select the lesser of these two values.
(Assignee)

Comment 17

3 years ago
Created attachment 8366959 [details] [diff] [review]
962060_2.patch
Attachment #8363899 - Attachment is obsolete: true
Attachment #8366959 - Flags: review?(LpSolit)

Comment 18

3 years ago
Comment on attachment 8366959 [details] [diff] [review]
962060_2.patch

>+    if (defined $params->{maxusermatches}) {

We shouldn't allow this user parameter to be 0, because 0 means "no limit" and it would always win below. So remove "defined".


>+        $limit = $params->{maxusermatches} if $params->{maxusermatches} < $limit;

If the (system) parameter is set to 0 (no limit), this code won't let you limit the number of returned values. You probably want:

  $limit = $limit ? min($params->{maxusermatches}, $limit) : $params->{maxusermatches};

min() is available from List::Util, which you have to import explicitly.
Attachment #8366959 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 19

3 years ago
Created attachment 8379024 [details] [diff] [review]
962060_3.patch
Attachment #8366959 - Attachment is obsolete: true
Attachment #8379024 - Flags: review?(LpSolit)

Comment 20

3 years ago
Comment on attachment 8379024 [details] [diff] [review]
962060_3.patch

>+    if ($params->{maxusermatches}) {

Now that we officially list this parameter, I think it should be renamed to something more readable. maxusermatches is really an internal name. Why not max_user_matches?


>+            || ThrowCodeError('param_must_be_numeric',

This new error (error code 52) is not listed in the list of errors returned by this method.


Otherwise looks good and works fine. r=LpSolit with the fixes above.
Attachment #8379024 - Flags: review?(LpSolit) → review+

Updated

3 years ago
Flags: approval?
Flags: approval4.4?
(Assignee)

Comment 21

3 years ago
Created attachment 8380740 [details] [diff] [review]
962060_4.patch

Changed the externally facing parameter to be 'limit' and made sure the documentation mentions that limit is only for doing user matching. Moving forward r+.
Attachment #8379024 - Attachment is obsolete: true
Attachment #8380740 - Flags: review+
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval+
(Assignee)

Comment 22

3 years ago
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified Bugzilla/WebService/User.pm
Committed revision 8930. 

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/4.4
modified Bugzilla/WebService/User.pm
Committed revision 8661.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.