Last Comment Bug 533018 - "Confirm match" displays full email address to logged-out users in request.cgi
: "Confirm match" displays full email address to logged-out users in request.cgi
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: User Accounts (show other bugs)
: 3.4.4
: All All
: -- normal (vote)
: Bugzilla 3.4
Assigned To: Frédéric Buclin
: default-qa
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-04 15:58 PST by Frédéric Buclin
Modified: 2010-02-17 16:21 PST (History)
3 users (show)
mkanat: approval+
mkanat: approval3.6+
LpSolit: blocking3.6+
mkanat: approval3.4+
LpSolit: blocking3.4.6+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch - v1 (1.25 KB, patch)
2009-12-04 16:28 PST, Reed Loden [:reed] (use needinfo?)
mkanat: review-
Details | Diff | Review
patch - v2 (2.43 KB, patch)
2009-12-30 18:31 PST, Reed Loden [:reed] (use needinfo?)
mkanat: review-
Details | Diff | Review
Check for $user->id before returning @users (781 bytes, patch)
2010-02-13 11:35 PST, Jonathan Martens
LpSolit: review-
Details | Diff | Review
patch for 3.4, v4 (2.11 KB, patch)
2010-02-17 07:32 PST, Frédéric Buclin
mkanat: review+
Details | Diff | Review
patch for 3.6 and 3.7, v4 (2.28 KB, patch)
2010-02-17 07:38 PST, Frédéric Buclin
mkanat: review-
Details | Diff | Review
patch for 3.6 and 3.7, v4.1 (2.26 KB, patch)
2010-02-17 15:53 PST, Frédéric Buclin
mkanat: review+
Details | Diff | Review

Description Frédéric Buclin 2009-12-04 15:58:44 PST
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] (use needinfo?) 2009-12-04 16:28:41 PST
Created attachment 416198 [details] [diff] [review]
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 2009-12-10 15:45:52 PST
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 2009-12-17 13:56:26 PST
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 2009-12-30 11:32:44 PST
(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] (use needinfo?) 2009-12-30 18:31:00 PST
Created attachment 419634 [details] [diff] [review]
patch - v2

Good idea, Fred.
Comment 6 Max Kanat-Alexander 2009-12-31 02:20:46 PST
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 2009-12-31 04:00:27 PST
(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 2009-12-31 04:22:08 PST
(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 2010-01-20 15:38:53 PST
(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 2010-01-21 17:12:50 PST
  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 2010-01-31 09:20:30 PST
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 2010-01-31 09:31:42 PST
(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 2010-02-11 13:32:58 PST
<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 2010-02-13 11:35:41 PST
Created attachment 426841 [details] [diff] [review]
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 2010-02-17 06:46:38 PST
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 2010-02-17 07:32:03 PST
Created attachment 427331 [details] [diff] [review]
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 2010-02-17 07:38:02 PST
Created attachment 427332 [details] [diff] [review]
patch for 3.6 and 3.7, v4

Same patch but for 3.6 and tip.
Comment 18 Jonathan Martens 2010-02-17 11:05:38 PST
(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 2010-02-17 11:14:50 PST
(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 2010-02-17 15:32:27 PST
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 2010-02-17 15:33:37 PST
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 2010-02-17 15:35:41 PST
(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 2010-02-17 15:53:10 PST
Created attachment 427477 [details] [diff] [review]
patch for 3.6 and 3.7, v4.1
Comment 24 Max Kanat-Alexander 2010-02-17 16:07:35 PST
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 2010-02-17 16:08:09 PST
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 2010-02-17 16:21:02 PST
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/User.pm
modified template/en/default/global/confirm-user-match.html.tmpl
Committed revision 7004.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.6/
modified Bugzilla/User.pm
modified template/en/default/global/confirm-user-match.html.tmpl
Committed revision 6982.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.4/
modified Bugzilla/User.pm
modified template/en/default/global/confirm-user-match.html.tmpl
Committed revision 6727.

Note You need to log in before you can comment on or make changes to this bug.