Bug 21325 - Prevent authentication when sending userid and password via querystring parameters
Summary: Prevent authentication when sending userid and password via querystring param...
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Marcel de Rooy
URL:
Keywords:
: 27199 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-09-07 15:13 UTC by Marcel de Rooy
Modified: 2021-04-22 19:51 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
This change may break custom or creative (yet insecure) authentication integration using GET requests. These auth requests do not exist in Koha, but they may be used by extensions, customizations, or clever end users.
Version(s) released in:
21.05.00


Attachments
Bug 21325: Prevent authentication when sending userid and password in querystring (2.12 KB, patch)
2020-12-24 01:32 UTC, David Cook
Details | Diff | Splinter Review
Bug 21325: Fix t/db_dependent/Auth.t test (1.22 KB, patch)
2020-12-24 01:44 UTC, David Cook
Details | Diff | Splinter Review
Bug 21325: Prevent authentication when sending userid and password in querystring (2.17 KB, patch)
2021-04-14 19:04 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21325: Fix t/db_dependent/Auth.t test (1.27 KB, patch)
2021-04-14 19:04 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21325: Prevent authentication when sending userid and password in querystring (2.27 KB, patch)
2021-04-16 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21325: Fix t/db_dependent/Auth.t test (1.37 KB, patch)
2021-04-16 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21325: Add tests (2.36 KB, patch)
2021-04-16 12:14 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 Marcel de Rooy 2018-09-07 15:13:19 UTC
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?
Comment 1 David Cook 2020-12-24 01:11:55 UTC
(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.
Comment 2 David Cook 2020-12-24 01:32:23 UTC
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
Comment 3 David Cook 2020-12-24 01:44:41 UTC
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.
Comment 4 David Cook 2020-12-24 01:46:32 UTC
It's a quick and dirty patch, but it works.
Comment 5 Owen Leonard 2021-04-14 19:04:30 UTC
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>
Comment 6 Owen Leonard 2021-04-14 19:04:34 UTC
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>
Comment 7 Marcel de Rooy 2021-04-16 07:35:20 UTC
QA: Looking here
Comment 8 Marcel de Rooy 2021-04-16 09:45:50 UTC
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>
Comment 9 Marcel de Rooy 2021-04-16 09:45:53 UTC
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>
Comment 10 Marcel de Rooy 2021-04-16 09:46:32 UTC
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.
Comment 11 Jonathan Druart 2021-04-16 12:14:11 UTC
Created attachment 119721 [details] [review]
Bug 21325: Add tests
Comment 12 Jonathan Druart 2021-04-16 13:12:07 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 13 Katrin Fischer 2021-04-17 13:46:49 UTC
I assume we should add something to the release notes about this as it might be a breaking change for some customizations?
Comment 14 David Cook 2021-04-19 00:15:56 UTC
(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.
Comment 15 David Cook 2021-04-19 01:48:51 UTC
*** Bug 27199 has been marked as a duplicate of this bug. ***
Comment 16 Nick Clemens 2021-04-19 12:35:21 UTC
Should we do the same for svc scripts?
https://wiki.koha-community.org/wiki/Koha_/svc/_HTTP_API#GET_.2Fsvc.2Fbib.2F.24biblio
Comment 17 David Cook 2021-04-19 23:27:44 UTC
(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.
Comment 18 Fridolin Somers 2021-04-22 14:29:48 UTC
I propose we don't backport to stable branches to avoid breaking existing authentications.
Comment 19 Katrin Fischer 2021-04-22 19:51:02 UTC
(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.