In the OPAC, a user can see their own ILL requests in the OPAC at this URL: https://<opac>/cgi-bin/koha/opac-illrequests.pl Users can then click on "View" to see more details for one request: https://<opac>/cgi-bin/koha/opac-illrequests.pl?method=view&illrequest_id=4714 But by changing the illrequest_id at the end of that URL, the patron can see details of any and all ILL requests, not just their own. There is no standardized information about what user a request is connected to (since the display is designed for users looking at their own requests), but notes are also displayed, which might reveal the identity of the patron, or other sensitive information. There should be a check that the currently active patron is connected to the ILL request before details are displayed.
Created attachment 150892 [details] [review] Bug 33702; Patrons should only see their own ILLs in the OPAC To reproduce: - Enable the ILL module - Install the FreeForm backend as described here: https://wiki.koha-community.org/wiki/ILL_backends - Go to the ILL module and add two different ILL requests by clicking on "New ILL request" and entering the necessary details. - Make sure you connect the two requests to two *different* patrons in the field marked "Card number, username or surname" - Make the two titles different, and make a not of which title is connected to which patron - Log in as one of the two patrons who now have an ILL request each, in the OPAC - Go to the "Interlibrary loan requests" tab - Click on "View" for the request connected to this patron. The URL will look like something like this: http://<opac>/cgi-bin/koha/opac-illrequests.pl?method=view&illrequest_id=2 - Now change the number at the end to correspond to the the ILL request connected to the *other* patron - Verify you can see the details of an ILL request conncted to another patron than the patron you are logged in as To test: - Apply the patch - Restart all the things if you are testing with ktd - Reload the detail view of the ILL request that belongs to the patron you are not logged in as - Verify you are redirect to the 404 page and can not see the details of the request that belongs to the patron you are not logged in as
The patch above redirects patrons to the 404 page if they try to view an ILL request that does not belong to them. We could perhaps be nicer and display a more friendly "You are not allowed to see this" message, but I don't think there are any good reasons why anyone would end up at the URL of a request they are not supposed to see, so I think a plain old 404 should do the job.
Other actions are affected as well.
Created attachment 150923 [details] [review] Bug 33702: Patrons should only see their own ILLs in the OPAC To reproduce: - Enable the ILL module - Install the FreeForm backend as described here: https://wiki.koha-community.org/wiki/ILL_backends - Go to the ILL module and add two different ILL requests by clicking on "New ILL request" and entering the necessary details. - Make sure you connect the two requests to two *different* patrons in the field marked "Card number, username or surname" - Make the two titles different, and make a not of which title is connected to which patron - Log in as one of the two patrons who now have an ILL request each, in the OPAC - Go to the "Interlibrary loan requests" tab - Click on "View" for the request connected to this patron. The URL will look like something like this: http://<opac>/cgi-bin/koha/opac-illrequests.pl?method=view&illrequest_id=2 - Now change the number at the end to correspond to the the ILL request connected to the *other* patron - Verify you can see the details of an ILL request conncted to another patron than the patron you are logged in as To test: - Apply the patch - Restart all the things if you are testing with ktd - Reload the detail view of the ILL request that belongs to the patron you are not logged in as - Verify you are redirect to the 404 page and can not see the details of the request that belongs to the patron you are not logged in as Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 150924 [details] [review] Bug 33702: Prevent ILL requests to be modified by somebody else Same as previous patch, but for 'update' and 'cancreq'. We remove the redirect, but here we only want to focus on the security fix. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
This looks very bad.
It affects all stable branches. We should release the last 21.05.x with this fix.
Created attachment 150939 [details] [review] Bug 33702: Prevent ILL requests to be modified by somebody else Same as previous patch, but for 'update' and 'cancreq'. We remove the redirect, but here we only want to focus on the security fix. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Confirmed. Without this patch a patron can modify and cancel any ILL request in the OPAC. With this patch the patron is redirected to the 404 page if modification or cancellation is attempted.
The code currently in master uses code like this to find and display details of an ILL request: my $request = Koha::Illrequests->find({ borrowernumber => $loggedinuser, illrequest_id => $params->{illrequest_id} }); I wonder if this was intended to only find requests connected to the currently logged in patron, because of the "borrowernumber => $loggedinuser,"? Is "find" supposed to work that way? I guess the real question is if a false assumption was made about how find works, or if find is not working the way it should?
It should either be my $request = Koha::Illrequests->search({ borrowernumber => $loggedinuser, illrequest_id => $params->{illrequest_id} })->next; Or what I suggested in the previous patch. Find is searching on a unique index or PK, here we pass the PK (illrequest_id) and so the other parameters are ignored.
Looking here
Created attachment 151111 [details] [review] Bug 33702: Patrons should only see their own ILLs in the OPAC To reproduce: - Enable the ILL module - Install the FreeForm backend as described here: https://wiki.koha-community.org/wiki/ILL_backends - Go to the ILL module and add two different ILL requests by clicking on "New ILL request" and entering the necessary details. - Make sure you connect the two requests to two *different* patrons in the field marked "Card number, username or surname" - Make the two titles different, and make a not of which title is connected to which patron - Log in as one of the two patrons who now have an ILL request each, in the OPAC - Go to the "Interlibrary loan requests" tab - Click on "View" for the request connected to this patron. The URL will look like something like this: http://<opac>/cgi-bin/koha/opac-illrequests.pl?method=view&illrequest_id=2 - Now change the number at the end to correspond to the the ILL request connected to the *other* patron - Verify you can see the details of an ILL request conncted to another patron than the patron you are logged in as To test: - Apply the patch - Restart all the things if you are testing with ktd - Reload the detail view of the ILL request that belongs to the patron you are not logged in as - Verify you are redirect to the 404 page and can not see the details of the request that belongs to the patron you are not logged in as Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 151112 [details] [review] Bug 33702: Prevent ILL requests to be modified by somebody else Same as previous patch, but for 'update' and 'cancreq'. We remove the redirect, but here we only want to focus on the security fix. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Confirmed. Without this patch a patron can modify and cancel any ILL request in the OPAC. With this patch the patron is redirected to the 404 page if modification or cancellation is attempted. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 151113 [details] [review] Bug 33702: (QA follow-up) Do not crash on borrowernumber Resolve: Can't call method "borrowernumber" on an undefined value at /usr/share/koha/opac/opac-illrequests.pl line 66 Test plan: Put an unexisting illrequest_id in the URL parameter. You should see a 404, not a crash. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
What happened here? It's part of 22.11.x only now? RMaints?
Backported to 22.05.x for 22.05.13
(In reply to Lucas Gass from comment #16) > Backported to 22.05.x for 22.05.13 hi team, all 3 following packages include the 3x 33702 patches 22.11.06 22.05.13 21.11.21
Still missing in 21.05 then?