Bug 27812

Summary: Remove the ability to transmit a patron's plain text password over email
Product: Koha Reporter: Kyle M Hall <kyle>
Component: PatronsAssignee: Kyle M Hall <kyle>
Status: Pushed to stable --- QA Contact: Martin Renvoize <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: amitddng135, andrewfh, dcook, fridolin.somers, gmcharlt, jonathan.druart+koha, karen, kyle.m.hall, martin.renvoize
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25936
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30442
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30611
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This bugfix/enhancement improves the default security of Koha by removing the pass of the plain text password to the ACCTDETAILS notice on patron creation. WARNING: You will need to update your notice template if you were relying on `<<borrowers.password>>` in this notice.
Version(s) released in:
22.05.00,21.11.05
Bug Depends on:    
Bug Blocks: 17648, 30611, 18543    
Attachments: Bug 27812: Remove the ability to transmit a patron's plain text password over email
Bug 27812: Remove the ability to transmit a patron's plain text password over email
Bug 27812: Remove the ability to transmit a patron's plain text password over email
Bug 27812: Don't require password in data to trigger email
Bug 27812: Remove the ability to transmit a patron's plain text password over email
Bug 27812: Email should send for all new patron as per the syspref description
Bug 27812: Add DBRev to warn of the change to the account details notice
Bug 27812: Remove the ability to transmit a patron's plain text password over email
Bug 27812: Email should send for all new patron as per the syspref description
Bug 27812: Add DBRev to warn of the change to the account details notice
Bug 27812: (QA follow-up) Fix atomicupdate

Description Kyle M Hall 2021-02-26 18:16:48 UTC
We should not give libraries the ability to compromise patron accounts, it is considered a huge security issue and nobody in network security would ever recommend allowing passwords to be transmitted in clear text over email.  It should simply not be possible to send a patron's password in plain text via email. As such, we should remove this ability from Koha.
Comment 1 Kyle M Hall 2021-02-26 18:19:44 UTC
Created attachment 117395 [details] [review]
Bug 27812: Remove the ability to transmit a patron's plain text password over email

We should not give libraries the ability to compromise patron accounts, it is considered a huge security issue and nobody in network security would ever recommend allowing passwords to be transmitted in clear text over email.  It should simply not be possible to send a patron's password in plain text via email. As such, we should remove this ability from Koha.

Test Plan:
1) Apply this patch
2) Create a patron to generate the ACCTDETAILS email
3) Note you can no longer transmit the patron's password in the email
Comment 2 Owen Leonard 2021-03-16 13:14:33 UTC
(In reply to Kyle M Hall from comment #1)

> 2) Create a patron to generate the ACCTDETAILS email

Under what circumstances is this notice generated?
Comment 3 Katrin Fischer 2021-03-16 22:20:54 UTC
(In reply to Owen Leonard from comment #2)
> (In reply to Kyle M Hall from comment #1)
> 
> > 2) Create a patron to generate the ACCTDETAILS email
> 
> Under what circumstances is this notice generated?

You need to activate AutoEmailOpacUser first (I hope i got that one right).
Then when you create the account, you need to fill in:
- name
- userid
- cardnumber
- password
Before saving for the first time (IIRC). It might be a field too many :)

The message doesn't appear in the message queue, so you need a system set up for emailing in order to see this change.
Comment 4 Amit Gupta 2021-03-24 03:24:10 UTC
Created attachment 118708 [details] [review]
Bug 27812: Remove the ability to transmit a patron's plain  text password over email

Patch looks good and agree with Kyle.

Test Plan:
1) Apply this patch
2) Create a patron to generate the ACCTDETAILS email
3) Note you can no longer transmit the patron's password in the email
Comment 5 Martin Renvoize 2021-03-25 13:45:10 UTC
Created attachment 118797 [details] [review]
Bug 27812: Remove the ability to transmit a patron's plain text password over email

We should not give libraries the ability to compromise patron accounts, it is considered a huge security issue and nobody in network security would ever recommend allowing passwords to be transmitted in clear text over email.  It should simply not be possible to send a patron's password in plain text via email. As such, we should remove this ability from Koha.

Test Plan:
1) Apply this patch
2) Create a patron to generate the ACCTDETAILS email
3) Note you can no longer transmit the patron's password in the email

Signed-off-by: Amit Gupta <amitddng135@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2021-03-25 13:45:35 UTC
Good call, works as expected. Passing QA
Comment 7 Jonathan Druart 2021-04-01 16:01:08 UTC
We are removing the password from the notice template for new installations only, for existing installations the generated notices will be "password: ". I don't think we should remove the password key in the controller script.
Comment 8 Kyle M Hall 2021-04-01 18:06:24 UTC
(In reply to Jonathan Druart from comment #7)
> We are removing the password from the notice template for new installations
> only, for existing installations the generated notices will be "password: ".
> I don't think we should remove the password key in the controller script.

I disagree. This is a security bug. We should not allow patron's security to be compromised in the future just because we've allowed it to be compromised in the past.
Comment 9 Kyle M Hall 2021-04-01 18:07:44 UTC
(In reply to Kyle M Hall from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > We are removing the password from the notice template for new installations
> > only, for existing installations the generated notices will be "password: ".
> > I don't think we should remove the password key in the controller script.
> 
> I disagree. This is a security bug. We should not allow patron's security to
> be compromised in the future just because we've allowed it to be compromised
> in the past.

Would you like me to add a follow-up to modify the templates in the database?
Comment 10 Katrin Fischer 2021-04-02 14:39:57 UTC
> Would you like me to add a follow-up to modify the templates in the database?

I think this is meant well but won't really work for translated notices - so we can't really fix this automatically. 

But I agree with Kyle about the need to remove this 'non-feature', so we just need to figure out how to do it best.

- Change the default notice templates - check!
- Are we sure we will remove all translated installers? Then they can be ignored.
- Deal with existing installations using the feature
  - Add well visible note to the release notes
  - Make sure the notices are still processed, just password will not be output
  - Add a note to the pref?
  - Remove password from field list to add to a notice template?
Comment 11 Katrin Fischer 2021-04-02 14:42:07 UTC
Another thing: I believe you currently need to fill in password in order to have the notice generated at all - we should remove that check as well if I am correct.

Thinking about this line:

https://git.koha-community.org/Koha-community/Koha/src/commit/02d82b142d998f1544fbab6e7f35f6f0fceef084/members/memberentry.pl#L468
Comment 12 Kyle M Hall 2021-04-05 12:31:02 UTC
(In reply to Katrin Fischer from comment #11)
> Another thing: I believe you currently need to fill in password in order to
> have the notice generated at all - we should remove that check as well if I
> am correct.
> 
> Thinking about this line:
> 
> https://git.koha-community.org/Koha-community/Koha/src/commit/
> 02d82b142d998f1544fbab6e7f35f6f0fceef084/members/memberentry.pl#L468

Good catch!
Comment 13 Kyle M Hall 2021-04-05 12:34:23 UTC
Created attachment 119176 [details] [review]
Bug 27812: Don't require password in data to trigger email
Comment 14 Jonathan Druart 2021-04-06 09:59:21 UTC
(In reply to Katrin Fischer from comment #10)
> > Would you like me to add a follow-up to modify the templates in the database?
> 
> I think this is meant well but won't really work for translated notices - so
> we can't really fix this automatically. 
> 
> But I agree with Kyle about the need to remove this 'non-feature', so we
> just need to figure out how to do it best.
> 
> - Change the default notice templates - check!
> - Are we sure we will remove all translated installers? Then they can be
> ignored.

We won't remove all the translated installer files for 21.05, and this patch may be backported. We must fix all the .sql files.

> - Deal with existing installations using the feature
>   - Add well visible note to the release notes

Indeed.

>   - Make sure the notices are still processed, just password will not be
> output
>   - Add a note to the pref?
>   - Remove password from field list to add to a notice template?

And what about a warning during the upgrade process?
Comment 15 Katrin Fischer 2021-05-29 14:05:07 UTC
A little sad this missed release, got out of my view too. Boldly marking it as 21.11 candidate - I really believe this one needs to go.
Comment 16 David Cook 2021-05-31 01:56:17 UTC
(In reply to Kyle M Hall from comment #8)
> I disagree. This is a security bug. We should not allow patron's security to
> be compromised in the future just because we've allowed it to be compromised
> in the past.

+1
Comment 17 Katrin Fischer 2021-05-31 09:38:38 UTC
Also made a note about this one on the road map - please add your names so we can sure this makes it into next release together.
Comment 18 Jonathan Druart 2021-06-03 08:16:20 UTC
Existing notice templates still need to be adjusted.

If we cannot adjust them (because modified or translated) we should add a warning if "borrowers.password" or "patron.password" exists.
Comment 19 Martin Renvoize 2021-10-19 13:52:36 UTC
I actually liked that we triggered on password change.. it meant you could write a notice to warn users than a password change had taken place and that if it wasn't them they should contact the library ;).. I realise that's it's own use case, but it would be nice to somehow account for that somewhere.. perhaps a new notice..
Comment 20 Katrin Fischer 2021-10-23 13:51:30 UTC
(In reply to Martin Renvoize from comment #19)
> I actually liked that we triggered on password change.. it meant you could
> write a notice to warn users than a password change had taken place and that
> if it wasn't them they should contact the library ;).. I realise that's it's
> own use case, but it would be nice to somehow account for that somewhere..
> perhaps a new notice..

I don't think this notice triggered on password change - but it's a little hard to tell as it's not in the databases. The only time I got it to sent a notice was when first adding a patron and I believe that's how it's documented as well. But a notice on password change might be a nice addition?

Will see if I can revive this one.
Comment 21 Katrin Fischer 2022-02-26 21:52:57 UTC
I'd really like to see this moving, trying to summarize:

1) Triggering the notice

(In reply to Martin Renvoize from comment #19)
> I actually liked that we triggered on password change.. 

I have had another look, I think the change is that now it will trigger when the userid is changed:

Before:
if ( C4::Context->preference("AutoEmailOpacUser") == 1 && $newdata{'userid'}  && $newdata{'password'}) {

After:
if ( C4::Context->preference("AutoEmailOpacUser") == 1 && $newdata{'userid'}) {

This might be out of scope here, but it seems a little odd before and after the patch. We might want to have a more solid way of triggering this. My suggestion would be to match what we have 'on the tin' and what has been the source of a lot of confusion over the years: 

"Send/Don't send an email to newly created patrons with their account details." 

* Email when a new user is added/saved. This would also resolve bug 18543 about not being able to send the auto-generated userid.
* Email when a librarian triggers the notice. We could add a button to do that. 

This would also resolve bug 9097.


2) Updating existing notice templates

(In reply to Jonathan Druart from comment #18)
> Existing notice templates still need to be adjusted.
> If we cannot adjust them (because modified or translated) we should add a
> warning if "borrowers.password" or "patron.password" exists.

I think there are possibly too many variations in the templates to auto-update them. Where should we add the warning? System information tab? The database update?

3) Translated installers

> We won't remove all the translated installer files for 21.05, and this patch may be backported. We must fix all the .sql files.

True. I could provide a patch for this, but the others are a little too tricky for me.

4) Move notices to message queue

This is a totally separate bug, but it will be so great: Once this is in we can move to message_queue and we will exactly know when or when not a notice was sent (bug 17648)
Comment 22 Kyle M Hall 2022-02-28 14:08:07 UTC
Created attachment 131166 [details] [review]
Bug 27812: Remove the ability to transmit a patron's plain text password over email

We should not give libraries the ability to compromise patron accounts, it is considered a huge security issue and nobody in network security would ever recommend allowing passwords to be transmitted in clear text over email.  It should simply not be possible to send a patron's password in plain text via email. As such, we should remove this ability from Koha.

Test Plan:
1) Apply this patch
2) Create a patron to generate the ACCTDETAILS email
3) Note you can no longer transmit the patron's password in the email

Signed-off-by: Amit Gupta <amitddng135@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Kyle M Hall 2022-02-28 14:08:18 UTC
Created attachment 131167 [details] [review]
Bug 27812: Email should send for all new patron as per the syspref description
Comment 24 Kyle M Hall 2022-02-28 14:08:22 UTC
Created attachment 131168 [details] [review]
Bug 27812: Add DBRev to warn of the change to the account details notice
Comment 25 Katrin Fischer 2022-02-28 14:34:04 UTC
Oh, I had missed the updates here!! Thx Kyle!
Comment 26 Martin Renvoize 2022-03-01 16:01:15 UTC
Created attachment 131194 [details] [review]
Bug 27812: Remove the ability to transmit a patron's plain text password over email

We should not give libraries the ability to compromise patron accounts,
it is considered a huge security issue and nobody in network security
would never recommend allowing passwords to be transmitted in clear text
over email.

It should simply not be possible to send a patron's password in plain text
via email. As such, we should remove this ability from Koha.

Test Plan:
1) Apply this patch
2) Create a patron to generate the ACCTDETAILS email
3) Note you can no longer transmit the patron's password in the email

Signed-off-by: Amit Gupta <amitddng135@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2022-03-01 16:01:20 UTC
Created attachment 131195 [details] [review]
Bug 27812: Email should send for all new patron as per the syspref description

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2022-03-01 16:01:24 UTC
Created attachment 131196 [details] [review]
Bug 27812: Add DBRev to warn of the change to the account details notice

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2022-03-01 16:01:28 UTC
Created attachment 131197 [details] [review]
Bug 27812: (QA follow-up) Fix atomicupdate

We removed a bit too much from the skeliton here ;)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2022-03-01 16:06:31 UTC
Right, I think we've all agreed now to keep this bug to scope as a security related issue.

The code works as expected and we've agreed that updating the notices isn't reliably possible and so a warning should be used instead.

We now have such a warning posted during the database update and added in the 'Text to go in the release notes' field above.

As such, with the QA scripts passing and no nasty regressions found I think we can safely mark this PQA..

I will now go and QA the dependant bugs and report a new bug for the 'Your password has been changed' notice I suggested was a good idea here.

PQA
Comment 31 Martin Renvoize 2022-03-01 16:11:54 UTC
*** Bug 18543 has been marked as a duplicate of this bug. ***
Comment 32 Martin Renvoize 2022-03-01 16:38:44 UTC
Bug 25936 already records my request for password change notifications :)
Comment 33 Martin Renvoize 2022-03-02 16:40:35 UTC
Note.. I'm almost tempted to just ditch the ACCTDETAILS notice entirely and replace it with a new 'WELCOME' notice instead.. thus making the whole template and translations stuff mute.

Especially as the bugs that all hang off this now make the whole feature feel much more like a Welcome Email feature.
Comment 34 Andrew Fuerste-Henry 2022-03-02 17:10:30 UTC
+1 on the idea of creating a new Welcome Email feature to replace ACCTDETAILS. The current implementation is confusingly limited and inflexible.
Comment 35 Martin Renvoize 2022-03-09 12:18:50 UTC
See bug 30237 for the rename/rephrase of the feature notice and preference
Comment 36 Fridolin Somers 2022-03-17 21:23:42 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 37 Kyle M Hall 2022-03-25 13:31:43 UTC
Pushed to 21.11.x for 21.11.05
Comment 38 Katrin Fischer 2022-03-26 17:46:10 UTC
(In reply to Kyle M Hall from comment #37)
> Pushed to 21.11.x for 21.11.05

It's a big change for stable - bit worried libraires using the feature might be too surprised it.