There is no server side filtering of the borrower fields in the OPAC self-registration. This makes it trivial to self register as a super librarian by simply adding an input element to the registration form using the DOM inspector: <input type="hidden" name="borrower_flags" value="1" /> Any field can be modified using this. (It is not possible to edit existing by adding a borrower_borrowernumber field, although this seems to be coincidental rather than by design.) Fields that should not be editable in the OPAC must be filtered from the borrower data on the server side before adding the borrower to the database.
whooot!?
Confirmed
(In reply to Andreas Jonsson from comment #0) > There is no server side filtering of the borrower fields in the OPAC > self-registration. This makes it trivial to self register as a super > librarian by simply adding an input element to the registration form using > the DOM inspector: Great catch !
Created attachment 124308 [details] [review] Bug 28929: Prevent flags to be sent during selfreg
This patch fixes the problem but does not feel very robust. We will need a selenium test.
Self modification seems safe.
While the field flags is the most serious problem, there are additional fields that should not be editable. For instance these: dateenrolled, dateexpiry, date_renewed, lost, debarred, debarredcomment, borrowernote, contactnote, opacnote, sort1, sort2, login_attempts, lastseen. Maybe others as well. borrowernumber should be explicitly filtered. Also, fields in PatronSelfRegistrationBorrowerUnwantedField should be filtered I think. Also, patron attributes that should not be editable in OPAC.
Created attachment 124310 [details] [review] Bug 28929: Add selenium tests
I am done for today.
(In reply to Andreas Jonsson from comment #7) > While the field flags is the most serious problem, there are additional > fields that should not be editable. For instance these: dateenrolled, > dateexpiry, date_renewed, lost, debarred, debarredcomment, borrowernote, > contactnote, opacnote, sort1, sort2, login_attempts, lastseen. Maybe others > as well. > borrowernumber should be explicitly filtered. > > Also, fields in PatronSelfRegistrationBorrowerUnwantedField should be > filtered I think. > > Also, patron attributes that should not be editable in OPAC. Lots of good points, although we often let people update their sort1 and sort2 as those fields get used for all kinds of different purposes.
I think that we'd need an allow list to specify what columns are allowed. While we could use a deny list, it's harder to remember to add items to it. Plus, garbage data is much more difficult to filter out then. I suppose we could "use ./koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/borrowers.json" to provide an allow list, although I'm not sure it's available in all supported Koha versions... I think Andreas has a point about filtering out unwanted fields too which would need to cover these 3 system preferences: - BorrowerUnwantedField - PatronSelfModificationBorrowerUnwantedField - PatronSelfRegistrationBorrowerUnwantedField Looking at ./members/memberentry.pl and it looks like you could do the same thing. It implements a bit of a denylist, but not comprehensive enough it seems. For a different project, I've actually had to do per-column authorization by role, and it's not necessarily that easy to do comprehensively when different roles have different authorizations. In this case, we wouldn't necessarily have to go that far.
An allow list looks like it's problematic too though, as not everything that passes through ParseCgiForBorrower is actually a column. For example: password2, gdpr_proc_consent. That said, we could just add those to our list of allowed fields I suppose. An additive rather than subtractive process will be more secure in my opinion, although it could lead to more bugs in the short-term if something is missed from the allow list. It looks like patron attributes are handled separately by ParsePatronAttributes().
Another option would be to use the CheckforInvalidFields function.
In 20.11, I was able to update my own flags via the Staff Interface as well.
After reviewing https://owasp.org/www-project-proactive-controls/v3/en/c5-validate-inputs, I think we really should be using an allow list. I'll have a crack at a first draft.
After looking at this more, there is actually a massive difference between the OPAC and Staff Interface input forms, and fixing the Staff Interface is going to be much more work than the OPAC.
I agree with David that an allow list is a better solution.
Created attachment 124315 [details] [review] [Alternate] Bug 28929: Filter user input through allowlist for patrons This patch creates public and staff allowlists that are used to filter the user input from the Staff Interface, OPAC, and OPAC self-registration. The lists are hard-coded into modules, but they could be generated by other means in the future to make them more suited to user context. Test plan: 0. The following should be done for the Staff Interface, a logged in OPAC user, and a OPAC self-registration user 1. Go to a patron create/edit screen 2. Using F12 dev tools in your browser, add an input for "flags" or "borrower_flags" depending on the other inputs available on that screen. Give it a value of "1". 3. Submit the change 4. Check the Staff Interface or database to confirm that the flags have not been set to 1 for that user Additional test plan: 1. prove t/Koha/Patron/Allowlist.t 2. Note that all tests pass
I've marked as Needs Signoff, but it should probably be "In Discussion". I don't love my patch, and it needs more testing (especially for GDPR and the unit tests could be expanded), but it seems to be correct and effective so far. I would actually like to have more context-specific allowlists based on the user authorizations, but hopefully I've left enough room in the approach for that to be possible. Most of Koha's authorizations are done at the scrpit-level, so it would be nice to actually have a system-wide granular authorization system, but I wasn't going to try to create that today... I figure this is simple enough yet comprehensive enough for these member entry pages, but curious to hear what you all think.
Created attachment 124316 [details] [review] [Alternate] Bug 28929: Filter user input through allowlist for patrons This patch creates public and staff allowlists that are used to filter the user input from the Staff Interface, OPAC, and OPAC self-registration. The lists are hard-coded into modules, but they could be generated by other means in the future to make them more suited to user context. Test plan: 0. The following should be done for the Staff Interface, a logged in OPAC user, and a OPAC self-registration user 1. Go to a patron create/edit screen 2. Using F12 dev tools in your browser, add an input for "flags" or "borrower_flags" depending on the other inputs available on that screen. Give it a value of "1". 3. Submit the change 4. Check the Staff Interface or database to confirm that the flags have not been set to 1 for that user Additional test plan: 1. prove t/Koha/Patron/Allowlist.t 2. Note that all tests pass
I think that the apply function should throw an exception if the input parameters are incorrect. As it stands, it will silently allow everything if this is the case. if ($ui_fields && ref $ui_fields eq 'HASH'){ ... if ( $input && ref $input eq 'HASH' ){ ...
Created attachment 124317 [details] [review] Bug 28929: Centralize the allow list We want to make sure developpers won't forget to update the allow list and that we get discrepancy between the list. This patch suggests to have: 1. A global "all fields" list which must match the borrowers table columns 2. A global deny list to apply on top of this list. This deny list is the list of attributes we want to reject from patron's edition from both staff and OPAC interfaces 3. A specific deny list per interface Moreover there is now a warning when a user edit an attribute part of the deny list.
(In reply to Andreas Jonsson from comment #23) > I think that the apply function should throw an exception if the input > parameters are incorrect. As it stands, it will silently allow everything > if this is the case. > > if ($ui_fields && ref $ui_fields eq 'HASH'){ > ... > > if ( $input && ref $input eq 'HASH' ){ > ... It depends on what we want this function to do. Either it's a filter, or a confirmation we did things correctly prior to its call. I have added a warn there and it highlights the problem: we now have a lot of warnings in the logs. [2021/09/01 08:48:09] [WARN] Forbidden - Tried to modify 'dateexpiry' with '2029-12-01' from CGI::Compile::ROOT::kohadevbox_koha_opac_opac_2dmemberentry_2epl /kohadevbox/koha/opac/opac-memberentry.pl 277 at /kohadevbox/koha/Koha/Patron/Allowlist.pm line 56.
(In reply to Jonathan Druart from comment #6) > Self modification seems safe. This was wrong, self modification is not safe either.
I am a little worried at seeing the complexity of the proposed fix. I wonder if in light of backporting to older affected versions blocking flags as a first important step is the way to go and having/discussing a more complete solution as a second step. Especially as we won't be able to provide fixed for all possibly affected old versions, a simple "here do this" might be nice. If people can edit fields they should not, that's not great, but I'd not qualify it as a security issue.
This goes back far, just verified the problem with OPAC self registration and OPAC self modification in 18.11.16. For self modification it shows "flags" as changed field on moderating the change request, but this still could be missed easily.
(In reply to Katrin Fischer from comment #28) > Especially as we won't be able to provide fixed for all possibly affected > old versions, a simple "here do this" might be nice. This change should do it and should be applicable to old versions. diff --git a/members/memberentry.pl b/members/memberentry.pl index 5844b9c3f90..c9460583af7 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -215,6 +215,7 @@ if ( $op eq 'insert' || $op eq 'modify' || $op eq 'save' || $op eq 'duplicate' ) # remove keys from %newdata that is not part of patron's attributes { my @keys_to_delete = ( + qr/^flags$/, qr/^BorrowerMandatoryField$/, qr/^category_type$/, qr/^check_member$/, diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl index 8945cc339ba..c940a6e1af6 100755 --- a/opac/opac-memberentry.pl +++ b/opac/opac-memberentry.pl @@ -522,6 +522,7 @@ sub ParseCgiForBorrower { # Replace checkbox 'agreed' by datetime in gdpr_proc_consent $borrower{gdpr_proc_consent} = dt_from_string if $borrower{gdpr_proc_consent} && $borrower{gdpr_proc_consent} eq 'agreed'; + delete $borrower{flags}; return %borrower; }
Can someone verify if turning off PatronSelfRegistration and PatronDetails is enough to prevent this bug as an immediate action? PatronDetails only seems to deactivate the form fields on first glance. I tried also adding the submit button back in DOM and can submit, but get no change request - so I didn't manage to exploit it yet. But would love if someone could verify.
Below a copy of the email I sent to some people. Hello everybody, If you get this email I am expecting from you to talk about it to whom could be concerned. Yesterday a bug (28929) was reported about a possible privilege escalation. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28929 It is confirmed and is highlighly critical in some cases, and stay critical in all other cases. It's really bad. Basically we are missing filtering on patron's data when we create or edit a patron. Which means a user can edit whichever info they want, by modifying the DOM of the page. And, it includes borrowers.flags (that's where it hurts really badly). It impacts both OPAC and Staff interfaces. What you should do: 1. Reduce the risks! * Disable PatronSelfRegistration (!) * Disable AutoApprovePatronProfileSettings (!) or you can totally disable OPACPatronDetails 2. Make sure the staff interface is not accessible to the world 3. Remove old librarian accounts 4. Something else you have in mind? If so, please share! Then you should (or ask your team to) help us. We will need people on the bug to: 1. Write patches and discuss the best approach to fix the different issues (there are patches already) 2. Test! and test! We will have to test on five different versions (master+4 stables)! 3. QA, and *really* review, all the different versions (we really want to prevent a mess publishing a fix that will introduce regressions). We will then need to coordinate. A plan (to discuss) could be: 1. Tell the big actors to be ready for an urgent release (This is the goal of this email!) 2. Make the patches ready for the different versions (see the above) 3. Have all the packages ready, before we communicate publicly (Mason, will you be available?) 4. Tell the people we know to upgrade (so, you, and those who are aware of the problem) 5. Announce publicly What do you think? This info should not be public, but please communicate as much as you can around you (people you trust of course). No need to over communicate either, as we are not ready yet! If you don't have access to the bug, just tell me (or Tomas, Katrin, Martin, Nick, etc.) Cheers, Jonathan
(In reply to Katrin Fischer from comment #31) > Can someone verify if turning off PatronSelfRegistration and PatronDetails > is enough to prevent this bug as an immediate action? > > PatronDetails only seems to deactivate the form fields on first glance. I > tried also adding the submit button back in DOM and can submit, but get no > change request - so I didn't manage to exploit it yet. But would love if > someone could verify. I managed to create a modification request, even with PatronDetails and PatronSelfRegistration turned off (modified initials to flags, set 1, added the submit button, and copied csrf from another form).
(In reply to Jonathan Druart from comment #30) > diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl > index 8945cc339ba..c940a6e1af6 100755 > --- a/opac/opac-memberentry.pl > +++ b/opac/opac-memberentry.pl > @@ -522,6 +522,7 @@ sub ParseCgiForBorrower { > # Replace checkbox 'agreed' by datetime in gdpr_proc_consent > $borrower{gdpr_proc_consent} = dt_from_string if > $borrower{gdpr_proc_consent} && $borrower{gdpr_proc_consent} eq 'agreed'; > > + delete $borrower{flags}; > return %borrower; > } Why not use sub CheckForInvalidFields ?
Created attachment 124323 [details] [review] Bug 28929: [SIGNED_OFF] Prevent flags to be sent during selfreg Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested OPAC and staff side. Prevents mangling flags column.
Focusing on the privileges escalation here.
(In reply to Marcel de Rooy from comment #34) > (In reply to Jonathan Druart from comment #30) > > > diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl > > index 8945cc339ba..c940a6e1af6 100755 > > --- a/opac/opac-memberentry.pl > > +++ b/opac/opac-memberentry.pl > > @@ -522,6 +522,7 @@ sub ParseCgiForBorrower { > > # Replace checkbox 'agreed' by datetime in gdpr_proc_consent > > $borrower{gdpr_proc_consent} = dt_from_string if > > $borrower{gdpr_proc_consent} && $borrower{gdpr_proc_consent} eq 'agreed'; > > > > + delete $borrower{flags}; > > return %borrower; > > } > > Why not use sub CheckForInvalidFields ? The idea was to make sure the fix would be backportable/applicable easily on older versions. Feel free to suggest an alternative patch if you found something better :)
I would be in favor of pushing the simple patch now first (easy to backport). It seems that flags is the most critical field. I have been testing a bit with trying to modify borrowernumber but that seems to work fine. So we cannot change another borrower by adding an input borrowernumber. Surely we do not want to hack cardnumber, dateexpiry, debarred and so, but that could follow soon.
Created attachment 124325 [details] [review] Bug 28929: Prevent flags to be sent during patron's edition * selfreg and selfmod for OPAC * patron's edition on staff Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested OPAC and staff side. Prevents mangling flags column.
The two other patches have been moved to bug 28935.
(In reply to Andreas Jonsson from comment #7) > While the field flags is the most serious problem, there are additional > fields that should not be editable. For instance these: dateenrolled, > dateexpiry, date_renewed, lost, debarred, debarredcomment, borrowernote, > contactnote, opacnote, sort1, sort2, login_attempts, lastseen. Maybe others > as well. > > borrowernumber should be explicitly filtered. Done on bug 28935 (with the patches moved there). > Also, fields in PatronSelfRegistrationBorrowerUnwantedField should be > filtered I think. Additional patch added on bug 28935 > Also, patron attributes that should not be editable in OPAC. Didn't recreate that, but still enforced on the last patch on bug 28935.
Created attachment 124364 [details] [review] Bug 28929: Add selenium tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 124365 [details] [review] Bug 28929: Prevent flags to be sent during patron's edition * selfreg and selfmod for OPAC * patron's edition on staff Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested OPAC and staff side. Prevents mangling flags column. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 124366 [details] [review] Bug 28929: (follow-up) Add exec flag to tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 124405 [details] [review] Bug 28929: Add selenium tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 124406 [details] [review] Bug 28929: Prevent flags to be sent during patron's edition * selfreg and selfmod for OPAC * patron's edition on staff Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested OPAC and staff side. Prevents mangling flags column. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 124407 [details] [review] Bug 28929: (follow-up) Add exec flag to tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to Katrin Fischer from comment #28) > I am a little worried at seeing the complexity of the proposed fix. I wonder > if in light of backporting to older affected versions blocking flags as a > first important step is the way to go and having/discussing a more complete > solution as a second step. I was thinking the same thing yesterday. Initially, we need a simple fix that is easily tested and distributed. It looks like OPAC users can only modify their own user, so the only field that should really be immediately dangerous is "flags".
(In reply to Kyle M Hall from comment #46) > Created attachment 124406 [details] [review] [review] > Bug 28929: Prevent flags to be sent during patron's edition > > * selfreg and selfmod for OPAC > * patron's edition on staff > > Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> > Tested OPAC and staff side. Prevents mangling flags column. > > Signed-off-by: Nick Clemens <nick@bywatersolutions.com> > > Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Three members of the QA team already signed off on this one! Kyle, is your signoff a QA stamp ?
(In reply to Marcel de Rooy from comment #49) > Three members of the QA team already signed off on this one! > Kyle, is your signoff a QA stamp ? I was wondering this too. The QA tools returned OK. I tried running the full unit test suite but got a lot of failures (my koha-testing-docker is probably outdated). They'll be unrelated to this change though. I'd be happy to add my QA stamp if needed.
We've implementd and tested this fix for 18.11 yesterday - so I probably should not be signing off. But can confirm that it works :) I'll take the chance to set to PQA.
Please continue the work on bug 28935. I think it must be included with this one in (at least) the last two stables.
(Are we missing any other entry points? Is it possible to modify users using a kiosk via SIP? It doesn't look like it at a glance.)
Is the screenshot taking still useful in the test? ↓↓↓ $driver->capture_screenshot('selenium_failure_x.png');
(In reply to Victor Grousset/tuxayo from comment #54) > Is the screenshot taking still useful in the test? > ↓↓↓ > $driver->capture_screenshot('selenium_failure_x.png'); Probably not ;)
Ok, I can add a patch to remove that unless there is an objection. Also, I missed this on the first review: $driver->get($opac_base_url . 'opac-main.pl'); $driver->get($opac_base_url . 'opac-memberentry.pl'); Is going to the homepage useful before going directly via URL to the self registration form? Otherwise, code look good and seems stable after a few hundred runs trial.
Tested on 20.05.x: - reproduced self registration attack and the patch prevents it - reproduced self modification attack and the patch prevents it - self registration with all the fields filled: no data loss - self modification with all the fields filled: no data loss What else should be tested?
Created attachment 124683 [details] [review] Bug 28929: Add selenium tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Bug 28929: (follow-up) Add exec flag to tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 124684 [details] [review] Bug 28929: Prevent flags to be sent during patron's edition * selfreg and selfmod for OPAC * patron's edition on staff Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested OPAC and staff side. Prevents mangling flags column. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 124685 [details] [review] Bug 28929: Add selenium tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Bug 28929: (follow-up) Add exec flag to tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
First and third patches squashed. (In reply to Victor Grousset/tuxayo from comment #54) > Is the screenshot taking still useful in the test? > ↓↓↓ > $driver->capture_screenshot('selenium_failure_x.png'); Indeed, was for debugging, removed. (In reply to Victor Grousset/tuxayo from comment #56) > Ok, I can add a patch to remove that unless there is an objection. > > Also, I missed this on the first review: > > $driver->get($opac_base_url . 'opac-main.pl'); > > $driver->get($opac_base_url . 'opac-memberentry.pl'); > > Is going to the homepage useful before going directly via URL to the self > registration form? Indeed, I initially wanted to click the "Register here" from opac-main but it didn't work (?). I removed the first get.
Backported: Pushed to 20.05.x security branch for 20.05.16
Backported to 21.05.04, 20.11.10, 20.05.16, 19.11.22
Pushed to master for 21.11, thanks to everybody involved!