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.
Bug 18616 makes it worst by enabling cardnumber
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 ) ) {
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?
This code is going to be error-prone, it looks like there is unnecessary complexity in this sequence of conditions.
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.
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
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 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.
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>
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>
# 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
(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.
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...
(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.
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.
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.
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>
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.
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>
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
(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.
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.
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.
Back to PQA where it came from.
Pushed to master for 17.11, thanks to everybody involved!
These patches have been pushed to 16.11.x and will be in 16.11.13.
These patches have been pushed to 17.05.x and are in 17.05.05.
Pushed to 16.05.x, for 16.05.18 release