This has been found on bug 28786 comment 91. check_cookie_auth is assuming that the user is authenticated if a cookie exists and that the login/username exists in the DB. This is very bad! I have no idea what are the possible breaches it opens, but there are certainly some.
Created attachment 129634 [details] [review] Bug 29914: Make check_cookie_auth compare the userid check_cookie_auth is assuming that the user is authenticated if a cookie exists and that the login/username exists in the DB. So basically if you hit the login page, fill the login input with a valid username, click "login" => A cookie will be generated, and the sessions table will contain a line with this session id. On the second hit, if the username is in the DB, it will be enough to be considered authenticated.
Looking here now
We need some way of testing check_cookie_auth. Trying a bit.
(In reply to Marcel de Rooy from comment #3) > We need some way of testing check_cookie_auth. Trying a bit. I was planning to provide a selenium test on bug 28786. But providing a unit test here is certainly good as well.
I don't manage to open a breach, this may be only theoretical.
Still something wrong.
} elsif ( $session->param("id") eq $userid ) { warn "L1703 undef " if ! defined $session->param('id'); warn "L1704 e.s. " if defined $session->param('id') && !$session->param('id'); warn "L1705 $userid - ".$session->param('number'),'-'. $session->param('id'); $session->param( 'lasttime', time() ); my $flags = defined($flagsrequired) ? haspermission( $userid, $flagsrequired ) : 1; if ($flags) { warn "L1711 ok"; return ( "ok", $session ); } [2022/01/20 10:27:38] [WARN] Use of uninitialized value in string eq at /usr/share/koha/C4/Auth.pm line 1703. [2022/01/20 10:27:38] [WARN] Use of uninitialized value $userid in string eq at /usr/share/koha/C4/Auth.pm line 1703. [2022/01/20 10:27:38] [WARN] L1703 undef at /usr/share/koha/C4/Auth.pm line 1704. [2022/01/20 10:27:38] [WARN] Use of uninitialized value $userid in concatenation (.) or string at /usr/share/koha/C4/Auth.pm line 1706. [2022/01/20 10:27:38] [WARN] Use of uninitialized value in concatenation (.) or string at /usr/share/koha/C4/Auth.pm line 1706. [2022/01/20 10:27:38] [WARN] Use of uninitialized value in concatenation (.) or string at /usr/share/koha/C4/Auth.pm line 1706. [2022/01/20 10:27:38] [WARN] L1705 - - at /usr/share/koha/C4/Auth.pm line 1706. [2022/01/20 10:27:38] [WARN] L1711 ok at /usr/share/koha/C4/Auth.pm line 1711. [2022/01/20 10:27:38] [WARN] Use of uninitialized value $s_userid in string ne at /usr/share/koha/C4/Auth.pm line 878. You are comparing undef with undef. Giving lots of warns but they are euqal.
Man I wish you could review with context in bugzilla.. I don't understand your patch.. On line 1684 we have `my $userid = $session->param('id');` Then with your patch, we check `$session->param('id') eq $userid`.. but I don't see anything that changes either of those params between.. so they will always be equal won't they?
Created attachment 129643 [details] [review] Bug 29914: Make check_cookie_auth compare the userid check_cookie_auth is assuming that the user is authenticated if a cookie exists and that the login/username exists in the DB. So basically if you hit the login page, fill the login input with a valid username, click "login" => A cookie will be generated, and the sessions table will contain a line with this session id. On the second hit, if the username is in the DB, it will be enough to be considered authenticated.
I am thinking that we are not in the right direction yet. API does this: if ($status eq "ok") { $user = Koha::Patrons->find( $session->param('number') ) unless $session->param('sessiontype') and $session->param('sessiontype') eq 'anon'; $cookie_auth = 1; } When you enabled RESTPublicAnonymousRequests, the API expects an OK for an anonymous session. This change here makes the public API fail. Perhaps we should not call this a bug, but start with documenting better what we exactly expect from check_cookie_auth. And we should look again at the Auth.pm fixes on bug 28786 instead. It seems that these changes do not interpret the response of check_cookie_auth correctly? Thats another perspective.
Created attachment 129652 [details] [review] Bug 29914: Make check_cookie_auth compare the userid check_cookie_auth is assuming that the user is authenticated if a cookie exists and that the login/username exists in the DB. So basically if you hit the login page, fill the login input with a valid username, click "login" => A cookie will be generated, and the sessions table will contain a line with this session id. On the second hit, if the username is in the DB, it will be enough to be considered authenticated.
+ elsif ($status eq "anon") { + $cookie_auth = 1; Does it compile ?
+ elsif ($status eq "anon") { + $cookie_auth = 1; elsif ($status eq "maintenance") {
This is FQA, wait a bit please.
Created attachment 129664 [details] [review] Bug 29914: Make check_cookie_auth compare the userid check_cookie_auth is assuming that the user is authenticated if a cookie exists and that the login/username exists in the DB. So basically if you hit the login page, fill the login input with a valid username, click "login" => A cookie will be generated, and the sessions table will contain a line with this session id. On the second hit, if the username is in the DB, it will be enough to be considered authenticated.
Ready for testing again.
I would like to see tests :)
Created attachment 129682 [details] [review] Bug 29914: Add tests
Created attachment 129693 [details] [review] Bug 29914: Remove warn on timeout The value of the system preference 'timeout' is not correct, defaulting to 600. Caused by previous test. Actually an omission in another sub that does not seem to support 10x.
(In reply to Marcel de Rooy from comment #19) > Created attachment 129693 [details] [review] [review] > Bug 29914: Remove warn on timeout > > The value of the system preference 'timeout' is not correct, defaulting to > 600. > > Caused by previous test. Actually an omission in another sub that > does not seem to support 10x. No, reading it badly. But the fix is ok.
Created attachment 129694 [details] [review] Bug 29914: (QA follow-up) Remove warn on timeout The warn is: The value of the system preference 'timeout' is not correct, defaulting to 600. Caused by previous test.
seems this also was the reason for that long-time present "WARN": [WARN] Use of uninitialized value $s_userid in string ne at /usr/share/koha/lib/C4/Auth.pm line 878. - I started once pre-analysis, that happened because some anonymous sessions firstly anyway created then on second pass re-used and led through wrong if's set (or something like that) - didn't yet get deeper, because it's a very responsible place to do any experiments :)...
(I marked an obsolete an old version of a patch that had a commit message change.) As for testing what can be done? > I don't manage to open a breach, this may be only theoretical. So running the tests? And checking auth for staff, OPAC and self-checkout still works.
Running the tests before and after the patchset shows a new warning. Use of uninitialized value $remote_addr in string ne at /kohadevbox/koha/C4/Auth.pm line 1698. It happens consistently but the line doesn't seems related to the patch changes :o
Created attachment 129718 [details] [review] Bug 29914: Make check_cookie_auth compare the userid check_cookie_auth is assuming that the user is authenticated if a cookie exists and that the login/username exists in the DB. So basically if you hit the login page, fill the login input with a valid username, click "login" => A cookie will be generated, and the sessions table will contain a line with this session id. On the second hit, if the username is in the DB, it will be enough to be considered authenticated. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129719 [details] [review] Bug 29914: Add tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129720 [details] [review] Bug 29914: (QA follow-up) Remove warn on timeout The warn is: The value of the system preference 'timeout' is not correct, defaulting to 600. Caused by previous test. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested public API. Ran some api/v1 tests. Tested file upload. Checked the code in various places calling check_cookie_auth. Found one problem; solved on other bug. Looks good to me.
Created attachment 129725 [details] [review] Bug 29914: Add tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 129726 [details] [review] Bug 29914: (QA follow-up) Remove warn on timeout The warn is: The value of the system preference 'timeout' is not correct, defaulting to 600. Caused by previous test. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 129727 [details] [review] Bug 29914: (QA follow-up) Expand tests to cover failure case before patches When asking for permissions we get 'failed', without we get 'ok' Adding explicit checks for not 'ok' Add a FIXME: We should cover the case where we return 'failed' after changes, but that is a larger undertaking Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 129728 [details] [review] Bug 29914: Make check_cookie_auth compare the userid check_cookie_auth is assuming that the user is authenticated if a cookie exists and that the login/username exists in the DB. So basically if you hit the login page, fill the login input with a valid username, click "login" => A cookie will be generated, and the sessions table will contain a line with this session id. On the second hit, if the username is in the DB, it will be enough to be considered authenticated. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 129729 [details] [review] Bug 29914: (QA follow-up) Add comment to explain last case Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
I was able to get the 'OK' before patches, but not to exploit anything, seems most calls also rely on username/borrowernumber form user env which wasn't set - I think checkoutnotes may be vulnerable, but you need to know issueid etc. Hit most of the scripts that call this sub and all seems to work after the patches Added a specific test to cover the failure case before patches and a comment to clarify the 'failed' case
from the commit: (QA follow-up) Add comment to explain last case > # could be an 'else' at 1710, but left here to catch any errors Is there an alternative to having a line number? It will certainly move and quickly add confusion. Like for 20.11.x there are 100 lines difference in the line number for the same code in this part of the file. What about this? : «this else case could have been after the above "if ($flags)" but left here to catch any errors» I get two additional warnings with the patch, are they easy to fix? Use of uninitialized value $remote_addr in string ne at /kohadevbox/koha/C4/Auth.pm line 1827. Use of uninitialized value $remote_addr in string ne at /kohadevbox/koha/C4/Auth.pm line 1827.
Thanks all for your work. This will be included in January release.
Created attachment 129742 [details] [review] Bug 29914: (QA follow-up) Add comment to explain last case Signed-off-by: Nick Clemens <nick@bywatersolutions.com> JD amended patch: remove ref to line number
Created attachment 129743 [details] [review] Bug 29914: Remove 'Use of uninitialized value ' warnings
(In reply to Victor Grousset/tuxayo from comment #37) > from the commit: (QA follow-up) Add comment to explain last case > > > # could be an 'else' at 1710, but left here to catch any errors > > Is there an alternative to having a line number? It will certainly move and > quickly add confusion. > Like for 20.11.x there are 100 lines difference in the line number for the > same code in this part of the file. > > What about this? : > «this else case could have been after the above "if ($flags)" but left here > to catch any errors» Adjusted. > I get two additional warnings with the patch, are they easy to fix? > > Use of uninitialized value $remote_addr in string ne at > /kohadevbox/koha/C4/Auth.pm line 1827. > Use of uninitialized value $remote_addr in string ne at > /kohadevbox/koha/C4/Auth.pm line 1827. Fixed.
Jonathan, can you provide a rebase of 29914 on top of security/21.11.x? I tried but I keep getting the error "Auth ERROR: Cannot get_session() at /kohadevbox/koha/C4/Auth.pm line 1003." when I run Auth.t
(In reply to Kyle M Hall from comment #42) > Jonathan, can you provide a rebase of 29914 on top of security/21.11.x? I > tried but I keep getting the error "Auth ERROR: Cannot get_session() at > /kohadevbox/koha/C4/Auth.pm line 1003." when I run Auth.t I put in the wrong error. Here is the correct error: t/db_dependent/Auth.t .. 1/24 OPAC: Database update needed, redirecting to maintenance. Database is 20.1113000 and Koha is 21.1101004 at /kohadevbox/koha/C4/Auth.pm line 768. Un-mocked method 'redirect()' called at /kohadevbox/koha/C4/Auth.pm line 769. A context appears to have been destroyed without first calling release(). Based on $@ it does not look like an exception was thrown (this is not always a reliable test) This is a problem because the global error variables ($!, $@, and $?) will not be restored. In addition some release callbacks will not work properly from inside a DESTROY method. Here are the context creation details, just in case a tool forgot to call release(): File: t/db_dependent/Auth.t Line: 112 Tool: Test::More::subtest Cleaning up the CONTEXT stack... # Test ended with extra hubs on the stack! # Looks like you planned 24 tests but ran 1. t/db_dependent/Auth.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 23/24 subtests Test Summary Report ------------------- t/db_dependent/Auth.t (Wstat: 65280 Tests: 1 Failed: 0) Non-zero exit status: 255 Parse errors: Bad plan. You planned 24 tests but ran 1. Files=1, Tests=1, 1 wallclock secs ( 0.01 usr 0.00 sys + 1.08 cusr 0.10 csys = 1.19 CPU) Result: FAIL
(In reply to Kyle M Hall from comment #43) Hi Kyle, I have checked out security/21.11.x, applied the missing patches, reset_all and prove t/db_dependent/Auth.t passes for me. Applying: Bug 29914: (QA follow-up) Expand tests to cover failure case before patches Applying: Bug 29914: (QA follow-up) Add comment to explain last case Applying: Bug 29914: Remove 'Use of uninitialized value ' warnings > (In reply to Kyle M Hall from comment #42) > > Jonathan, can you provide a rebase of 29914 on top of security/21.11.x? I > > tried but I keep getting the error "Auth ERROR: Cannot get_session() at > > /kohadevbox/koha/C4/Auth.pm line 1003." when I run Auth.t > > I put in the wrong error. Here is the correct error: > > t/db_dependent/Auth.t .. 1/24 OPAC: Database update needed, redirecting to > maintenance. Database is 20.1113000 and Koha is 21.1101004 at Did you see this error? Try `updatedatabase`.
(In reply to Jonathan Druart from comment #40) > Created attachment 129743 [details] [review] [review] > Bug 29914: Remove 'Use of uninitialized value ' warnings + $ENV{REMOTE_ADDR} = '127.0.0.1'; + Thats not a fix to be proud of ;)
(In reply to Marcel de Rooy from comment #45) > (In reply to Jonathan Druart from comment #40) > > Created attachment 129743 [details] [review] [review] [review] > > Bug 29914: Remove 'Use of uninitialized value ' warnings > > + $ENV{REMOTE_ADDR} = '127.0.0.1'; > + > > Thats not a fix to be proud of ;) Working! Thanks yinz!
Thanks for the followups Jonathan :)
Backported: Pushed to 20.11.x-security branch for 20.11.14
Backported to 19.11.x-security branch on the Security repo for 19.11.25 security release.
Pushed to security/21.05.x for 21.05.09
We broke SCO completely here.
(In reply to Jonathan Druart from comment #51) > We broke SCO completely here. * print slip is broken * whole SCO is broken if AutoSelfCheckAllowed = don't allow
(In reply to Jonathan Druart from comment #52) > (In reply to Jonathan Druart from comment #51) > > We broke SCO completely here. > > * print slip is broken > * whole SCO is broken if AutoSelfCheckAllowed = don't allow Only print slip is broken actually, see bug 30045.
(In reply to Jonathan Druart from comment #51) > We broke SCO completely here. 29543 is in the game too