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

RESOLVED FIXED in Bugzilla 3.4

Status

()

Bugzilla
User Accounts
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Frédéric Buclin, Assigned: Frédéric Buclin)

Tracking

3.4.4
Bugzilla 3.4
Bug Flags:
approval +
approval3.6 +
blocking3.6 +
approval3.4 +
blocking3.4.6 +

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

8 years ago
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.
Flags: blocking3.4.5?
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.
Assignee: user-accounts → reed
Status: NEW → ASSIGNED
Attachment #416198 - Flags: review?(mkanat)

Updated

8 years ago
Flags: blocking3.4.5? → blocking3.4.5+

Comment 2

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

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

Comment 3

8 years ago
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).
Attachment #416198 - Flags: review?(mkanat) → review-
(Assignee)

Comment 4

8 years ago
(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.
Created attachment 419634 [details] [diff] [review]
patch - v2

Good idea, Fred.
Attachment #416198 - Attachment is obsolete: true
Attachment #419634 - Flags: review?(mkanat)

Comment 6

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

Comment 7

8 years ago
(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

8 years ago
(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.
(Assignee)

Comment 9

8 years ago
(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

8 years ago
  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

8 years ago
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.
Attachment #419634 - Flags: review?(mkanat) → review-
(Assignee)

Comment 12

8 years ago
(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.
(Assignee)

Updated

8 years ago
Flags: blocking3.4.5+ → blocking3.4.6+
(Assignee)

Updated

8 years ago
Flags: blocking3.6+
(Assignee)

Comment 13

8 years ago
<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

8 years ago
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.
Attachment #426841 - Flags: review?(mkanat)
(Assignee)

Updated

8 years ago
Attachment #426841 - Flags: review?(mkanat) → review-
(Assignee)

Comment 15

8 years ago
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.
(Assignee)

Comment 16

8 years ago
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).
Assignee: reed → LpSolit
Attachment #419634 - Attachment is obsolete: true
Attachment #426841 - Attachment is obsolete: true
Attachment #427331 - Flags: review?(mkanat)
(Assignee)

Comment 17

8 years ago
Created attachment 427332 [details] [diff] [review]
patch for 3.6 and 3.7, v4

Same patch but for 3.6 and tip.
Attachment #427332 - Flags: review?(mkanat)

Comment 18

8 years ago
(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
(Assignee)

Comment 19

8 years ago
(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

8 years ago
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

8 years ago
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.
Attachment #427332 - Flags: review?(mkanat) → review-
(Assignee)

Comment 22

8 years ago
(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.
(Assignee)

Comment 23

8 years ago
Created attachment 427477 [details] [diff] [review]
patch for 3.6 and 3.7, v4.1
Attachment #427332 - Attachment is obsolete: true
Attachment #427477 - Flags: review?(mkanat)

Comment 24

8 years ago
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.
Attachment #427477 - Flags: review?(mkanat) → review+

Comment 25

8 years ago
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.
Attachment #427331 - Flags: review?(mkanat) → review+

Updated

8 years ago
Flags: approval3.6+
Flags: approval3.4+
Flags: approval+
(Assignee)

Comment 26

8 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.