Bug 5511 - Check for Change in Remote IP address for Session Security. Disable when remote ip address changes frequently.
Summary: Check for Change in Remote IP address for Session Security. Disable when remo...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Amit Gupta
QA Contact: Bugs List
URL:
Keywords:
: 3578 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-12-16 11:15 UTC by Amit Gupta
Modified: 2015-12-03 22:03 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Check for Change in Remote IP address for Session Security. Disable when remote ip address changes frequently. (5.35 KB, patch)
2010-12-16 11:15 UTC, Amit Gupta
Details | Diff | Splinter Review
Check for Change in Remote IP address for Session Security. Disable when remote ip address changes frequently. (5.47 KB, patch)
2011-07-07 23:31 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5511 Updating all other language sysprefs (9.85 KB, patch)
2011-07-07 23:31 UTC, Chris Cormack
Details | Diff | Splinter Review
Check for Change in Remote IP address for Session Security. Disable when remote IP address changes frequently (3.08 KB, patch)
2013-10-08 04:13 UTC, Amit Gupta
Details | Diff | Splinter Review
Added new system preference: SessionRestrictionByIP (2.22 KB, patch)
2013-10-08 04:14 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug -5511- Added new system preference: SessionRestrictionByIP (2.18 KB, patch)
2013-10-30 06:58 UTC, Chris Cormack
Details | Diff | Splinter Review
[SIGNED OFF] Bug - 5511: Check for Change in Remote IP address for Session Security. Disable when remote ip address changes frequently. (3.13 KB, patch)
2014-12-09 13:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
[SIGNED OFF] Bug -5511- Added new system preference: SessionRestrictionByIP (2.31 KB, patch)
2014-12-09 13:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug - 5511 [Followup]: Switched defults (3.26 KB, patch)
2014-12-09 13:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug - 5511 [Followup]: Switched defults (3.30 KB, patch)
2014-12-09 19:45 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug - 5511 [Followup]: Missed Instance (1017 bytes, patch)
2014-12-11 13:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug - 5511 [Followup]: Missed Instance (1.04 KB, patch)
2014-12-21 19:07 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug - 5511: Check for Change in Remote IP address for Session Security. Disable when remote ip address changes frequently. (3.19 KB, patch)
2014-12-27 13:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug -5511- Added new system preference: SessionRestrictionByIP (2.35 KB, patch)
2014-12-27 13:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug - 5511 [Followup]: Switched defults (3.37 KB, patch)
2014-12-27 13:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug - 5511 [Followup]: Missed Instance (1.11 KB, patch)
2014-12-27 13:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 5511: QA follow up - several fixes (4.83 KB, patch)
2014-12-27 13:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug - 5511: [Followup]: QA Fixes (4.92 KB, patch)
2014-12-30 09:18 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Amit Gupta 2010-12-16 11:15:01 UTC
Check for Change in Remote IP address for Session Security. Disable when remote ip address changes frequently.
Comment 1 Amit Gupta 2010-12-16 11:15:58 UTC Comment hidden (obsolete)
Comment 2 Paul Poulain 2011-03-01 21:16:22 UTC
The patch does not apply anymore
Comment 3 Chris Cormack 2011-07-07 23:31:38 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2011-07-07 23:31:49 UTC Comment hidden (obsolete)
Comment 5 Sébastien Marie 2011-07-08 06:16:38 UTC
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).
Comment 6 Paul Poulain 2011-09-12 13:47:48 UTC
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"
Comment 7 Galen Charlton 2012-07-10 18:02:36 UTC
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.
Comment 8 Sébastien Marie 2012-07-11 05:16:12 UTC
(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)
Comment 9 Amit Gupta 2013-10-08 04:13:48 UTC Comment hidden (obsolete)
Comment 10 Amit Gupta 2013-10-08 04:14:30 UTC Comment hidden (obsolete)
Comment 11 I'm just a bot 2013-10-30 06:54:50 UTC
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".
Comment 12 Chris Cormack 2013-10-30 06:58:42 UTC Comment hidden (obsolete)
Comment 13 Chris Cormack 2013-12-27 20:13:18 UTC
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.
Comment 14 Pablo AB 2014-09-24 13:10:22 UTC
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.
Comment 15 Marc Véron 2014-11-24 07:31:45 UTC
(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
Comment 16 Chris Cormack 2014-11-24 07:54:03 UTC
(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
Comment 17 Amit Gupta 2014-11-24 08:25:16 UTC
Hi Chris,

I will recreate this patch.
Comment 18 Martin Renvoize 2014-12-03 11:41:11 UTC
*** Bug 3578 has been marked as a duplicate of this bug. ***
Comment 19 Martin Renvoize 2014-12-03 11:53:00 UTC
Any further movement on this Amit?
Comment 20 Martin Renvoize 2014-12-09 13:52:35 UTC Comment hidden (obsolete)
Comment 21 Martin Renvoize 2014-12-09 13:52:45 UTC Comment hidden (obsolete)
Comment 22 Martin Renvoize 2014-12-09 13:52:51 UTC Comment hidden (obsolete)
Comment 23 Martin Renvoize 2014-12-09 13:54:25 UTC
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.
Comment 24 Chris Cormack 2014-12-09 19:45:59 UTC Comment hidden (obsolete)
Comment 25 Martin Renvoize 2014-12-11 13:40:38 UTC Comment hidden (obsolete)
Comment 26 Martin Renvoize 2014-12-12 10:07:00 UTC
That last followup still needs a signoff.. not sure how I missed it when testing first time around.
Comment 27 Chris Cormack 2014-12-21 19:07:19 UTC Comment hidden (obsolete)
Comment 28 Katrin Fischer 2014-12-27 13:15:15 UTC
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>
Comment 29 Katrin Fischer 2014-12-27 13:15:19 UTC
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>
Comment 30 Katrin Fischer 2014-12-27 13:15:24 UTC
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>
Comment 31 Katrin Fischer 2014-12-27 13:15:28 UTC
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>
Comment 32 Katrin Fischer 2014-12-27 13:15:32 UTC Comment hidden (obsolete)
Comment 33 Katrin Fischer 2014-12-27 13:16:52 UTC
Maybe we could phrase the system preference description a bit stronger. 
I added a follow up with some fixes.
Comment 34 Martin Renvoize 2014-12-30 09:18:48 UTC
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>
Comment 35 Martin Renvoize 2014-12-30 09:25:27 UTC
Pushed to Module Maintainers Branch - https://github.com/mrenvoize/Koha/tree/authentication
Comment 36 Tomás Cohen Arazi 2015-02-09 20:18:54 UTC
Patches pushed to master.

Thanks Amit!
Comment 37 Chris Cormack 2015-02-14 07:11:45 UTC
Pushed to 3.18.x will be in 3.18.4
Comment 38 Mason James 2015-04-22 12:41:09 UTC
Possible selection for 3.16.x