Bug 18956 - Possible privacy breach with OPAC password recovery
Summary: Possible privacy breach with OPAC password recovery
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: M. Tompsett
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 18653 15585
Blocks:
  Show dependency treegraph
 
Reported: 2017-07-19 11:18 UTC by Fridolin SOMERS
Modified: 2017-11-08 08:22 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18956: Password recovery - Do not disclose email if not given (1.92 KB, patch)
2017-07-19 16:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18956: Leaking username/cardnumber or email (5.43 KB, patch)
2017-08-15 16:22 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 18956: Password recovery - Do not disclose email if not given (1.98 KB, patch)
2017-08-19 10:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 18956: Leaking username/cardnumber or email (4.97 KB, patch)
2017-08-19 10:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 18956: Counter patch - prevent leaking during password recovery (5.26 KB, patch)
2017-08-25 19:28 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18956: Counter patch - prevent leaking during password recovery (5.48 KB, patch)
2017-08-25 19:37 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18956: Prevent leaking during password recovery (5.56 KB, patch)
2017-09-08 06:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18956: [QA Follow-up] Resolve a CGI::Param in list context warn (1.43 KB, patch)
2017-09-08 06:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18956: Fix empty to in message queue (2.08 KB, patch)
2017-09-13 03:48 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18956: Fix empty to in message queue (2.21 KB, patch)
2017-10-11 12:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin SOMERS 2017-07-19 11:18:39 UTC
If borrower already has a password recovery.
After entering only its login/cadnumber and submitting the page shows its email in form and in link "Get new password recovery link".
Bug 18653 has not corrected this.

So in my opinion, login/cadnumber AND email should be mandatory to allow password recovery. It is too dangerous for privacy and SPAM attacks.

Someone loosing its login/cadnumber or forgetting which email he gave must  directly contact the library.
Comment 1 Fridolin SOMERS 2017-07-19 11:21:14 UTC
Bug 18616 makes it worst by enabling cardnumber
Comment 2 Jonathan Druart 2017-07-19 16:01:59 UTC
The problem comes from this line, $email is the parameter and $firstNonEmptyEmail comes from the DB, I do not understand how this condition can be useful here.

  elsif ( !$email && !( $email = $firstNonEmptyEmail ) ) {
Comment 3 Jonathan Druart 2017-07-19 16:08:43 UTC
Created attachment 65105 [details] [review]
Bug 18956: Password recovery - Do not disclose email if not given

If a patron has started the password recovery process and another user
enter the same login (or cardnumber), without a email address, the
screen will display the email address of the account.

Test plan:
Given an account with
  login:    koha
  password: whatever
  email:    koha@k-c.org
Enable the pref OpacResetPassword
At the OPAC, ask for another password: "Forgot your password?" link
Enter the correct login/email
Assume you are somebody else and retry the process, but do not fill the
email field.
=> Without this patch the email will be displayed on the screen
=> With this patch applied you will get "No account was found with the
provided information."

QA: You will need to understand the condition affected by this patch.
Why was it there? Are we sure we do not introduce a regression with this
patch?
Comment 4 Jonathan Druart 2017-07-19 16:10:10 UTC
This code is going to be error-prone, it looks like there is unnecessary complexity in this sequence of conditions.
Comment 5 M. Tompsett 2017-08-15 14:51:34 UTC
Suggested TEST PLAN

correct login/cardnumber - emailed.
delete from borrower_password_recovery;
correct email - emailed.
delete from borrower_password_recovery;
correct login/cardnumber && correct email -- emailed.
delete from borrower_password_recovery;
wrong login/cardnumber - error page as expected
delete from borrower_password_recovery;
wrong login/cardnumber && correct email -- error page as expected
delete from borrower_password_recovery;
correct login/cardnumber && wrong email -- error page as expected
delete from borrower_password_recovery;
wrong login/cardnumber && wrong email -- error page as expected
delete from borrower_password_recovery;
empty -- INTERNAL SERVER ERROR?!
delete from borrower_password_recovery;
--- None of these displayed an email
correct login/cardnumber - emailed.
correct login/cardnumber - error page as expected, BUT
--LEAKED EMAIL!
delete from borrower_password_recovery;
correct email - emailed.
correct email - error page as expected, BUT
--LEAKED LOGIN/USERID
delete from borrower_password_recovery;

I don't like this patch's solution, as the message should stay the same without leaking. It does not address the INTERNAL SERVER ERROR, nor the leaked login/userid.
Comment 6 M. Tompsett 2017-08-15 16:22:13 UTC
Created attachment 66026 [details] [review]
Bug 18956: Leaking username/cardnumber or email

TEST PLAN

correct login/cardnumber - emailed.
delete from borrower_password_recovery;
correct email - emailed.
delete from borrower_password_recovery;
correct login/cardnumber && correct email -- emailed.
delete from borrower_password_recovery;
wrong login/cardnumber - error page as expected
delete from borrower_password_recovery;
wrong login/cardnumber && correct email -- error page as expected
delete from borrower_password_recovery;
correct login/cardnumber && wrong email -- error page as expected
delete from borrower_password_recovery;
wrong login/cardnumber && wrong email -- error page as expected
delete from borrower_password_recovery;
empty -- INTERNAL SERVER ERROR?!
delete from borrower_password_recovery;
--- None of these displayed an email
correct login/cardnumber - emailed.
correct login/cardnumber - error page as expected, BUT
--LEAKED EMAIL!
delete from borrower_password_recovery;
correct email - emailed.
correct email - error page as expected, BUT
--LEAKED LOGIN/USERID
delete from borrower_password_recovery;

Now apply the patch, and do all the error pages make sense?
Is more information displayed than was entered?
Is there an INTERNAL SERVER ERROR?

Run qa test tools
Comment 7 M. Tompsett 2017-08-15 16:23:29 UTC
I've attached a larger counter patch, apply either the first or second, but not both for testing. Sign off the one that makes the most sense.
Comment 8 M. Tompsett 2017-08-15 16:28:56 UTC
Comment on attachment 66026 [details] [review]
Bug 18956: Leaking username/cardnumber or email

Review of attachment 66026 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-password-recovery.pl
@@ +31,4 @@
>  my $minPassLength  = C4::Context->preference('minPasswordLength');
>  my $id             = $query->param('id');
>  my $uniqueKey      = $query->param('uniqueKey');
> +my $username       = $query->param('username') // q{};

This keeps the grep quiet when username is not passed.

@@ -53,4 @@
>  if ( $query->param('sendEmail') || $query->param('resendEmail') ) {
>  
>      #try with the main email
> -    $email ||= '';    # avoid undef

This was not necessary, there is a // q{} above.

@@ -65,4 @@
>          $search_results = Koha::Patrons->search( { -or => { email => $email, emailpro => $email, B_email  => $email } } );
>      }
>  
> -    if ( not $search_results || $search_results->count < 1) {

For some reason not $search_results was evaluating true, and ->count was triggering errors on undefined.

@@ -78,4 @@
>          $errMultipleAccountsForEmail = 1;
>      }
>      elsif ( $borrower = $search_results->next() ) {    # One matching borrower
> -        $username ||= $borrower->userid;

This leaked username/cardnumber.

@@ -93,5 @@
>              $errNoBorrowerFound = 1;
>          }
>  
> -# If we dont have an email yet. Get one of the borrower's email or raise an error.
> -        elsif ( !$email && !( $email = $firstNonEmptyEmail ) ) {

This assignment leaked email.
Comment 9 Katrin Fischer 2017-08-19 10:19:19 UTC
Created attachment 66221 [details] [review]
[SIGNED OFF] Bug 18956: Password recovery - Do not disclose email if not given

If a patron has started the password recovery process and another user
enter the same login (or cardnumber), without a email address, the
screen will display the email address of the account.

Test plan:
Given an account with
  login:    koha
  password: whatever
  email:    koha@k-c.org
Enable the pref OpacResetPassword
At the OPAC, ask for another password: "Forgot your password?" link
Enter the correct login/email
Assume you are somebody else and retry the process, but do not fill the
email field.
=> Without this patch the email will be displayed on the screen
=> With this patch applied you will get "No account was found with the
provided information."

QA: You will need to understand the condition affected by this patch.
Why was it there? Are we sure we do not introduce a regression with this
patch?

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2017-08-19 10:19:49 UTC
Created attachment 66222 [details] [review]
[SIGNED OFF] Bug 18956: Leaking username/cardnumber or email

TEST PLAN

correct login/cardnumber - emailed.
delete from borrower_password_recovery;
correct email - emailed.
delete from borrower_password_recovery;
correct login/cardnumber && correct email -- emailed.
delete from borrower_password_recovery;
wrong login/cardnumber - error page as expected
delete from borrower_password_recovery;
wrong login/cardnumber && correct email -- error page as expected
delete from borrower_password_recovery;
correct login/cardnumber && wrong email -- error page as expected
delete from borrower_password_recovery;
wrong login/cardnumber && wrong email -- error page as expected
delete from borrower_password_recovery;
empty -- INTERNAL SERVER ERROR?!
delete from borrower_password_recovery;

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Marcel de Rooy 2017-08-25 06:07:02 UTC
# If we dont have an email and one was not provided.
        elsif ( !$email && !$firstNonEmptyEmail ) {
            $hasError           = 1;
            $errNoBorrowerFound = 1;
        }

# If we dont have an email and one was provided.
        elsif ( !$email && $firstNonEmptyEmail ) {
            $hasError           = 1;
            $errAlreadyStartRecovery = 1;
        }

Note that I am not a fan of this if structure. 
Imo the comments are not very clear too.
We dont have an email == !$email (Seems clear, but can be misunderstood)
One was not provided == !$firstNonEmptyMail (Provided here seems to refer to the borrower email addresses in the database. I would think of provided on the form.)
If the user did not enter an email on the form, we must have identified him by username or cardnumber according to the code. If he has an email address, why does this mean that the recovery already started ??

And what about: empty -- INTERNAL SERVER ERROR?!

Please provide some feedback
Comment 12 M. Tompsett 2017-08-25 11:00:47 UTC
(In reply to Marcel de Rooy from comment #11)
[SNIP]
> 
> Note that I am not a fan of this if structure.

Noted.


> Imo the comments are not very clear too.
[SNIP]

Okay, I'll improve them.


> And what about: empty -- INTERNAL SERVER ERROR?!

My COUNTER patch (the second one), doesn't have that issue.
See comment #8 explaining why all the changes.
It might be better to read the comments in context by clicking
review of review to see the obsoleted version with comments.
Comment 13 Katrin Fischer 2017-08-25 11:53:03 UTC
Hm, it wasn't clear to me that yours was a counter patch. I had applied it on top of the other with a small conflict and tested together...
Comment 14 M. Tompsett 2017-08-25 17:11:15 UTC
(In reply to Katrin Fischer from comment #13)
> Hm, it wasn't clear to me that yours was a counter patch. I had applied it
> on top of the other with a small conflict and tested together...

Sorry, that was stated in comment #7.
Comment 15 M. Tompsett 2017-08-25 19:28:44 UTC
Created attachment 66491 [details] [review]
Bug 18956: Counter patch - prevent leaking during password recovery

TEST PLAN
---------

It is assumed you have set the OpacResetPassword to 'allowed',
and likely in combination with OpacPasswordChange to 'Allowed'.

You will have two patrons: one with and another without
any email address entered. You will want to test this test plan
with both patrons.

$ git checkout -b bug_18956 origin/master

Prepend the following as understood between step sections:
opac -> forgot password and then enter...

correct login/cardnumber, it will email
delete from borrower_password_recovery;

correct email, it will email
delete from borrower_password_recovery;

correct login/cardnumber && correct email, it will email
delete from borrower_password_recovery;

wrong login/cardnumber && correct email, error page as expected
delete from borrower_password_recovery;

correct login/cardnumber && wrong email, error page as expected
delete from borrower_password_recovery;

wrong login/cardnumber && wrong email, error page as expected
delete from borrower_password_recovery;

submit empty -- INTERNAL SERVER ERROR?!
delete from borrower_password_recovery;

-- None of the above step sections displayed email.

correct login/cardnumber, it will email

correct login/cardnumber again, but it leaks email address!
delete from borrower_password_recovery;

correct email, it will email

correct email again, but it leaks login/cardnumber!
delete from borrower_password_recovery;

$ git bz apply 18956
-- choose interactive, and choose this counter patch.

repeat the same test set again
-- no leaks will occur, error message pages returned should
   be reasonable, code should read reasonably.

run koha qa test tools.
Comment 16 M. Tompsett 2017-08-25 19:37:18 UTC
Created attachment 66492 [details] [review]
Bug 18956: Counter patch - prevent leaking during password recovery

TEST PLAN
---------

It is assumed you have set the OpacResetPassword to 'allowed',
and likely in combination with OpacPasswordChange to 'Allowed'.

You will have two patrons: one with and another without
any email address entered. You will want to test this test plan
with both patrons.

$ git checkout -b bug_18956 origin/master

Prepend the following as understood between step sections:
opac -> forgot password and then enter...

correct login/cardnumber, it will email
delete from borrower_password_recovery;

correct email, it will email
delete from borrower_password_recovery;

correct login/cardnumber && correct email, it will email
delete from borrower_password_recovery;

wrong login/cardnumber && correct email, error page as expected
delete from borrower_password_recovery;

correct login/cardnumber && wrong email, error page as expected
delete from borrower_password_recovery;

wrong login/cardnumber && wrong email, error page as expected
delete from borrower_password_recovery;

submit empty -- INTERNAL SERVER ERROR?!
delete from borrower_password_recovery;

-- None of the above step sections displayed email.

correct login/cardnumber, it will email

correct login/cardnumber again, but it leaks email address!
delete from borrower_password_recovery;

correct email, it will email

correct email again, but it leaks login/cardnumber!
delete from borrower_password_recovery;

$ git bz apply 18956
-- choose interactive, and choose this counter patch.

repeat the same test set again
-- no leaks will occur, error message pages returned should
   be reasonable, code should read reasonably.

run koha qa test tools.
Comment 17 Marcel de Rooy 2017-09-08 06:43:47 UTC
Created attachment 66972 [details] [review]
Bug 18956: Prevent leaking during password recovery

TEST PLAN
---------

It is assumed you have set the OpacResetPassword to 'allowed',
and likely in combination with OpacPasswordChange to 'Allowed'.

You will have two patrons: one with and another without
any email address entered. You will want to test this test plan
with both patrons.

$ git checkout -b bug_18956 origin/master

Prepend the following as understood between step sections:
opac -> forgot password and then enter...

correct login/cardnumber, it will email
delete from borrower_password_recovery;

correct email, it will email
delete from borrower_password_recovery;

correct login/cardnumber && correct email, it will email
delete from borrower_password_recovery;

wrong login/cardnumber && correct email, error page as expected
delete from borrower_password_recovery;

correct login/cardnumber && wrong email, error page as expected
delete from borrower_password_recovery;

wrong login/cardnumber && wrong email, error page as expected
delete from borrower_password_recovery;

submit empty -- INTERNAL SERVER ERROR?!
delete from borrower_password_recovery;

-- None of the above step sections displayed email.

correct login/cardnumber, it will email

correct login/cardnumber again, but it leaks email address!
delete from borrower_password_recovery;

correct email, it will email

correct email again, but it leaks login/cardnumber!
delete from borrower_password_recovery;

$ git bz apply 18956
-- choose interactive, and choose this counter patch.

repeat the same test set again
-- no leaks will occur, error message pages returned should
   be reasonable, code should read reasonably.

run koha qa test tools.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2017-09-08 06:50:13 UTC
QA Comment:
Looks good to me. The URL does not reveal additional information anymore.
I signed the counterpatch and obsoleted the other one now, adjusted the commit message in this regard.

While testing this, I found two related issues (to be solved on another report):
[1] The link in the email is converted from <a href="link">link</a> to something like:
[/cgi-bin/koha/opac-password-recovery.pl?uniqueKey=$2a$08$RMWYZCr7FywXXXETJO0F2e]/cgi-bin/koha/opac-password-recovery.pl?uniqueKey=$2a$08$RMWYZCr7FywXXXETJO0F2e 
This is probably depending on the mail server used.

[2] If you test with a borrower that has no email, you only get Error (without additional info). Cause? The script passes errNoBorrowerEmail to the template, but the template does not use that variable at all. Should be an easy fix, just think about a appropriate message.
Comment 19 Marcel de Rooy 2017-09-08 06:56:38 UTC
Created attachment 66973 [details] [review]
Bug 18956: [QA Follow-up] Resolve a CGI::Param in list context warn

From the plack-error.log:
CGI::param called in list context from package CGI::Compile::ROOT::usr_share_koha_masterclone_opac_opac_2dpassword_2drecovery_2epl line 129, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Jonathan Druart 2017-09-12 13:53:14 UTC
Something wrong, steps to recreate:

- enter login+email
- enter login only, you get:

Error

The process of password recovery has already been started for this account ("koha")
You should have received an email with a link to reset your password.
If you did not receive this email, you can request a new one: Get new password recovery link 

- Click on "Get new password recovery link"
=> " You will receive an email shortly. "

New letter PASSWORD_RESET added to the queue, but to_address is empty
Comment 21 M. Tompsett 2017-09-12 14:52:53 UTC
(In reply to Jonathan Druart from comment #20)
> Something wrong, steps to recreate:
...

Thanks for the test case to check. I'll get to it.
Comment 22 M. Tompsett 2017-09-13 03:48:06 UTC
Created attachment 67107 [details] [review]
Bug 18956: Fix empty to in message queue

Follow the test plan in comment #20.
Also tweaked string, because it was really 'or' before too.
Comment 23 Marcel de Rooy 2017-10-11 12:20:13 UTC
Created attachment 67952 [details] [review]
Bug 18956: Fix empty to in message queue

Follow the test plan in comment #20.
Also tweaked string, because it was really 'or' before too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended text in added comment.
Comment 24 Marcel de Rooy 2017-10-11 12:20:56 UTC
Back to PQA where it came from.
Comment 25 Jonathan Druart 2017-10-27 17:19:29 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 26 Katrin Fischer 2017-10-29 16:36:32 UTC
These patches have been pushed to 16.11.x and will be in 16.11.13.
Comment 27 Fridolin SOMERS 2017-11-08 08:22:00 UTC
These patches have been pushed to 17.05.x and are in 17.05.05.