Bug 5995 - Glitch with checkauth
Summary: Glitch with checkauth
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: PATCH-Sent (DO NOT USE) blocker (vote)
Target Milestone: Oldversions
Assignee: Matthias Meusburger
QA Contact: Bugs List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 5992
  Show dependency treegraph
 
Reported: 2011-03-29 21:03 UTC by Chris Cormack
Modified: 2018-12-16 19:31 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed Patch (3.72 KB, patch)
2011-03-29 21:19 UTC, Chris Cormack
Details | Diff | Splinter Review
Signed off patch (3.77 KB, patch)
2011-05-12 23:37 UTC, Chris Cormack
Details | Diff | Splinter Review
Propose Followup Patch (3.13 KB, patch)
2011-10-14 20:42 UTC, Ian Walls
Details | Diff | Splinter Review
Another proposed followup patch (1.58 KB, patch)
2011-10-18 14:24 UTC, Ian Walls
Details | Diff | Splinter Review
[SIGN-OFF]Bug 5995 Follow up: variable scope in koha/sco/sco-main.pl (1.65 KB, patch)
2011-10-21 16:55 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2011-03-29 21:03:42 UTC

    
Comment 1 Chris Cormack 2011-03-29 21:19:37 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2011-05-05 13:39:35 UTC
The patch says "Users could log in intranet using their cardnumber, with superlibrarian rights."

I don't find this to be true for just any patron. Under what circumstances is this the case?
Comment 3 Paul Poulain 2011-05-09 16:00:02 UTC
Owen: this is the case only if the library has a CAS server for authentication. ( The patch should have specified it )

also updating criticity, as it's really security issue !
Comment 4 Chris Cormack 2011-05-10 22:14:14 UTC
Paul do you have a cas server set up we can use to test this against?
Comment 5 Paul Poulain 2011-05-11 08:17:28 UTC
sorry but no :(

(we used to have one locally, we have a private acces to a customer one, but nothing I can give you :(((( )
Comment 6 Chris Cormack 2011-05-12 23:33:27 UTC
I have test this locally, and it doesn't introduce any side effects. I will assume it fixes the problem with CAS and sign off, so it escalates up for QA
Comment 7 Chris Cormack 2011-05-12 23:37:51 UTC
Created attachment 4091 [details] [review]
Signed off patch
Comment 8 Ian Walls 2011-05-13 13:30:37 UTC
To confirm, the problem is that ANY user could log in to a CAS server with their cardnumber and get superlibrarian privileges?  I need to know exactly what I'm testing against in order to determine if it works or not.

Also, any instructions on how to set up a local CAS server would be welcome as well.  (I'm not trying to gum up the works on getting this into Koha, I just want to do the full diligence on a blocker-level bug)
Comment 9 Liz Rea 2011-05-13 14:28:38 UTC
I also confirm the bug and the fix.
Comment 10 Ian Walls 2011-05-13 14:43:57 UTC
The problem:

Unless cardnumber = userid in your system, completely unprivileged patrons can log into the staff client (with superlibrarian privileges) with their cardnumber and password.  This DOES NOT depend on CAS or LDAP.


After applying the patch:

1.  unprivileged users can no longer log into the staff client with cardnumber/password (problem resolved)

2.  privileged users can still log into the staff client, either with username or cardnumber, and their correct privileges are retained

3.  both privileged and unprivileged users can log into the OPAC as normal

Testing regimen does not include LDAP or CAS testing, since that level of authentication is done before falling back to cardnumber authentication.

Marking this patch as Passed QA.
Comment 11 Chris Cormack 2011-05-13 16:31:10 UTC
Pushed to master.

I think this should be cherry picked to 3.2.x as well as 3.4.x and people encouraged strongly to upgrade their versions to 3.2.8  and 3.4.1 .

3.2.8 is due out in a couple of days, perhaps we should do a security release for 3.4.1 too?
Comment 12 Chris Nighswonger 2011-09-07 18:14:19 UTC
This fix has been committed to master, 3.4.x, and 3.2.x. Please test and close.
Comment 13 Ian Walls 2011-10-14 20:41:22 UTC
There is a problem with checkpw's return value being misused since this patch was put in place.  Cardnumber is returned, but treated as userid, particulary in the case of LDAP.
Comment 14 Ian Walls 2011-10-14 20:42:06 UTC
Created attachment 5895 [details] [review]
Propose Followup Patch

Makes checkpw return cardnumber AND userid, in all cases, so the code can use whichever is appropriate in the context.
Comment 15 Chris Cormack 2011-10-15 00:26:28 UTC
Follow up pushed, nasty bug, thanks for the catch Ian
Comment 16 Ian Walls 2011-10-18 14:24:19 UTC Comment hidden (obsolete)
Comment 17 Paul Poulain 2011-10-21 16:55:54 UTC
Created attachment 6022 [details] [review]
[SIGN-OFF]Bug 5995 Follow up: variable scope in koha/sco/sco-main.pl

The patronid value (cardnumber) set by checkpw in the case of SelfCheckoutByLogin
was improperly scoped with 'my' inside a conditional.  The changes followup to 5995
made this more apparent, causing logins to fail.

Also added "parts copyright" statement to the script, since ByWater Solutions did make some
significant contributions to the operations of the page

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Comment 18 Paul Poulain 2011-10-21 16:57:17 UTC
QA comment :
small patch, nothing to say from QA point of view.
I've checked everything works after the patch as before.

Patch submitted by ian, signed-off by me, marking "passed QA" otherwise we will be stuck ;-)
Chris, please check carefully
Comment 19 Chris Cormack 2011-10-21 17:38:26 UTC
Pushed