Bug 32178 - query parameters in check_api_auth lets anyone assume a user id
Summary: query parameters in check_api_auth lets anyone assume a user id
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Agustín Moyano
QA Contact: Nick Clemens (kidclamp)
URL:
Keywords:
: 32140 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-11-11 12:47 UTC by Agustín Moyano
Modified: 2023-10-14 12:32 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 32178: Remove security breach introduced in bug 31378 (7.63 KB, patch)
2022-11-11 22:33 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 32178: Remove security breach introduced in bug 31378 (7.69 KB, patch)
2022-11-13 23:42 UTC, David Cook
Details | Diff | Splinter Review
Bug 32178: Remove security breach introduced in bug 31378 (7.74 KB, patch)
2022-11-14 15:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32178: Remove security breach introduced in bug 31378 (7.80 KB, patch)
2022-11-14 15:54 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32178: (follow-up) Transform 'staff' interface to 'intranet' (2.27 KB, patch)
2022-11-15 12:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 32178: Adapt tests to new auth.session params (7.25 KB, patch)
2022-11-18 15:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32178: (QA follow-up) Correct modules import (781 bytes, patch)
2022-11-18 15:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Agustín Moyano 2022-11-11 12:47:37 UTC
As Jonathan specified in the comment https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31378#c268 query parameters introduced in check_api_auth by bug 31378 allow anyone to call any endpoint that uses this method to assume the identity of any user, by specifying the query parameter "userid"

That part of the code in check_api_auth must be removed, and another way to create a session for external IdP must be found.
Comment 1 Agustín Moyano 2022-11-11 22:33:37 UTC
Created attachment 143799 [details] [review]
Bug 32178: Remove security breach introduced in bug 31378

This patch removes a security breach in C4::Auth::check_api_auth introduced by bug 31378, where when someone called an api with the parameters userid and auth_client_login, check_api_auth would automatically asume the user calling was that userid.

This patch also introduces C4::Auth::create_basic_session(), a function that creates a session and adds the minimum basic parameters.
Comment 2 Martin Renvoize 2022-11-12 07:30:00 UTC
Did you see bug 31050?

It needs unit tests but introduces a very similar method but in the Koha:: namespace.. it would be great to merge these bugs.
Comment 3 Martin Renvoize 2022-11-12 07:32:11 UTC
Oh, I'm confused.. I was thinking of bug 28507
Comment 4 David Cook 2022-11-13 23:02:57 UTC
(In reply to Martin Renvoize from comment #2)
> Did you see bug 31050?
> 
> It needs unit tests but introduces a very similar method but in the Koha::
> namespace.. it would be great to merge these bugs.

They are pretty similar. In bug 31050, one difference is C4::Context->setup_session takes a "session" argument, since C4::Auth and C4::InstallAuth create the CGI::Session object in ways that weren't conducive to simple refactoring.

I'll test Agustin's patch as fixing it ASAP seems like a good idea. Maybe the next release cycle would be a good time to revive bug 31050. I let it hibernate since there wasn't an urgent need for it locally or globally.
Comment 5 David Cook 2022-11-13 23:24:31 UTC
Using https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31378#c268 I went to "http://localhost:8081/cgi-bin/koha/catalogue/getitem-ajax.pl?auth_client_login=1&userid=koha&itemnumber=73" and I did see the information leak. Fortunately, the authentication cookie isn't returned by that script, so you're not able to fully log in, but I imagine there's other callers of check_api_auth that would let you fully log in. 

So good to get a fix in. 

Now to test this patch...
Comment 6 David Cook 2022-11-13 23:34:47 UTC
*** Bug 32140 has been marked as a duplicate of this bug. ***
Comment 7 David Cook 2022-11-13 23:36:25 UTC
Test plan:
1) Apply patch
2) koha-plack --restart kohadev
3) Go to http://localhost:8081/cgi-bin/koha/catalogue/getitem-ajax.pl?auth_client_login=1&userid=koha&itemnumber=73
4) Get 403 error
5) Test that OIDC client still works

(Side note: It looks like this patch will also resolve the session "interface" issue I reported as bug 32140, although on the staff interface it says "staff" rather than "intranet"...)
Comment 8 David Cook 2022-11-13 23:41:19 UTC
Hmm I think maybe "Koha/REST/V1/OAuth/Client.pm" or C4::Auth::create_basic_session should translate "staff" to "intranet" for the "interface for $c->auth->session just to be on the safe side. 

Otherwise, the session in the database is looking good.
Comment 9 David Cook 2022-11-13 23:41:45 UTC
I think the "interface" issue could be fixed in a QA follow-up, so I'll still SO on this.
Comment 10 David Cook 2022-11-13 23:42:38 UTC
Created attachment 143816 [details] [review]
Bug 32178: Remove security breach introduced in bug 31378

This patch removes a security breach in C4::Auth::check_api_auth introduced by bug 31378, where when someone called an api with the parameters userid and auth_client_login, check_api_auth would automatically asume the user calling was that userid.

This patch also introduces C4::Auth::create_basic_session(), a function that creates a session and adds the minimum basic parameters.

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 11 David Cook 2022-11-13 23:58:50 UTC
A little disappointed that I didn't catch this problem sooner. 

I pointed out fairly early on that C4::Auth::check_api_auth() wasn't the right thing to use at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31378#c87 but I hadn't delved into the changes to check_api_auth. I was mostly focused on the OIDC functionality not working. By the time it was working, it had already been pushed to master, so I didn't bother doing a deeper dive into the code...

Kudos to Jonathan for his thoroughness. 

(Bugs like this are also why I advocate for having additional layers of security - like IP restrictions - around the staff interface.)
Comment 12 David Cook 2022-11-14 00:15:53 UTC
I don't have the time/energy to re-write C4::Auth alone, but if folk want to collaborate on that, I'd be happy to join. 

I'd suggest creating fully functional and unit tested Koha::Auth classes, and then replacing AuthN/AuthZ code in the OPAC, since there are only 93 calls to "get_template_and_user" in the OPAC, whereas there are about 467 calls in the rest of Koha.

Once we prove the new methodology in the OPAC, we could then add it into the staff interface and API. After that, we'd then delete C4::Auth.

In Bug 31380, I basically split "get_template_and_user" into authenticate(), authorize(), and prepare_template(). I need to go back and add more tests and fix a typo, but overall it works pretty well.
Comment 13 Marcel de Rooy 2022-11-14 14:22:11 UTC
(In reply to David Cook from comment #12)
> I don't have the time/energy to re-write C4::Auth alone, but if folk want to
> collaborate on that, I'd be happy to join. 
> 
> I'd suggest creating fully functional and unit tested Koha::Auth classes,
> and then replacing AuthN/AuthZ code in the OPAC, since there are only 93
> calls to "get_template_and_user" in the OPAC, whereas there are about 467
> calls in the rest of Koha.
> 
> Once we prove the new methodology in the OPAC, we could then add it into the
> staff interface and API. After that, we'd then delete C4::Auth.
> 
> In Bug 31380, I basically split "get_template_and_user" into authenticate(),
> authorize(), and prepare_template(). I need to go back and add more tests
> and fix a typo, but overall it works pretty well.

Sounds like a good plan, but time has proven that it is not as easy to accomplish. But it definitely needs a rewrite.
Problem with the gradual approach is that the transition period can easily become much too long.
Comment 14 Kyle M Hall 2022-11-14 15:38:55 UTC
Created attachment 143841 [details] [review]
Bug 32178: Remove security breach introduced in bug 31378

This patch removes a security breach in C4::Auth::check_api_auth introduced by bug 31378, where when someone called an api with the parameters userid and auth_client_login, check_api_auth would automatically asume the user calling was that userid.

This patch also introduces C4::Auth::create_basic_session(), a function that creates a session and adds the minimum basic parameters.

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Nick Clemens (kidclamp) 2022-11-14 15:54:18 UTC
Created attachment 143842 [details] [review]
Bug 32178: Remove security breach introduced in bug 31378

This patch removes a security breach in C4::Auth::check_api_auth introduced by bug 31378, where when someone called an api with the parameters userid and auth_client_login, check_api_auth would automatically asume the user calling was that userid.

This patch also introduces C4::Auth::create_basic_session(), a function that creates a session and adds the minimum basic parameters.

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Agustín Moyano 2022-11-15 12:19:21 UTC
Created attachment 143899 [details] [review]
Bug 32178: (follow-up) Transform 'staff' interface to 'intranet'
Comment 17 Tomás Cohen Arazi 2022-11-15 18:46:48 UTC
Ok, enough QA team members signed already :-D
Comment 18 Tomás Cohen Arazi 2022-11-15 22:26:32 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 19 Tomás Cohen Arazi 2022-11-18 15:02:19 UTC
Created attachment 144069 [details] [review]
Bug 32178: Adapt tests to new auth.session params

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi 2022-11-18 15:02:25 UTC
Created attachment 144070 [details] [review]
Bug 32178: (QA follow-up) Correct modules import

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Jonathan Druart 2022-11-22 07:46:53 UTC
Any future plans to now use this new subroutine from other places?
Comment 22 David Cook 2022-11-22 22:36:24 UTC
(In reply to Jonathan Druart from comment #21)
> Any future plans to now use this new subroutine from other places?

Patterned off bug 31050, this new subroutine would probably need to be refactored to add a "session" parameter, but C4::Auth::checkauth() and C4::Auth::check_api_auth() could both use it. Maybe C4::InstallAuth::checkauth() although that one is a very special case.

I don't have any immediate plans to write those patches, but I'd test/QA someone else's patches. If no one else writes them, I might be able to do it around February-April 2023.
Comment 23 David Cook 2023-06-02 03:24:08 UTC
*** Bug 31050 has been marked as a duplicate of this bug. ***