Bug 18275 - opac-memberentry.pl security vulnerabilities
Summary: opac-memberentry.pl security vulnerabilities
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low blocker (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 11077
Blocks: 18442
  Show dependency treegraph
 
Reported: 2017-03-15 16:30 UTC by DEVINIM
Modified: 2017-04-28 05:26 UTC (History)
11 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:


Attachments
Bug 18275: Do not rely on CGI param userid to log a user in if auth is not required (2.79 KB, patch)
2017-03-15 18:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18275: Do not rely on CGI param userid to log a user in if auth is not required (2.84 KB, patch)
2017-03-15 18:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18275: Do not rely on CGI param userid to log a user in if auth is not required (2.84 KB, patch)
2017-03-15 18:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18275: Do not rely on CGI param userid to log a user in if auth is not required (2.91 KB, patch)
2017-03-15 18:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18275: Regression test (1.96 KB, patch)
2017-03-16 12:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18275: Do not rely on CGI param userid to log a user in if auth is not required (3.11 KB, patch)
2017-03-16 12:26 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 DEVINIM 2017-03-15 16:30:07 UTC
In the opac-memberentry.pl authnotrequired area is 1 by default and this yields a security vulnerabilities.

You may get all detailed information of a user without giving username/password.
Comment 1 Jonathan Druart 2017-03-15 17:16:51 UTC
Confirmed!
Working on a patch, we will need a very quick security releases for all stable releases.
Comment 2 Jonathan Druart 2017-03-15 17:27:36 UTC
So basically it comes from:
C4::Auth l.920 $userid = $q_userid;

$q_userid is set previously to the 'userid' CGI param.

We end up here if authonotrequired is set AND CGISESSID does not exist
Comment 3 Jonathan Druart 2017-03-15 17:34:02 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2017-03-15 17:35:03 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2017-03-15 17:36:37 UTC
Sounds like the bug exists since bug 11077.
Comment 6 Jonathan Druart 2017-03-15 18:00:32 UTC
Created attachment 61110 [details] [review]
Bug 18275: Do not rely on CGI param userid to log a user in if auth is not required

From opac/opac-memberentry.pl, authnotrequired is set.
That means a patron can access the page without being logged in. It is
used on this page for the self registration feature.

From C4::Auth::get_template_and_user, we have
  $userid = $q_userid;
$q_userid is previously set to the 'userid' CGI param.

We end up here if authonotrequired is set AND CGISESSID does not exist.

Test plan:
Do not help script kiddies...
Comment 7 Kyle M Hall 2017-03-15 18:13:34 UTC
Created attachment 61113 [details] [review]
Bug 18275: Do not rely on CGI param userid to log a user in if auth is not required

From opac/opac-memberentry.pl, authnotrequired is set.
That means a patron can access the page without being logged in. It is
used on this page for the self registration feature.

From C4::Auth::get_template_and_user, we have
  $userid = $q_userid;
$q_userid is previously set to the 'userid' CGI param.

We end up here if authonotrequired is set AND CGISESSID does not exist.

Test plan:
Do not help script kiddies...

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2017-03-15 18:14:07 UTC
Created attachment 61114 [details] [review]
Bug 18275: Do not rely on CGI param userid to log a user in if auth is not required

From opac/opac-memberentry.pl, authnotrequired is set.
That means a patron can access the page without being logged in. It is
used on this page for the self registration feature.

From C4::Auth::get_template_and_user, we have
  $userid = $q_userid;
$q_userid is previously set to the 'userid' CGI param.

We end up here if authonotrequired is set AND CGISESSID does not exist.

Test plan:
Do not help script kiddies...

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Martin Renvoize 2017-03-15 18:45:04 UTC
Created attachment 61118 [details] [review]
Bug 18275: Do not rely on CGI param userid to log a user in if auth is not required

From opac/opac-memberentry.pl, authnotrequired is set.
That means a patron can access the page without being logged in. It is
used on this page for the self registration feature.

From C4::Auth::get_template_and_user, we have
  $userid = $q_userid;
$q_userid is previously set to the 'userid' CGI param.

We end up here if authonotrequired is set AND CGISESSID does not exist.

Test plan:
Do not help script kiddies...

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2017-03-15 18:48:03 UTC
What a nasty one.. well done getting a patch for it so quick.

Code looks good to me.. tested with shibboleth, ldap and local logins to the extent of my test setup.. not got a CAS server to test against but it doesn't look as though it should interrupt anything there.

One minor QA comment would be the slight lose/butchering of the final comment line by the patch.. but the comment was plain wrong before this patch too so that can be a followup for whenever someone finally gets their head around this in it's entirety.

Passed QA
Comment 11 Tomás Cohen Arazi 2017-03-16 12:26:03 UTC
Created attachment 61167 [details] [review]
Bug 18275: Regression test

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2017-03-16 12:26:11 UTC
Created attachment 61168 [details] [review]
Bug 18275: Do not rely on CGI param userid to log a user in if auth is not required

From opac/opac-memberentry.pl, authnotrequired is set.
That means a patron can access the page without being logged in. It is
used on this page for the self registration feature.

From C4::Auth::get_template_and_user, we have
  $userid = $q_userid;
$q_userid is previously set to the 'userid' CGI param.

We end up here if authonotrequired is set AND CGISESSID does not exist.

Test plan:
- Run:
  $ prove t/db_dependent/Auth.t
=> FAIL: Regression test for checkauth fails
- Apply this patch
- Run:
  $ prove t/db_dependent/Auth.t
=> SUCCESS: Tests pass.
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Kyle M Hall 2017-03-20 13:00:21 UTC
Pushed to master for 17.05, thanks Jonathan, Tomas!
Comment 14 Katrin Fischer 2017-03-20 20:48:41 UTC
This patch has been pushed to 16.11.x and is part of the security release 16.11.05.
Comment 15 Marcel de Rooy 2017-03-22 13:43:05 UTC
Somehow it confused me to see $userid with cas and $q_userid with shib:

( $return, $cardnumber, $retuserid ) = checkpw( $dbh, $q_userid, undef, $query );
[...]
( $return, $cardnumber, $retuserid ) = checkpw( $dbh, $userid, $password, $query, $type );

If I am not mistaken, cas does not look at the userid at all. Would it be better to replace $userid in the last statement by undef? Should not make a difference, but for completeness?
Comment 16 Katrin Fischer 2017-03-22 13:45:01 UTC
Can't talk about the code, but your userid in Koha must match your userid in CAS for it all to work.
Comment 17 Marcel de Rooy 2017-03-22 13:46:26 UTC
(In reply to Katrin Fischer from comment #16)
> Can't talk about the code, but your userid in Koha must match your userid in
> CAS for it all to work.
Well, this is about the code.
Comment 18 Marcel de Rooy 2017-03-22 13:59:18 UTC
(In reply to Marcel de Rooy from comment #17)
> (In reply to Katrin Fischer from comment #16)
> > Can't talk about the code, but your userid in Koha must match your userid in
> > CAS for it all to work.
> Well, this is about the code.

To clarify: the code does not look to the userid, it looks at the ticket parameter here.
Comment 19 Jonathan Druart 2017-03-22 14:01:48 UTC
Yes Marcel, reading the code it makes sense to me to replace it by undef (or at least $q_userid for consistency).
Comment 20 Mason James 2017-04-03 14:35:47 UTC
Pushed to 16.05.x, for 16.05.11 release
Comment 21 Jonathan Druart 2017-04-27 20:57:44 UTC
ping Julian? This needs to be backported ASAP.
Comment 22 Julian Maurice 2017-04-28 05:23:50 UTC
(In reply to Jonathan Druart from comment #21)
> ping Julian? This needs to be backported ASAP.

Thanks, I missed that one
Comment 23 Julian Maurice 2017-04-28 05:26:10 UTC
(In reply to Julian Maurice from comment #22)
> (In reply to Jonathan Druart from comment #21)
> > ping Julian? This needs to be backported ASAP.
> 
> Thanks, I missed that one
In fact I just forgot to add a comment. It is pushed in 3.22.18