Bug 29914 - check_cookie_auth not strict enough
Summary: check_cookie_auth not strict enough
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 29957 30045 29915
  Show dependency treegraph
 
Reported: 2022-01-20 09:09 UTC by Jonathan Druart
Modified: 2022-02-08 12:26 UTC (History)
17 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.02,21.05.09,20.11.14,19.11.25


Attachments
Bug 29914: Make check_cookie_auth compare the userid (1.77 KB, patch)
2022-01-20 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29914: Make check_cookie_auth compare the userid (1.73 KB, patch)
2022-01-20 10:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29914: Make check_cookie_auth compare the userid (2.95 KB, patch)
2022-01-20 14:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29914: Make check_cookie_auth compare the userid (2.99 KB, patch)
2022-01-20 14:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29914: Add tests (1.87 KB, patch)
2022-01-21 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29914: Remove warn on timeout (1.27 KB, patch)
2022-01-21 10:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29914: (QA follow-up) Remove warn on timeout (1.23 KB, patch)
2022-01-21 10:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29914: Make check_cookie_auth compare the userid (3.08 KB, patch)
2022-01-24 10:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29914: Add tests (1.96 KB, patch)
2022-01-24 10:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29914: (QA follow-up) Remove warn on timeout (1.29 KB, patch)
2022-01-24 10:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29914: Add tests (1.98 KB, patch)
2022-01-24 14:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29914: (QA follow-up) Remove warn on timeout (1.31 KB, patch)
2022-01-24 14:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29914: (QA follow-up) Expand tests to cover failure case before patches (2.39 KB, patch)
2022-01-24 14:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29914: Make check_cookie_auth compare the userid (3.10 KB, patch)
2022-01-24 14:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29914: (QA follow-up) Add comment to explain last case (834 bytes, patch)
2022-01-24 14:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29914: (QA follow-up) Add comment to explain last case (902 bytes, patch)
2022-01-25 10:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29914: Remove 'Use of uninitialized value ' warnings (872 bytes, patch)
2022-01-25 10:58 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 Jonathan Druart 2022-01-20 09:09:24 UTC
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.
Comment 1 Jonathan Druart 2022-01-20 09:12:37 UTC
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.
Comment 2 Marcel de Rooy 2022-01-20 09:17:19 UTC
Looking here now
Comment 3 Marcel de Rooy 2022-01-20 09:33:18 UTC
We need some way of testing check_cookie_auth. Trying a bit.
Comment 4 Jonathan Druart 2022-01-20 09:54:25 UTC
(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.
Comment 5 Jonathan Druart 2022-01-20 10:12:42 UTC
I don't manage to open a breach, this may be only theoretical.
Comment 6 Marcel de Rooy 2022-01-20 10:29:27 UTC
Still something wrong.
Comment 7 Marcel de Rooy 2022-01-20 10:30:22 UTC
        } 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.
Comment 8 Martin Renvoize 2022-01-20 10:32:44 UTC
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?
Comment 9 Jonathan Druart 2022-01-20 10:43:10 UTC
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.
Comment 10 Marcel de Rooy 2022-01-20 12:38:25 UTC
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.
Comment 11 Jonathan Druart 2022-01-20 14:00:14 UTC
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.
Comment 12 Marcel de Rooy 2022-01-20 14:02:38 UTC
+        elsif ($status eq "anon") {
+            $cookie_auth = 1;

Does it compile ?
Comment 13 Marcel de Rooy 2022-01-20 14:03:03 UTC
+        elsif ($status eq "anon") {
+            $cookie_auth = 1;
         elsif ($status eq "maintenance") {
Comment 14 Jonathan Druart 2022-01-20 14:18:55 UTC
This is FQA, wait a bit please.
Comment 15 Jonathan Druart 2022-01-20 14:39:46 UTC
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.
Comment 16 Jonathan Druart 2022-01-20 14:41:34 UTC
Ready for testing again.
Comment 17 Marcel de Rooy 2022-01-20 14:59:14 UTC
I would like to see tests :)
Comment 18 Jonathan Druart 2022-01-21 08:35:22 UTC
Created attachment 129682 [details] [review]
Bug 29914: Add tests
Comment 19 Marcel de Rooy 2022-01-21 10:52:41 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2022-01-21 10:55:03 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2022-01-21 10:57:15 UTC
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.
Comment 22 Andrew Nugged 2022-01-21 13:40:08 UTC
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 :)...
Comment 23 Victor Grousset/tuxayo 2022-01-22 00:33:01 UTC
(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.
Comment 24 Victor Grousset/tuxayo 2022-01-22 00:39:34 UTC
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
Comment 27 Marcel de Rooy 2022-01-24 10:36:08 UTC
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>
Comment 28 Marcel de Rooy 2022-01-24 10:36:12 UTC
Created attachment 129719 [details] [review]
Bug 29914: Add tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2022-01-24 10:36:16 UTC
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>
Comment 30 Marcel de Rooy 2022-01-24 10:36:35 UTC
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.
Comment 31 Nick Clemens 2022-01-24 14:36:30 UTC
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>
Comment 32 Nick Clemens 2022-01-24 14:36:34 UTC
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>
Comment 33 Nick Clemens 2022-01-24 14:36:38 UTC
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>
Comment 34 Nick Clemens 2022-01-24 14:36:42 UTC
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>
Comment 35 Nick Clemens 2022-01-24 14:36:46 UTC
Created attachment 129729 [details] [review]
Bug 29914: (QA follow-up) Add comment to explain last case

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 36 Nick Clemens 2022-01-24 14:41:26 UTC
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
Comment 37 Victor Grousset/tuxayo 2022-01-25 05:10:33 UTC
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.
Comment 38 Fridolin Somers 2022-01-25 06:40:27 UTC
Thanks all for your work.
This will be included in January release.
Comment 39 Jonathan Druart 2022-01-25 10:58:26 UTC
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
Comment 40 Jonathan Druart 2022-01-25 10:58:32 UTC
Created attachment 129743 [details] [review]
Bug 29914: Remove 'Use of uninitialized value ' warnings
Comment 41 Jonathan Druart 2022-01-25 10:58:52 UTC
(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.
Comment 42 Kyle M Hall 2022-01-25 11:53:10 UTC
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
Comment 43 Kyle M Hall 2022-01-25 12:00:23 UTC
(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
Comment 44 Jonathan Druart 2022-01-25 12:48:49 UTC
(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`.
Comment 45 Marcel de Rooy 2022-01-25 12:55:31 UTC
(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 ;)
Comment 46 Kyle M Hall 2022-01-25 13:17:40 UTC
(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!
Comment 47 Victor Grousset/tuxayo 2022-01-25 21:35:31 UTC
Thanks for the followups Jonathan :)
Comment 48 Victor Grousset/tuxayo 2022-01-25 23:43:47 UTC
Backported: Pushed to 20.11.x-security branch for 20.11.14
Comment 49 wainuiwitikapark 2022-01-26 05:00:32 UTC
Backported to 19.11.x-security branch on the Security repo for 19.11.25 security release.
Comment 50 Andrew Fuerste-Henry 2022-01-28 14:15:05 UTC
Pushed to security/21.05.x for 21.05.09
Comment 51 Jonathan Druart 2022-02-08 09:56:21 UTC
We broke SCO completely here.
Comment 52 Jonathan Druart 2022-02-08 09:58:00 UTC
(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
Comment 53 Jonathan Druart 2022-02-08 11:12:14 UTC
(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.
Comment 54 Marcel de Rooy 2022-02-08 12:26:20 UTC
(In reply to Jonathan Druart from comment #51)
> We broke SCO completely here.

29543 is in the game too