In testing bug 13779, I just noticed that an URL like this works: https://[your_domain]/cgi-bin/koha/opac-reserve.pl?biblionumber=[some_biblionumber]&userid=[your_userid]&password=[your_password] Obviously, we should not expose credentials like that. But it raises the question: should we allow it then?
(In reply to Marcel de Rooy from comment #0) > In testing bug 13779, I just noticed that an URL like this works: > > https://[your_domain]/cgi-bin/koha/opac-reserve. > pl?biblionumber=[some_biblionumber]&userid=[your_userid]&password=[your_passw > ord] > > Obviously, we should not expose credentials like that. But it raises the > question: should we allow it then? In my opinion, we 100% should not allow it.
Created attachment 114695 [details] [review] Bug 21325: Prevent authentication when sending userid and password in querystring This patch permits authentication via userid/password only when the HTTP method is POST when using C4::Auth::checkauth(). The goal is to stop people from supplying userid and password in querystrings in order to log into web pages. Test plan: 0. Do not apply patch yet 1. Open a new browser (ie we don't want any existing CGISESSID cookies available - opening a new tab/window isn't enough. It must be a new instance or you can clear your cookies) 2. Go to http://localhost:8080/cgi-bin/koha/opac-reserve.pl?biblionumber=29&userid=koha&password=koha 3. Note the user has been logged in and is being asked to confirm hold. 4. Apply the patch 5. Go to http://localhost:8080/cgi-bin/koha/opac-reserve.pl?biblionumber=29&userid=koha&password=koha 6. Note the user is not logged in and the user is presented with a login screen
Created attachment 114696 [details] [review] Bug 21325: Fix t/db_dependent/Auth.t test This patch fixes some warnings in t/db_dependent/Auth.t. Note that it doesn't add any tests.
It's a quick and dirty patch, but it works.
Created attachment 119600 [details] [review] Bug 21325: Prevent authentication when sending userid and password in querystring This patch permits authentication via userid/password only when the HTTP method is POST when using C4::Auth::checkauth(). The goal is to stop people from supplying userid and password in querystrings in order to log into web pages. Test plan: 0. Do not apply patch yet 1. Open a new browser (ie we don't want any existing CGISESSID cookies available - opening a new tab/window isn't enough. It must be a new instance or you can clear your cookies) 2. Go to http://localhost:8080/cgi-bin/koha/opac-reserve.pl?biblionumber=29&userid=koha&password=koha 3. Note the user has been logged in and is being asked to confirm hold. 4. Apply the patch 5. Go to http://localhost:8080/cgi-bin/koha/opac-reserve.pl?biblionumber=29&userid=koha&password=koha 6. Note the user is not logged in and the user is presented with a login screen Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 119601 [details] [review] Bug 21325: Fix t/db_dependent/Auth.t test This patch fixes some warnings in t/db_dependent/Auth.t. Note that it doesn't add any tests. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
QA: Looking here
Created attachment 119697 [details] [review] Bug 21325: Prevent authentication when sending userid and password in querystring This patch permits authentication via userid/password only when the HTTP method is POST when using C4::Auth::checkauth(). The goal is to stop people from supplying userid and password in querystrings in order to log into web pages. Test plan: 0. Do not apply patch yet 1. Open a new browser (ie we don't want any existing CGISESSID cookies available - opening a new tab/window isn't enough. It must be a new instance or you can clear your cookies) 2. Go to http://localhost:8080/cgi-bin/koha/opac-reserve.pl?biblionumber=29&userid=koha&password=koha 3. Note the user has been logged in and is being asked to confirm hold. 4. Apply the patch 5. Go to http://localhost:8080/cgi-bin/koha/opac-reserve.pl?biblionumber=29&userid=koha&password=koha 6. Note the user is not logged in and the user is presented with a login screen Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 119698 [details] [review] Bug 21325: Fix t/db_dependent/Auth.t test This patch fixes some warnings in t/db_dependent/Auth.t. Note that it doesn't add any tests. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Few minor questions/remarks: Should we add a warn close to the POST test when we encounter a GET request (or even another) ? Or silently ignore like we do now? (out of scope) While glancing thru Auth, I was surprised that we do not seem to check if the password is not empty. We always pass it to checkpw. You touched the test, but did not add tests. I think the benefits of this small change outweigh the lack of new tests. Lets see if RM thinks so too.
Created attachment 119721 [details] [review] Bug 21325: Add tests
Pushed to master for 21.05, thanks to everybody involved!
I assume we should add something to the release notes about this as it might be a breaking change for some customizations?
(In reply to Katrin Fischer from comment #13) > I assume we should add something to the release notes about this as it might > be a breaking change for some customizations? Not only customization but also creative authentication by end users. See Bug 27305. Nico was very creative using the querystring to authenticate Google Calendar with Koha. I originally worked on this to stop him from being able to do that, although I feel bad that we don't have a ready feature to replace it for him.
*** Bug 27199 has been marked as a duplicate of this bug. ***
Should we do the same for svc scripts? https://wiki.koha-community.org/wiki/Koha_/svc/_HTTP_API#GET_.2Fsvc.2Fbib.2F.24biblio
(In reply to Nick Clemens from comment #16) > Should we do the same for svc scripts? > https://wiki.koha-community.org/wiki/Koha_/svc/_HTTP_API#GET_.2Fsvc.2Fbib.2F. > 24biblio It would probably be a good idea although more likely to break things. I suppose we should actually double-check all instances of checkpw(). They're in checkauth() but also check_api_auth and a few other places like C4/ILSDI/Services.pm, opac/sco/sco-main.pl, etc.
I propose we don't backport to stable branches to avoid breaking existing authentications.
(In reply to Fridolin Somers from comment #18) > I propose we don't backport to stable branches to avoid breaking existing > authentications. I agree, this could block libraries depending on it from getting other bugfixes. Better to give them a little bit of time.