Check for Change in Remote IP address for Session Security. Disable when remote ip address changes frequently.
Created attachment 2860 [details] [review] Check for Change in Remote IP address for Session Security. Disable when remote ip address changes frequently.
The patch does not apply anymore
Created attachment 4585 [details] [review] Check for Change in Remote IP address for Session Security. Disable when remote ip address changes frequently.
Created attachment 4586 [details] [review] Bug 5511 Updating all other language sysprefs
Firstly, if I have understand the default value for the preference in the patch, the security of all koha-based library will downgrade to "Disable by default" ? (The default is set to "0") I will prefer the option "secure by default". For more security information, see http://capec.mitre.org/data/definitions/60.html [CAPEC-60: Reusing Session IDs (aka Session Replay)]. The restrict-by-IP check is a "multifactor authentication". Secondly, does this solution isn't worst than the problem ? I think the problem is a "by session" problem for some roaming users. And this solution is disable globally security check at "site level". An alternation solution is an option at the login page: "restrict session by this IP ? Y/n" (and "Y" by default, of course).
QA comment(s) Before testing this patch, some QA comments : + remoteipcheck => C4::Context->preference("remoteipcheck"), seems useless to me = there's no need to send this syspref to the templates (and you don't doo anything with it anyway) Also note that this patch won't apply anymore due to the changes in the syspref system that has been pushed recently (see bug 6537) Also note that we have suggested/proposed that updatedatabase are provided in a separate patch, so as you take care of the previous suggestion, can you do this as well ? Could you submit an enhanced patch please, and reset to "need signoff", I set "failed QA"
One of our customers ran into this recently. Given the continued existence of things like web proxy farms that can result in REMOTE_ADDR changing from request to request, a general question -- are there any improvements in the state of the art for anti-session-hijacking measures that would reasonably allow us to remove the IP address check (or implement a syspref like Amit's patch tried)? IMO, a "restrict session by this IP ? Y/n" widget on the login form doesn't seem like a friendly UI choice.
(En réponse au commentaire 7) > One of our customers ran into this recently. Given the continued existence > of things like web proxy farms that can result in REMOTE_ADDR changing from > request to request, a general question -- are there any improvements in the > state of the art for anti-session-hijacking measures that would reasonably > allow us to remove the IP address check (or implement a syspref like Amit's > patch tried)? If I remember well, the patch disable by default IP restriction. It is bad: hijacking will be easy (just need to steal cookie, with XSS for example). > IMO, a "restrict session by this IP ? Y/n" widget on the login form doesn't > seem like a friendly UI choice. but bugzilla propose the same on logon (and advice that is better to run with). It should be ok with a system with a syspref "session restriction by IP" - always (and no choice at logon) - yes (choice at logon and yes by default) - no (choice at logon and no by default) - never (and no choice at logon)
Created attachment 21864 [details] [review] Check for Change in Remote IP address for Session Security. Disable when remote IP address changes frequently To Test: 1) Enable the system preference SessionRestrictionByIP 2) Change your system IP. It will not checkout your system IP or signout.
Created attachment 21865 [details] [review] Added new system preference: SessionRestrictionByIP Added new system preference: SessionRestrictionByIP
Applying: Bug -5511- Added new system preference: SessionRestrictionByIP Using index info to reconstruct a base tree... M installer/data/mysql/sysprefs.sql M installer/data/mysql/updatedatabase.pl Falling back to patching base and 3-way merge... Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/sysprefs.sql Patch failed at 0001 Bug -5511- Added new system preference: SessionRestrictionByIP The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
Created attachment 22533 [details] [review] Bug -5511- Added new system preference: SessionRestrictionByIP
This patch set adds a new syspref, which disables session restrict by IP, by default. Which is dangerous, this should be a conscious decision made by someone knowing that they are reducing security.
As Paul and Galen talk here http://koha.1045719.n5.nabble.com/quot-IP-address-has-changed-Please-log-in-again-quot-td5756339.html this sometimes is really annoying, getting a logout with a "IP address has changed. Please log in again" every 15'. A workaround sometimes include setting up well Firefox, not using proxy for the IP for IPs where it is Koha. This is not an option if Koha is hosted on Amazon Cloud or somewhere else. Is very common error of users playing with Koha Demos. Deeper look is necessary, since this IP changes doesn't look a problem with other accounts login, so probably are other strategies friendlier but secure.
(In reply to Chris Cormack from comment #13) > This patch set adds a new syspref, which disables session restrict by IP, by > default. > > Which is dangerous, this should be a conscious decision made by someone > knowing that they are reducing security. Is still an issue, see "[Koha] IP Address Changed" on mailing list today. Maybe we should think about sysprefs that are only avallable to the database user. Marc
(In reply to Marc Véron from comment #15) > (In reply to Chris Cormack from comment #13) > > This patch set adds a new syspref, which disables session restrict by IP, by > > default. > > > > Which is dangerous, this should be a conscious decision made by someone > > knowing that they are reducing security. > > Is still an issue, see "[Koha] IP Address Changed" on mailing list today. > > Maybe we should think about sysprefs that are only avallable to the database > user. > > Marc The problem is just that the patch sets the ip restriction off by default, instead of on by default, and having to make a decision to potentially reduce security. It would have been a simple change, but the patch author has left it to languish as Failed QA
Hi Chris, I will recreate this patch.
*** Bug 3578 has been marked as a duplicate of this bug. ***
Any further movement on this Amit?
Created attachment 34216 [details] [review] [SIGNED OFF] Bug - 5511: Check for Change in Remote IP address for Session Security. Disable when remote ip address changes frequently. To Test: 1) Enable the system preference SessionRestrictionByIP 2) Change your system IP. It will not checkout your system IP or signout. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 34218 [details] [review] [SIGNED OFF] Bug -5511- Added new system preference: SessionRestrictionByIP Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 34219 [details] [review] Bug - 5511 [Followup]: Switched defults Agree with Chris that the defualt should maintain the higher security and not reduce it. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I gave up on waiting for Amit here. I agree with, and have implemented as a followup, the reversal of defaults that Chris suggests. I've also signed off on the first two patches.
Created attachment 34252 [details] [review] Bug - 5511 [Followup]: Switched defults Agree with Chris that the defualt should maintain the higher security and not reduce it. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Created attachment 34320 [details] [review] Bug - 5511 [Followup]: Missed Instance There was an instance of the pragma missed which meant the the original patch set didn't actually solve the problem in a large number of cases This aptch adds in the relevant statement.
That last followup still needs a signoff.. not sure how I missed it when testing first time around.
Created attachment 34616 [details] [review] Bug - 5511 [Followup]: Missed Instance There was an instance of the pragma missed which meant the the original patch set didn't actually solve the problem in a large number of cases This patch adds in the relevant statement. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Created attachment 34709 [details] [review] [PASSED QA] Bug - 5511: Check for Change in Remote IP address for Session Security. Disable when remote ip address changes frequently. To Test: 1) Enable the system preference SessionRestrictionByIP 2) Change your system IP. It will not checkout your system IP or signout. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 34710 [details] [review] [PASSED QA] Bug -5511- Added new system preference: SessionRestrictionByIP Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 34711 [details] [review] [PASSED QA] Bug - 5511 [Followup]: Switched defults Agree with Chris that the defualt should maintain the higher security and not reduce it. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 34712 [details] [review] [PASSED QA] Bug - 5511 [Followup]: Missed Instance There was an instance of the pragma missed which meant the the original patch set didn't actually solve the problem in a large number of cases This patch adds in the relevant statement. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 34713 [details] [review] Bug 5511: QA follow up - several fixes - Move database update entry to correct spot - Fix version number in database update - Fix capitalization in sys pref description - Fix sequence in sysprefs.sql
Maybe we could phrase the system preference description a bit stronger. I added a follow up with some fixes.
Created attachment 34824 [details] [review] [PASSED QA] Bug - 5511: [Followup]: QA Fixes - Move database update entry to correct spot - Fix version number in database update - Fix capitalization in sys pref description - Fix sequence in sysprefs.sql Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Pushed to Module Maintainers Branch - https://github.com/mrenvoize/Koha/tree/authentication
Patches pushed to master. Thanks Amit!
Pushed to 3.18.x will be in 3.18.4
Possible selection for 3.16.x