Bug 12954

Summary: Questionable logic regarding session handling in C4::Auth::checkauth()
Product: Koha Reporter: Jacek Ablewicz <abl>
Component: AuthenticationAssignee: Jacek Ablewicz <abl>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P3 CC: chris, dcook, dpavlin, fridolin.somers, jonathan.druart, katrin.fischer, martin.renvoize, mtj, mtompset, rkopaczka, robin, tomascohen, veron
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12951
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10952
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 13499, 13521    
Bug Blocks:    
Attachments: Bug 12954 - Questionable logic regarding session handling in C4::Auth::checkauth()
Bug 12954 - Questionable logic regarding session handling in C4::Auth::checkauth()
[Alternate Patch] Bug 12954: Failed login should retain anonymous session
Bug 12954: Failed login should retain anonymous session
Bug 12954: Failed login should retain anonymous session
Bug 12954: Failed login should retain anonymous session (3.16.x)

Description Jacek Ablewicz 2014-09-18 09:46:21 UTC
In case of the unsuccessfull log-in attempt (non-existing user ID, bad password, or both), session params are currently (since Bug 10952 got pushed) being set like that in checkauth():

- lasttime & ip params are set,
- number, id, cardnumber, firstname etc. are not set,
- sessiontype is NOT set to 'anon'

So the user session is essentially left in semi-authorised, semi-anonymous state. If then such an user goes directly to any page which does require authorisation/authentification, checkout() would treat this user as authentificated one, and would return prematurely without redirection to the login screen. This also typically (?) leads to get_template_and_user() being bypassed: it does return to the script from which it was called from (but with empty $borrowernumber return value).
In case of the (e.g.) opac/opac-user.pl script, observable effect of this bug is an DBI error:

opac-user.pl: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1 at /home/koha/devkohaclone/C4/Letters.pm line 200

(because C4::Letters::getalert() doesn't like to be called with empty $borrowernumber).

To replicate:

1/ try to log in with some nonexisting user id or wrong password in OPAC
2/ go directly to the opac/opac-user.pl page (e.g., enter it in the browser address bar, or just click on the "Log in" link); clicking on "Log in" button, when "Login" field is empty would also probably trigger this error
3/ observe DBI error displayed on the screen
4/ you are now in the "deadloop" of sorts (opac/opac-user.pl refuses to display the login screen, no matter how many times you try to reload it); to break the deadloop, one needs to:
- remove session cookie from the browser (or cause the session to expire in some other way - closing browser window would be probably enough for that)
- remove offending session on the server (from mysql sessions table, ..)
- log in with proper credentials using some other page (like opac/opac-main.pl right-side panel), which does not involve opac/opac-user.pl being called without "userid" CGI parameter.

Another way to replicate:

1/ try to log in with some nonexisting user id or wrong password in OPAC
2/ go to the staff interface
3/ observe that login screen displayed is different (not the "normal" one, but one with some additional buttons for loging-out etc. + "You do not have permision to access this page" message; see Bug 12951 attachment 31689 [details] for reference).
Comment 1 Jacek Ablewicz 2014-09-18 10:40:10 UTC Comment hidden (obsolete)
Comment 2 Jacek Ablewicz 2014-09-18 10:46:03 UTC
(In reply to Jacek Ablewicz from comment #0)

> authorisation/authentification, checkout() would treat this user as

s/checkout/checkauth/
Comment 3 Marc Véron 2014-12-19 14:40:38 UTC
I could replicate the issue:

Login to Opac with wrong credentials
- username only
- password only
- wrong username and/or password

See page witn message "You entered an incorrect username or password."

Hit Button "Login"

Result:

Software error:
Can't use an undefined value as an ARRAY reference at /usr/lib/perl5/DBI.pm line 2054.
Comment 4 Marc Véron 2014-12-19 14:50:57 UTC
With patch the error can no longer be replicated.

Switch to "Needs Signoff"
Comment 5 Marc Véron 2014-12-19 14:56:45 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2014-12-27 16:01:46 UTC
Martin, could you take a look at this please?
Comment 7 Martin Renvoize 2014-12-29 13:56:46 UTC
Comment on attachment 34551 [details] [review]
Bug 12954 - Questionable logic regarding session handling in C4::Auth::checkauth()

Review of attachment 34551 [details] [review]:
-----------------------------------------------------------------

First comments, but nothing really important so far..

Still working my way through the logic for the second part

::: C4/Auth.pm
@@ +780,4 @@
>          }
>          elsif ( !$lasttime || ($lasttime < time() - $timeout) ) {
>              # timed logout
> +            $info{'timed_out'} = 1 if $lasttime;

This feels unrelated to me, as such I'de rather see it in it's own bug rather than confusing the issue here.
Comment 8 Martin Renvoize 2014-12-29 14:01:53 UTC
I think I would prefer to see this done 'properly', i.e. setting the session to anon and ensuring it's tested for to prevent the loop we're finding here.

I'll have a play around and see if I can achieve this, unless you want to refine your attempt Jacek
Comment 9 Martin Renvoize 2014-12-30 14:12:27 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize 2014-12-30 14:15:19 UTC
Comment on attachment 34829 [details] [review]
[Alternate Patch] Bug 12954: Failed login should retain anonymous session

I've submitted an alternate patch, as the original broke cross login attempt anonymous search history.

It took a while to get my head around the code, but actually it looks to be a more trivial patch than initially submitted.

I've based my patch on the Tidied Auth.pm available in bug 13499
Comment 11 Mark Tompsett 2014-12-31 07:19:00 UTC
The test plan needs to include combinations:
- username/password
- cardnumber/password
- bad username/password
- username/bad password
- cardnumber/bad password
Comment 12 Mark Tompsett 2014-12-31 07:20:12 UTC
(In reply to Martin Renvoize from comment #7)

> > +            $info{'timed_out'} = 1 if $lasttime;
> 
> This feels unrelated to me, as such I'de rather see it in it's own bug
> rather than confusing the issue here.

I agree, please make another bug.
Comment 13 Mark Tompsett 2014-12-31 07:26:51 UTC
Comment on attachment 34551 [details] [review]
Bug 12954 - Questionable logic regarding session handling in C4::Auth::checkauth()

Review of attachment 34551 [details] [review]:
-----------------------------------------------------------------

::: C4/Auth.pm
@@ +1060,5 @@
> +                if ($session) {
> +                    $session->delete();
> +                    $session->flush;
> +                }
> +                C4::Context->_unset_userenv($sessionID) if $sessionID;

I'm not sure moving this outside the if ($userid) check is right. I vaguely recall strangeness with all the 5 test cases that I recommended when I was testing a custom authentication for our organization in terms of setting of userid and return and sessionID. I haven't tried this, but this code change seems wrong to me. I'll possibly attempt later.
Comment 14 Martin Renvoize 2014-12-31 10:25:33 UTC
Any chance you could look at my alternative patch too Mark...  I would fail qa on the first patch as it broke cross login attempt anonymous search history for me.. which is a regression, I left it there initially so people could see the difference between aproaches adn check whether I'de missed any fringe cases.

Test plan:
We need to test for the DBI error, and whether search history if maintained accross the login/failed login boundary.

Before Patch:
Good username   / Good password = Login with history maintained
Good cardnumber / Good password = Login with history maintained
Good username   / Bad password  = DBI Error
Good cardnumber / Bad password  = DBI Error
Bad username    / Good password = DBI Error
Bad cardnumber  / Good password = DBI Error

After Patch:
Good username   / Good password = Login with history maintained
Good cardnumber / Good password = Login with history maintained
Good username   / Bad password  = Bad password error, anon search history maintained
Good cardnumber / Bad password  = Bad password error, anon search history maintained
Bad username    / Good password = Bad password error, anon search history maintained
Bad cardnumber  / Good password = Bad password error, anon search history maintained
Comment 15 Chris Cormack 2015-01-18 05:28:23 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2015-01-20 16:01:56 UTC
Created attachment 35412 [details] [review]
Bug 12954: Failed login should retain anonymous session

A failed login should not leave the user in a half logged authenticated
state, but rather return them to an anonymouse session as per the
pre-login attempt state.

To replicate error:
1. Try to log in with some nonexisting user id or wrong password in the
   OPAC
2. Go directly to /opac-user.pl (e.g., enter it in the browser address
   bar, or just click on the "Log in" link)
3. Observe a DBI error displayed on the screen
4. You are now in the "deadloop" of sorts (opac/opac-user.pl refuses to
   display the login screen, no matter how many times you try to reload
   it); to break the deadloop, one needs to:
   - remove session cookie from the browser (or cause the session to
     expire in some other way - closing browser window would be probably
     enough for that)
   - remove offending session on the server (from mysql sessions table,
    ..)
   - log in with proper credentials using some other page (like
     opac/opac-main.pl right-side panel), which does not involve
     opac/opac-user.pl being called without "userid" CGI parameter.

To test:
1. Test as above, the DBI error should no longer be present
2. Check that search history works across failed and sucessful login
   attempts

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 17 Jonathan Druart 2015-01-20 16:03:19 UTC
This is a very bad bug!
I tried to fix something in opac-user.pl, to avoid the script to continue (a redirect if get_template_and_user does not return a $borrowernumber?), but don't know how to do it correctly.
I confirm this patch fixes the issues.

Marked as Passed QA.
Comment 18 Tomás Cohen Arazi 2015-01-24 19:29:21 UTC
Patch pushed to master.

Thanks Martin!
Comment 19 Chris Cormack 2015-02-02 18:49:23 UTC
Pushed to 3.18.x will be in 3.18.4
Comment 20 Chris Cormack 2015-02-05 01:00:55 UTC
Turns out this is really nasty, this bug allows you to bypass the OpacPublic setting.

If you have opacpublic switched off, and you try an invalid login, and then hit the opac again, you can now see the opac .. which you shouldn't be able to do.

This patch needs to be backported to 3.14.x and 3.16.x.

Shifting the search history to the session from a cookie accidentally caused this problem.
Comment 21 Fridolin Somers 2015-03-11 11:21:37 UTC
(In reply to Chris Cormack from comment #20)
> 
> This patch needs to be backported to 3.14.x and 3.16.x.

Indeed.
Do we agree that we can backport this bug without backporting its dependancies : Bug 13499 and Bug 13521 ?
Comment 22 Mark Tompsett 2015-03-11 12:49:59 UTC
(In reply to Fridolin SOMERS from comment #21)
> (In reply to Chris Cormack from comment #20)
> > 
> > This patch needs to be backported to 3.14.x and 3.16.x.
> 
> Indeed.
> Do we agree that we can backport this bug without backporting its
> dependancies : Bug 13499 and Bug 13521 ?

13499 is a perl tidy, and 13521 is fixing two tiny bugs.
I don't see why you wouldn't backport 13521.
Comment 23 Fridolin Somers 2015-03-23 14:44:13 UTC
(In reply to M. Tompsett from comment #22)
> 
> 13499 is a perl tidy, and 13521 is fixing two tiny bugs.
> I don't see why you wouldn't backport 13521.

Ok, Bug 13521 is very simple, it can be backported indeed
Comment 24 Mason James 2015-04-09 03:34:59 UTC
Created attachment 37595 [details] [review]
Bug 12954: Failed login should retain anonymous session (3.16.x)

A failed login should not leave the user in a half logged authenticated
state, but rather return them to an anonymouse session as per the
pre-login attempt state.

To replicate error:
1. Try to log in with some nonexisting user id or wrong password in the
   OPAC
2. Go directly to /opac-user.pl (e.g., enter it in the browser address
   bar, or just click on the "Log in" link)
3. Observe a DBI error displayed on the screen
4. You are now in the "deadloop" of sorts (opac/opac-user.pl refuses to
   display the login screen, no matter how many times you try to reload
   it); to break the deadloop, one needs to:
   - remove session cookie from the browser (or cause the session to
     expire in some other way - closing browser window would be probably
     enough for that)
   - remove offending session on the server (from mysql sessions table,
    ..)
   - log in with proper credentials using some other page (like
     opac/opac-main.pl right-side panel), which does not involve
     opac/opac-user.pl being called without "userid" CGI parameter.

To test:
1. Test as above, the DBI error should no longer be present
2. Check that search history works across failed and sucessful login
   attempts

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 25 Mason James 2015-04-09 03:42:33 UTC
hi Fridolin, you might want to pull my 3.16 patch
Comment 26 Mason James 2015-04-09 03:44:10 UTC
Pushed to 3.16.x, will be in 3.16.10
Comment 27 Fridolin Somers 2015-06-18 07:43:38 UTC
Pushed to 3.14.x, will be in 3.14.16