Bug 23634 - Privilege escalation vulnerability for staff users with 'edit_borrowers' permission and 'OpacResetPassword' enabled
Summary: Privilege escalation vulnerability for staff users with 'edit_borrowers' perm...
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords: rel_20_11_target
Depends on:
Blocks: 26383
  Show dependency treegraph
 
Reported: 2019-09-18 09:27 UTC by Martin Renvoize
Modified: 2020-09-04 09:44 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.03, 19.11.09, 19.05.14


Attachments
Bug 23634: Prevent superlibrarian password resets via the OPAC (1.59 KB, patch)
2019-09-18 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23634: Prevent superlibrarian password resets via the OPAC (1.65 KB, patch)
2019-09-20 14:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: Add is_superlibrarian method to Koha::Patron (940 bytes, patch)
2019-11-19 14:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23634: Prevent non-superlibrarians from editing superlibarian emails (5.17 KB, patch)
2019-11-19 14:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23634: Add is_superlibrarian method to Koha::Patron (995 bytes, patch)
2019-11-19 16:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: Prevent non-superlibrarians from editing superlibarian emails (5.14 KB, patch)
2019-11-19 16:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: Secure the email on the API (2.78 KB, patch)
2019-11-19 16:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: Add tests for is_superlibrarian (1.42 KB, patch)
2019-11-19 16:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: (follow-up) Prevent updates on POST (946 bytes, patch)
2019-11-19 17:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23634: Add is_superlibrarian method to Koha::Patron (995 bytes, patch)
2020-01-27 15:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: Prevent non-superlibrarians from editing superlibarian emails (5.12 KB, patch)
2020-01-27 15:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: Secure the email on the API (2.81 KB, patch)
2020-01-27 15:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: Add tests for is_superlibrarian (1.39 KB, patch)
2020-01-27 15:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: (follow-up) Prevent updates on POST (1001 bytes, patch)
2020-01-27 15:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: Add is_superlibrarian method to Koha::Patron (995 bytes, patch)
2020-06-22 11:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23634: Add tests for is_superlibrarian (1.39 KB, patch)
2020-06-22 11:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23634: Prevent non-superlibrarians from editing superlibarian emails (6.05 KB, patch)
2020-06-22 11:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23634: Secure the email on the API (2.81 KB, patch)
2020-06-22 11:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23634: (follow-up) Prevent updates on POST (943 bytes, patch)
2020-06-22 11:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23634: Add is_superlibrarian method to Koha::Patron (995 bytes, patch)
2020-07-09 14:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23634: Add tests for is_superlibrarian (1.45 KB, patch)
2020-07-09 14:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23634: Prevent non-superlibrarians from editing superlibarian emails (5.94 KB, patch)
2020-07-09 14:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23634: Secure the email on the API (2.87 KB, patch)
2020-07-09 14:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23634: (follow-up) Prevent updates on POST (1008 bytes, patch)
2020-07-09 14:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23634: (QA follow-up) Catch all email cases in API (5.10 KB, patch)
2020-07-10 09:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23634: (QA follow-up) Adjust tests (4.92 KB, patch)
2020-07-13 10:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23634: Add is_superlibrarian method to Koha::Patron (997 bytes, patch)
2020-07-13 15:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: Add tests for is_superlibrarian (1.51 KB, patch)
2020-07-13 15:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: Prevent non-superlibrarians from editing superlibarian emails (5.95 KB, patch)
2020-07-13 15:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: Secure the email on the API (2.93 KB, patch)
2020-07-13 15:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: (follow-up) Prevent updates on POST (1.04 KB, patch)
2020-07-13 15:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: (QA follow-up) Catch all email cases in API (5.16 KB, patch)
2020-07-13 15:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: (QA follow-up) Adjust tests (4.98 KB, patch)
2020-07-13 15:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: (QA follow-up) Our PUT is really a PATCH (3.26 KB, patch)
2020-07-13 15:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23634: Add is_superlibrarian method to Koha::Patron (1.07 KB, patch)
2020-07-17 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23634: Add tests for is_superlibrarian (1.60 KB, patch)
2020-07-17 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23634: Prevent non-superlibrarians from editing superlibarian emails (6.06 KB, patch)
2020-07-17 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23634: Secure the email on the API (3.02 KB, patch)
2020-07-17 08:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23634: (follow-up) Prevent updates on POST (1.13 KB, patch)
2020-07-17 08:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23634: (QA follow-up) Catch all email cases in API (5.26 KB, patch)
2020-07-17 08:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23634: (QA follow-up) Adjust tests (5.07 KB, patch)
2020-07-17 08:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23634: (QA follow-up) Our PUT is really a PATCH (3.36 KB, patch)
2020-07-17 08:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23634: Make is_superlibrarian return 1 or 0 (1.91 KB, patch)
2020-09-03 08:20 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2019-09-18 09:27:09 UTC
A disgruntled employee can gain a privilege escalation if they have the 'edit_borrowers' permission combined with the 'OpacRestPassword' preference enabled.

To reproduce
1) Create a user with just 'staffaccess' and 'edit_borrowers' permissions.
2) Use that to update the email address of any superlibrarian user.
3) Navigate to the OPAC and request a password reset for the superlibrarian user using the new email address added in the previous step
4) Follow the password reset to it's end and login as the superlibrarian.

Resolution: We should prevent superlibrarian level users from restting their passwords via the OPAC.
Comment 1 Martin Renvoize 2019-09-18 09:49:24 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2019-09-20 14:13:38 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2019-09-24 23:42:59 UTC
This sounds wrong to me.
We should not restrict the actions of the superlibrarian but prevent non superlibrarian to modify the emails of superlibrarians instead. Do not you think?
Comment 4 Marcel de Rooy 2019-09-27 05:50:23 UTC
(In reply to Jonathan Druart from comment #3)
> This sounds wrong to me.
> We should not restrict the actions of the superlibrarian but prevent non
> superlibrarian to modify the emails of superlibrarians instead. Do not you
> think?

Thats my thought too.
Comment 5 Martin Renvoize 2019-11-05 10:19:57 UTC
So.. we're saying we want a 'ProtectSuperlibrarianPrivileges' like permission to prevent normal users from updating a superlibrarians account details generally.

'ProtectSuperlibrarianAccount' perhaps?
Comment 6 Marcel de Rooy 2019-11-05 15:43:44 UTC
(In reply to Martin Renvoize from comment #5)
> So.. we're saying we want a 'ProtectSuperlibrarianPrivileges' like
> permission to prevent normal users from updating a superlibrarians account
> details generally.
> 
> 'ProtectSuperlibrarianAccount' perhaps?

Or just always restrict it for regular users ?
Comment 7 Martin Renvoize 2019-11-19 14:56:12 UTC Comment hidden (obsolete)
Comment 8 Martin Renvoize 2019-11-19 14:56:15 UTC Comment hidden (obsolete)
Comment 9 Martin Renvoize 2019-11-19 14:57:42 UTC
OK, so this clearly needs a test adding for the new is_superlibrarian method introduced into Koha::Patron in the first patch.. but I'm interested to hear if people are generally happy with the approach before I add that.

Setting to NSO to get attention.
Comment 10 Jonathan Druart 2019-11-19 15:11:06 UTC
It needs to be enforced at higher level.
Comment 11 Marcel de Rooy 2019-11-19 15:13:08 UTC
Hi Martin,
You prevent it via the interface now. But what about posting it anyway? We should also prevent it at server side. When storing data.
Comment 12 Tomás Cohen Arazi 2019-11-19 15:41:31 UTC
(In reply to Marcel de Rooy from comment #11)
> Hi Martin,
> You prevent it via the interface now. But what about posting it anyway? We
> should also prevent it at server side. When storing data.

And the API.
Comment 13 Tomás Cohen Arazi 2019-11-19 16:22:50 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2019-11-19 16:22:54 UTC Comment hidden (obsolete)
Comment 15 Tomás Cohen Arazi 2019-11-19 16:22:58 UTC Comment hidden (obsolete)
Comment 16 Tomás Cohen Arazi 2019-11-19 16:23:02 UTC Comment hidden (obsolete)
Comment 17 Tomás Cohen Arazi 2019-11-19 16:25:51 UTC
(In reply to Marcel de Rooy from comment #11)
> Hi Martin,
> You prevent it via the interface now. But what about posting it anyway? We
> should also prevent it at server side. When storing data.

I agree we should return a 403 if the conditions are met to forbid the change. The current patches only deal with generating the template, and can be circumvented by sending the right data on the POST.
Comment 18 Jonathan Druart 2019-11-19 16:42:25 UTC
(In reply to Tomás Cohen Arazi from comment #16)
> Created attachment 95577 [details] [review] [review]
> Bug 23634: Add tests for is_superlibrarian
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

I would mimick tests for C4::Context::IsSuperLibrarian to include undef and 0.
Comment 19 Martin Renvoize 2019-11-19 17:00:18 UTC Comment hidden (obsolete)
Comment 20 Martin Renvoize 2019-11-19 17:02:53 UTC
(In reply to Jonathan Druart from comment #10)
> It needs to be enforced at higher level.

I contemplated adding is into the Koha::Patron->store method, but I didn't like the idea of having to pass through the current logged_in_user to this routine somehow and I also didn't want to expressely break peoples workflows outside of this member entry route.  It does raise the question of what should we do with bulk changes, imports, commandline imports etc etc etc.. I was deliberately not handling those cases at this point.
Comment 21 Tomás Cohen Arazi 2020-01-27 15:54:53 UTC Comment hidden (obsolete)
Comment 22 Tomás Cohen Arazi 2020-01-27 15:54:57 UTC Comment hidden (obsolete)
Comment 23 Tomás Cohen Arazi 2020-01-27 15:55:01 UTC Comment hidden (obsolete)
Comment 24 Tomás Cohen Arazi 2020-01-27 15:55:05 UTC Comment hidden (obsolete)
Comment 25 Tomás Cohen Arazi 2020-01-27 15:55:09 UTC Comment hidden (obsolete)
Comment 26 Bernardo Gonzalez Kriegel 2020-05-06 01:09:54 UTC
Sorry, does not apply, please rebase and I'll test.
Comment 27 Martin Renvoize 2020-06-22 11:42:31 UTC
Created attachment 106159 [details] [review]
Bug 23634: Add is_superlibrarian method to Koha::Patron

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Martin Renvoize 2020-06-22 11:42:35 UTC
Created attachment 106160 [details] [review]
Bug 23634: Add tests for is_superlibrarian

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Martin Renvoize 2020-06-22 11:42:38 UTC
Created attachment 106161 [details] [review]
Bug 23634: Prevent non-superlibrarians from editing superlibarian emails

This patchset prevents a non-superlibrarian user from editing a
superlibrarians email address via memberentry.  This is to prevent a
privilege escalation vulnerability whereby a user could update a
superlibrarians contact details to match their own and then request a
password reset via the OPAC.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Martin Renvoize 2020-06-22 11:42:43 UTC
Created attachment 106162 [details] [review]
Bug 23634: Secure the email on the API

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 31 Martin Renvoize 2020-06-22 11:42:47 UTC
Created attachment 106163 [details] [review]
Bug 23634: (follow-up) Prevent updates on POST
Comment 32 Martin Renvoize 2020-06-22 11:43:46 UTC
Rebased and ready for testing.
Comment 33 Martin Renvoize 2020-07-09 14:58:01 UTC
Created attachment 106723 [details] [review]
Bug 23634: Add is_superlibrarian method to Koha::Patron

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 34 Martin Renvoize 2020-07-09 14:58:04 UTC
Created attachment 106724 [details] [review]
Bug 23634: Add tests for is_superlibrarian

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Martin Renvoize 2020-07-09 14:58:08 UTC
Created attachment 106725 [details] [review]
Bug 23634: Prevent non-superlibrarians from editing superlibarian emails

This patchset prevents a non-superlibrarian user from editing a
superlibrarians email address via memberentry.  This is to prevent a
privilege escalation vulnerability whereby a user could update a
superlibrarians contact details to match their own and then request a
password reset via the OPAC.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 36 Martin Renvoize 2020-07-09 14:58:11 UTC
Created attachment 106726 [details] [review]
Bug 23634: Secure the email on the API

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Martin Renvoize 2020-07-09 14:58:15 UTC
Created attachment 106727 [details] [review]
Bug 23634: (follow-up) Prevent updates on POST

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize 2020-07-09 15:01:17 UTC
Rebased again.. and restored Tomas's SO (whilst also SOing the API piece he added)
Comment 39 Nick Clemens 2020-07-09 15:28:54 UTC
(In reply to Martin Renvoize from comment #36)
> Created attachment 106726 [details] [review] [review]
> Bug 23634: Secure the email on the API
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Should the email have to be sent for a change request?
Should it be "defined $body->{email} && $body->{email} ne $patron->email"?
Comment 40 Martin Renvoize 2020-07-10 09:27:07 UTC
Created attachment 106764 [details] [review]
Bug 23634: (QA follow-up) Catch all email cases in API

The API was only catching the primary email change case, but we need to
catch email, emailpro and B_email.

We were also not accounting for any of the emails (on PUT or from the
DB) being undefined.
Comment 41 Martin Renvoize 2020-07-10 09:29:34 UTC
(In reply to Nick Clemens from comment #39)
> (In reply to Martin Renvoize from comment #36)
> > Created attachment 106726 [details] [review] [review] [review]
> > Bug 23634: Secure the email on the API
> > 
> > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
> > Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
> 
> Should the email have to be sent for a change request?
> Should it be "defined $body->{email} && $body->{email} ne $patron->email"?

Good catch.. though it's not quite as simple as that :(

We need to check for a difference inclusive of whether either value is undefined.. and it also highlighted to me that we were missing some forms of email on the API patch.

Followup attached, however I'm struggling to get the API test to pass (before and after my patch... it seems grabbing the user from the stash somehow breaks the is_librarian method...)
Comment 42 Katrin Fischer 2020-07-12 09:53:14 UTC
Ok, keeping this issue to: Taking over another person's account (not talking about: giving yourself more permissions)

I think the proposed solution is ok and I agree that we should stop misuse and not limit the abilities of superlibrarians.

Some things still appear problematic to me:

1) What if the notice is sent out by SMS? Not only the emails need to be protected, but the smsalertnumber a well. Maybe better to not allow editing a superlibrarian's account at all?
2) What about using the reports permission to get the reset password link straight from the message_queue? I have done that on systems where sending emails was turned off to demonstrate the feature in training. Or maybe it even shows in their notices tab? Then edit_borrowers would already be enough.
3) What about turning off ProtectSuperlibrarianPrivileges and give yourself superlibrarian if you have parameters permission?
4) Having staffaccess... can't they just reset the password in the GUI? (unless you mean catalogue permission and not staffaccess)?
5) What if you don't try this with a superlibrarian, but with someone else having other permissions you plan to misuse?

Tests don't pass for me - as noted in comment#41 there still appears to be an issue with them?

--------

I feel we a general concept for protecting access/staff accounts if that is really what we are aiming for. We are only talking about "breaking into" a superlibrarian accounts here. What about if you try to break into any other staff account that has the permissions flag? If you can get yourself access to all but superlibrarian, the difference is not very huge and a lot of damage could be potentially be caused. Just getting parameters or one of the batch_edit or export permissions is a potential risk and you need to be able to trust staff to not misuse those.

How do other systems handle these kind of things?
Most attempts of abuse will leave traces in logs and tables. If you can prove misuse... is that enough?


-------

Some different ideas:

staffaccess permission: Allow staff members to modify permissions, usernames, and passwords for other staff members.
Which does prevent changing another staff users login data to some extent. We could use that here and widen the scope to not be able to change other fields as well. Or sub permissions to not allow editing the records of other staff users at all?

ProtectSuperlibrarianPrivileges system preferences that prevents staff from changing the superlibrarian permission. We could possibly enhance that to include modifying superlibrarian accounts in other ways. Maybe restrict from modifying the accounts at all?
Comment 43 Martin Renvoize 2020-07-13 08:59:16 UTC
(In reply to Katrin Fischer from comment #42)
> Ok, keeping this issue to: Taking over another person's account (not talking
> about: giving yourself more permissions)
> 
> I think the proposed solution is ok and I agree that we should stop misuse
> and not limit the abilities of superlibrarians.
> 
> Some things still appear problematic to me:
> 
> 1) What if the notice is sent out by SMS? Not only the emails need to be
> protected, but the smsalertnumber a well. Maybe better to not allow editing
> a superlibrarian's account at all?

This is only designed to prevent password resets via the OPAC.. you cannot request such a reset using an SMSAlertnumber.

> 2) What about using the reports permission to get the reset password link
> straight from the message_queue? I have done that on systems where sending
> emails was turned off to demonstrate the feature in training. Or maybe it
> even shows in their notices tab? Then edit_borrowers would already be enough.

We're preventing the change of email such that a user cannot update to an email for which they have access and then request the password change.  We are not attempting to prevent attacks via reports.. the original patches would have caught such cases as we forbid superlibrarians from submitting password reset requests in the first place, but those patches were pushed back against as it reduced functionality for superlibrarians.

> 3) What about turning off ProtectSuperlibrarianPrivileges and give yourself
> superlibrarian if you have parameters permission?

?

> 4) Having staffaccess... can't they just reset the password in the GUI?
> (unless you mean catalogue permission and not staffaccess)?

That would be an entirely different bug.. but I believe that is not the case.. I'd need to test.

> 5) What if you don't try this with a superlibrarian, but with someone else
> having other permissions you plan to misuse?

We do not have a fine-grained role-based privilege system.. as such, we can't do any meaningful form of roles based access restrictions.. only the superlibrarian counts as a distinct role at the moment.
 
> 
> Tests don't pass for me - as noted in comment#41 there still appears to be
> an issue with them?

Awaiting feedback from Tomas.. I can't get the API tests to pass prior to my patches and they fail in the same way after too.

> 
> --------
> 
> I feel we a general concept for protecting access/staff accounts if that is
> really what we are aiming for. We are only talking about "breaking into" a
> superlibrarian accounts here. What about if you try to break into any other
> staff account that has the permissions flag? If you can get yourself access
> to all but superlibrarian, the difference is not very huge and a lot of
> damage could be potentially be caused. Just getting parameters or one of the
> batch_edit or export permissions is a potential risk and you need to be able
> to trust staff to not misuse those.

Superlibrarians are a special group. This has come up in multiple penetration tests as a hard fail.

> 
> How do other systems handle these kind of things?
> Most attempts of abuse will leave traces in logs and tables. If you can
> prove misuse... is that enough?
> 
> 
> -------
> 
> Some different ideas:
> 
> staffaccess permission: Allow staff members to modify permissions,
> usernames, and passwords for other staff members.
> Which does prevent changing another staff users login data to some extent.
> We could use that here and widen the scope to not be able to change other
> fields as well. Or sub permissions to not allow editing the records of other
> staff users at all?
> 
> ProtectSuperlibrarianPrivileges system preferences that prevents staff from
> changing the superlibrarian permission. We could possibly enhance that to
> include modifying superlibrarian accounts in other ways. Maybe restrict from
> modifying the accounts at all?

Frankly, I've been chasing this bug round in circles for nearly 2 years and I can't afford to delve weeks more time into developing a new more advanced solution with yet more new risks of getting stuck at QA.  We are legally obliged to have submitted this upstream and I feel taking so long for a security bug to work it's way through is massively detrimental to the project and highlights Koha in a very poor light.
Comment 44 Nick Clemens 2020-07-13 10:33:31 UTC
Created attachment 106826 [details] [review]
Bug 23634: (QA follow-up) Adjust tests
Comment 45 Nick Clemens 2020-07-13 10:36:27 UTC
I think Katrin's points are valid, but maybe we should open an omnibus bug as they are bigger issues than this patchset is attempting to solve

I added a follow-up to fix existing tests, just needed to remove (or set) the read only attributes.

I added an additional test which fails, however. This is the case I mentioned but perhaps didn't fully explain. The email fields are not required during a patron update but an update without these fields passed are now forbidden.
Comment 46 Tomás Cohen Arazi 2020-07-13 15:35:41 UTC
Created attachment 106837 [details] [review]
Bug 23634: Add is_superlibrarian method to Koha::Patron

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 47 Tomás Cohen Arazi 2020-07-13 15:35:46 UTC
Created attachment 106838 [details] [review]
Bug 23634: Add tests for is_superlibrarian

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 48 Tomás Cohen Arazi 2020-07-13 15:35:52 UTC
Created attachment 106839 [details] [review]
Bug 23634: Prevent non-superlibrarians from editing superlibarian emails

This patchset prevents a non-superlibrarian user from editing a
superlibrarians email address via memberentry.  This is to prevent a
privilege escalation vulnerability whereby a user could update a
superlibrarians contact details to match their own and then request a
password reset via the OPAC.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 49 Tomás Cohen Arazi 2020-07-13 15:35:59 UTC
Created attachment 106840 [details] [review]
Bug 23634: Secure the email on the API

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 50 Tomás Cohen Arazi 2020-07-13 15:36:04 UTC
Created attachment 106841 [details] [review]
Bug 23634: (follow-up) Prevent updates on POST

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 51 Tomás Cohen Arazi 2020-07-13 15:36:10 UTC
Created attachment 106842 [details] [review]
Bug 23634: (QA follow-up) Catch all email cases in API

The API was only catching the primary email change case, but we need to
catch email, emailpro and B_email.

We were also not accounting for any of the emails (on PUT or from the
DB) being undefined.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 52 Tomás Cohen Arazi 2020-07-13 15:36:19 UTC
Created attachment 106843 [details] [review]
Bug 23634: (QA follow-up) Adjust tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 53 Tomás Cohen Arazi 2020-07-13 15:36:26 UTC
Created attachment 106844 [details] [review]
Bug 23634: (QA follow-up) Our PUT is really a PATCH

This patch makes the controller not expect that there will always be all
the email fields. So it now checks if an email field was passed, and
changed, and renders the error if that stands.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: Tests written by Nick highlight a problem
2. Apply this patch
3. Repeat 1
=> SUCCESS: Problems solved
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 54 Katrin Fischer 2020-07-13 17:53:02 UTC
I didn't realize how old this was (although 1 year, not 2?).

Tomas, should this be signed off?
Comment 55 Martin Renvoize 2020-07-15 07:44:38 UTC
Thanks for the followups here Tomas, great work!
Comment 56 Marcel de Rooy 2020-07-17 06:12:22 UTC
Looking here now
Comment 57 Marcel de Rooy 2020-07-17 06:41:12 UTC
t/db_dependent/api/v1/patrons.t passes with prove (so probably OK) but fails with perl (enabled strict sql mode)
Anything to do perhaps with Test::Mojo, Tomas ?
Few fails like:

        #   Failed test at t/db_dependent/api/v1/patrons.t line 163.
        # found warning: DBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry 'NBR5Mt5ofyM6_KfwNIkEde_c' for key 'cardnumber' at /usr/share/koha/Koha/Object.pm line 166
        # expected to find warning: (?^u:DBD::mysql::st execute failed: Duplicate entry)
Comment 58 Marcel de Rooy 2020-07-17 06:53:27 UTC
--- a/Koha/REST/V1/Patrons.pm
+++ b/Koha/REST/V1/Patrons.pm
@@ -205,6 +205,38 @@ sub update {

My first thought, seeing that code, is: Should this check be in the API ? Or should it be in the Koha modules?
But I can live with that thought ;)
Comment 59 Marcel de Rooy 2020-07-17 07:03:21 UTC
> Frankly, I've been chasing this bug round in circles for nearly 2 years and
> I can't afford to delve weeks more time into developing a new more advanced
> solution with yet more new risks of getting stuck at QA.  We are legally
> obliged to have submitted this upstream and I feel taking so long for a
> security bug to work it's way through is massively detrimental to the
> project and highlights Koha in a very poor light.

You waited until September 18, 2019 to submit this report, Martin.
So we are only 10 months further. (Tomorrow)
No need to press QA with "2 years" imho ;) Please submit earlier?

The concerns Katrin raised are valid, but I agree that we do not need to widen the scope of this report now. We could address such things on new reports.

The feelings you describe about getting stuck in QA (or even before that) are very recognizable. If you have suggestions to improve, please follow the usual routes to do so :) The 'very poor light' is no QA crowbar..
Comment 60 Martin Renvoize 2020-07-17 07:21:18 UTC
I misread the date in the first comment.

But still.. I am worried that security bugs seem to not get a great deal of attention. :(
Comment 61 Marcel de Rooy 2020-07-17 07:42:02 UTC
(In reply to Martin Renvoize from comment #60)
> I misread the date in the first comment.
> 
> But still.. I am worried that security bugs seem to not get a great deal of
> attention. :(

Agreed
Comment 62 Marcel de Rooy 2020-07-17 08:43:48 UTC
(In reply to Tomás Cohen Arazi from comment #53)
> Created attachment 106844 [details] [review] [review]
> Bug 23634: (QA follow-up) Our PUT is really a PATCH

I tried PATCH by the way but we still do not support it. Any plans?
Comment 63 Marcel de Rooy 2020-07-17 08:44:47 UTC
Created attachment 106979 [details] [review]
Bug 23634: Add is_superlibrarian method to Koha::Patron

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 64 Marcel de Rooy 2020-07-17 08:44:51 UTC
Created attachment 106980 [details] [review]
Bug 23634: Add tests for is_superlibrarian

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 65 Marcel de Rooy 2020-07-17 08:44:56 UTC
Created attachment 106981 [details] [review]
Bug 23634: Prevent non-superlibrarians from editing superlibarian emails

This patchset prevents a non-superlibrarian user from editing a
superlibrarians email address via memberentry.  This is to prevent a
privilege escalation vulnerability whereby a user could update a
superlibrarians contact details to match their own and then request a
password reset via the OPAC.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 66 Marcel de Rooy 2020-07-17 08:45:01 UTC
Created attachment 106982 [details] [review]
Bug 23634: Secure the email on the API

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 67 Marcel de Rooy 2020-07-17 08:45:06 UTC
Created attachment 106983 [details] [review]
Bug 23634: (follow-up) Prevent updates on POST

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 68 Marcel de Rooy 2020-07-17 08:45:10 UTC
Created attachment 106984 [details] [review]
Bug 23634: (QA follow-up) Catch all email cases in API

The API was only catching the primary email change case, but we need to
catch email, emailpro and B_email.

We were also not accounting for any of the emails (on PUT or from the
DB) being undefined.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 69 Marcel de Rooy 2020-07-17 08:45:15 UTC
Created attachment 106985 [details] [review]
Bug 23634: (QA follow-up) Adjust tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 70 Marcel de Rooy 2020-07-17 08:45:21 UTC
Created attachment 106986 [details] [review]
Bug 23634: (QA follow-up) Our PUT is really a PATCH

This patch makes the controller not expect that there will always be all
the email fields. So it now checks if an email field was passed, and
changed, and renders the error if that stands.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: Tests written by Nick highlight a problem
2. Apply this patch
3. Repeat 1
=> SUCCESS: Problems solved
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 71 Victor Grousset/tuxayo 2020-08-25 07:48:27 UTC
Couldn't backport the API tests from
« Bug 23634: Secure the email on the API»
and
«Bug 23634: (QA follow-up) Catch all email cases in API»
and
«Bug 23634: (QA follow-up) Adjust tests»

Even after messing a lot with the tests to introduce what's needed to use stuff like:
$t->put_ok("//$userid:$password@/api/v1/patrons/"

I get a bunch of
        #          $got->{error} = 'Basic authentication disabled'
        #     $expected->{error} = 'Not enough privileges to change a superlibrarian's altaddress_email to undefined'


Which makes me think that the 19.11.x tests can't work like that in 19.05.x and that the request method must be changed in each test.

Moving on without the the tests. The other tests still passe and the changes have been tested upstream.
Backport of the tests still welcome.
Comment 72 Victor Grousset/tuxayo 2020-08-25 08:47:00 UTC
Backported to 19.05.x branch for 19.05.14

It was hard due to conflicts but doable by cherry-picking the 19.11.x version. The earlier attempt with master version lead to several hundred lines of conflicts.
Comment 73 Aleisha Amohia 2020-08-26 04:37:14 UTC
backported to 19.11.x for 19.11.09
Comment 74 Lucas Gass 2020-08-31 21:16:39 UTC
backported to 20.05.x for 20.05.03
Comment 75 Jonathan Druart 2020-09-03 08:20:49 UTC
Created attachment 109582 [details] [review]
Bug 23634: Make is_superlibrarian return 1 or 0
Comment 76 Martin Renvoize 2020-09-03 08:25:49 UTC
I agree with this followup.. it makes a lot of sense to enforce a boolean return as aposed to leaking the hash as has_permission does.
Comment 77 Jonathan Druart 2020-09-03 08:40:23 UTC
Nothing to worry for stable branches, it will not change anything.
Comment 78 Jonathan Druart 2020-09-03 08:41:52 UTC
Pushed to master for 20.11, thanks to everybody involved!