|
Description
Devinim
2017-03-15 16:30:07 UTC
Confirmed! Working on a patch, we will need a very quick security releases for all stable releases. 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 You also need PatronSelfRegistration OFF (In reply to Jonathan Druart from comment #3) > You also need PatronSelfRegistration OFF Easier and quicker fix, without any patches would be to... turn it on 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... 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> 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> 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> 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 Created attachment 61167 [details] [review] Bug 18275: Regression test Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> Pushed to master for 17.05, thanks Jonathan, Tomas! This patch has been pushed to 16.11.x and is part of the security release 16.11.05. 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? Can't talk about the code, but your userid in Koha must match your userid in CAS for it all to work. (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. (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. Yes Marcel, reading the code it makes sense to me to replace it by undef (or at least $q_userid for consistency). Pushed to 16.05.x, for 16.05.11 release ping Julian? This needs to be backported ASAP. (In reply to Jonathan Druart from comment #21) > ping Julian? This needs to be backported ASAP. Thanks, I missed that one (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 |