533018 - "Confirm match" displays full email address to logged-out users in request.cgi

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Frédéric Buclin]

A logged-out user can use request.cgi to collect email addresses very easily. Typing a partial email address will trigger the "confirm match" intermediate page with a list of users with their full email address. As our new policy is to not display full email addresses to logged-out users, this is a data leak.

Comment 1

[Reed Loden [:reed]]

Attachment: patch - v1

Would something like this work? It's untested, but it should just skip the confirm page completely if the user is not logged-in.

Comment 2

[Max Kanat-Alexander]

Comment on attachment 416198 [details] [diff] [review]
patch - v1

That's an extremely clever solution. I just have to test it.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 416198 [details] [diff] [review]
patch - v1

Okay, here's the problem--type "gmail.com" in the Requester or Requestee field, and lots of users are suddenly returned, and now I know their email address. I don't think that spam spiders are going to do that, but it doesn't mean that we shouldn't be thorough about it if we can (and if it's not too much work).

Comment 4

[Frédéric Buclin]

(In reply to comment #3)
> Okay, here's the problem--type "gmail.com" in the Requester or Requestee field,
> and lots of users are suddenly returned, and now I know their email address.

The workaround seems pretty easy to me, and is really not invasive. In match() in User.pm, write the following lines right after the definition of $wildstr:

if (!$user->id) {
    $wildstr = "*$wildstr*\@*" if $wildstr !~ /\@\*?$/;
}

This will trigger the wildcard search if the string doesn't already ends with @, and will force @ to be *after* the string passed by the user. So a logged out user won't be able to catch email addresses ending with @gmail.com. This line will also make the exact search and the substring search ineffective if the user didn't specify @ at the end of the string, as desired.

Comment 5

[Reed Loden [:reed]]

Attachment: patch - v2

Good idea, Fred.

Comment 6

[Max Kanat-Alexander]

Well, instead I'd rather just deny logged-out users any form of user-matching whatsoever, which is what we do in other places.

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> Well, instead I'd rather just deny logged-out users any form of user-matching
> whatsoever, which is what we do in other places.

The workaround is easy and well-localized enough to take it. I'm not sure which other places you are talking about.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> The workaround is easy and well-localized enough to take it. I'm not sure which
> other places you are talking about.

  The WebService.

Comment 9

[Frédéric Buclin]

(In reply to comment #8)
>   The WebService.

Except that WS has no GUI. Turning off user matching in 3.4 seems a bit too drastic to me, compared to 3.2 and older.

Comment 10

[Max Kanat-Alexander]

  Mmm, well, it's only for logged-out users. I think we should be consistent about not allowing user-matching if you're logged out, under any circumstances. I think that requests.cgi is probably mostly going to be being used by logged-in users anyway--I can't imagine many critical uses of it that involve logged-out users.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 419634 [details] [diff] [review]
patch - v2

Okay, so this is r- based on my previous comments--I don't wan to allow usermatching at all for logged-out users.

Comment 12

[Frédéric Buclin]

(In reply to comment #11)
> Okay, so this is r- based on my previous comments--I don't wan to allow
> usermatching at all for logged-out users.

This still seems too drastic to me, and I still see no reason to turn it off for logged out users.

Comment 13

[Frédéric Buclin]

<mkanat> See, if we simply deny usermatching, there's *no* chance of spammers using that interface to find email addresses.
<mkanat> If we enable it but it's somehow inconsistent with how usermatching works while logged in, that's just confusing.
<LpSolit> good point (about your last sentence)
<mkanat> Thanks.
***LpSolit looses

So just disable user matching for logged out users.

Comment 14

[Jonathan Martens]

Attachment: Check for $user->id before returning @users

This patch first checks for $user->id in match() in User.pm.

If not set it will return the empty @user hash (which is created just before it), if set the current logic will apply and matching is performed.

Comment 15

[Frédéric Buclin]

Comment on attachment 426841 [details] [diff] [review]
Check for $user->id before returning @users

>+   # Bail out when not logged in
>+   return \@users unless $user->id;

No, this doesn't work. Now you get errors all the time, even when you type the full email address of a user.

Comment 16

[Frédéric Buclin]

Attachment: patch for 3.4, v4

This patch is for the 3.4 branch. The confirmation page must still be displayed, as the user must understand what's going on. If we don't do this, the string entered by the user is silently skipped and you get the wrong data (the user would not understand why he gets more data than expected, as he would note that Bugzilla didn't restrict the results to the user email address he entered).

Comment 17

[Frédéric Buclin]

Attachment: patch for 3.6 and 3.7, v4

Same patch but for 3.6 and tip.

Comment 18

[Jonathan Martens]

(In reply to comment #15)
> (From update of attachment 426841 [details] [diff] [review])
> >+   # Bail out when not logged in
> >+   return \@users unless $user->id;
> 
> No, this doesn't work. Now you get errors all the time, even when you type the
> full email address of a user.

Strange, not in my test setup. Are you sure you are logged in?

If not, you might see errors indeed as the form will always return no user and hence the message:

Requester:  	
??? did not match anything 

or 

Requestee:  	
??? did not match anything

Comment 19

[Frédéric Buclin]

(In reply to comment #18)
> Strange, not in my test setup. Are you sure you are logged in?

I'm not, which is the purpose of the bug.


> If not, you might see errors indeed as the form will always return no user and
> hence the message:

That's not what I want when I enter the exact email address. In that case, it should succeed.

Comment 20

[Max Kanat-Alexander]

Comment on attachment 427332 [details] [diff] [review]
patch for 3.6 and 3.7, v4

Hmm. I wonder, would it be simpler to just have it call Bugzilla::User->check on the list, if you're logged out?

Comment 21

[Max Kanat-Alexander]

Comment on attachment 427332 [details] [diff] [review]
patch for 3.6 and 3.7, v4

  In any case, the functionality is generally OK, but:

>+    [% IF !user.id %]
>+      Note that user matching is disabled as you are currently logged out.
>+      To enable this feature, you must first log in.
>+    [% END %]

  That message is a little hidden and most users won't know what "user matching" is, because that's a term that only the admin sees. So we need a better sentence than that.

Comment 22

[Frédéric Buclin]

(In reply to comment #20)
> (From update of attachment 427332 [details] [diff] [review])
> Hmm. I wonder, would it be simpler to just have it call Bugzilla::User->check
> on the list, if you're logged out?

If we replace the "exact match" part by this, this would be done as a separate bug, for 3.7 only.

Comment 23

[Frédéric Buclin]

Attachment: patch for 3.6 and 3.7, v4.1

Comment 24

[Max Kanat-Alexander]

Comment on attachment 427477 [details] [diff] [review]
patch for 3.6 and 3.7, v4.1

Cool. At the start of the bold part, add "Note: " on checkin.

Comment 25

[Max Kanat-Alexander]

Comment on attachment 427331 [details] [diff] [review]
patch for 3.4, v4

This is fine, though the template part from the HEAD patch needs to come forward.

Comment 26

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/User.pm
modified template/en/default/global/confirm-user-match.html.tmpl
Committed revision 7004.

Committing to: bzr+ssh://[e-mail]/bugzilla/3.6/
modified Bugzilla/User.pm
modified template/en/default/global/confirm-user-match.html.tmpl
Committed revision 6982.

Committing to: bzr+ssh://[e-mail]/bugzilla/3.4/
modified Bugzilla/User.pm
modified template/en/default/global/confirm-user-match.html.tmpl
Committed revision 6727.