Closed Bug 533018 Opened 15 years ago Closed 14 years ago

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

Categories

(Bugzilla :: User Accounts, defect)

3.4.4
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.4

People

(Reporter: LpSolit, Assigned: LpSolit)

Details

Attachments

(2 files, 4 obsolete files)

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?
Attached patch patch - v1 (obsolete) — Splinter Review
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)
Flags: blocking3.4.5? → blocking3.4.5+
Comment on attachment 416198 [details] [diff] [review]
patch - v1

That's an extremely clever solution. I just have to test it.
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-
(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.
Attached patch patch - v2 (obsolete) — Splinter Review
Good idea, Fred.
Attachment #416198 - Attachment is obsolete: true
Attachment #419634 - Flags: review?(mkanat)
Well, instead I'd rather just deny logged-out users any form of user-matching whatsoever, which is what we do in other places.
(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.
(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.
(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.
  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 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-
(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.
Flags: blocking3.4.5+ → blocking3.4.6+
Flags: blocking3.6+
<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.
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)
Attachment #426841 - Flags: review?(mkanat) → review-
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.
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)
Attached patch patch for 3.6 and 3.7, v4 (obsolete) — Splinter Review
Same patch but for 3.6 and tip.
Attachment #427332 - Flags: review?(mkanat)
(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
(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 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 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-
(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.
Attachment #427332 - Attachment is obsolete: true
Attachment #427477 - Flags: review?(mkanat)
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 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+
Flags: approval3.6+
Flags: approval3.4+
Flags: approval+
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
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.