Ticket #3211 (closed defect: wontfix)
Opened 6 years ago
Last modified 19 months ago
IP check too strict
| Reported by: | bbrazil | Owned by: | jonas |
|---|---|---|---|
| Priority: | low | Milestone: | |
| Component: | general | Version: | devel |
| Severity: | normal | Keywords: | consider |
| Cc: | dalius@… | ||
| Release Notes: | |||
| API Changes: | |||
Description
In trac/web/auth.py the IP check against cookies/sessions requries the exact same IP. This doesn't work for NAT over multiple IP addresses, or using multiple proxies which makes login impossible. The solution is to check only the /24.
Patch (implementation by Dinko Korunic):
--- trac/web/auth.py (revision 3356)
+++ trac/web/auth.py (working copy)
@@ -166,9 +166,11 @@
db = self.env.get_db_cnx()
cursor = db.cursor()
if self.check_ip:
+ ipaddr = '.'.join(req.remote_addr.split('.')[:-1])
+ ipaddr = ipaddr + '%'
cursor.execute("SELECT name FROM auth_cookie "
- "WHERE cookie=%s AND ipnr=%s",
- (cookie.value, req.remote_addr))
+ "WHERE cookie=%s AND ipnr LIKE %s",
+ (cookie.value, ipaddr))
else:
cursor.execute("SELECT name FROM auth_cookie WHERE cookie=%s",
(cookie.value,))
This is related to #1485
Attachments
Change History
comment:1 Changed 6 years ago by eblot
- Milestone 0.9.6 deleted
comment:2 Changed 6 years ago by coderanger
This can already be controled with the check_auth_ip option.
comment:3 Changed 6 years ago by Dinko Korunic <dinko.korunic@…>
No, it cannot be controlled. There is a big difference between completely turning off the IP cookie check (via check_auth_ip) and comparing a part of IP address (and being less prone to cookie-based attacks).
comment:4 Changed 6 years ago by Dinko Korunic <dinko.korunic@…>
Anyway, I absolutely agree that idea to hardcode /24 check is bad - I presume there shouldn't be to complex to add one more config notification setting which will define netmask width and use it here.
comment:5 Changed 5 years ago by cboos
- Keywords consider added
- Milestone set to 2.0
- Priority changed from normal to low
A patch would probably help to revive this issue...
comment:6 Changed 4 years ago by Dalius Dobravolskas
Here is alternate fix using mask. It is a little bit better than proposed because it is configurable to use either strict IP check or /24. By default it is /24 (mask=255.255.255.0).
My fix is for trac OpenID authentication plugin (http://trac-hacks.org/wiki/AuthOpenIdPlugin) but I'm pretty sure it is not hard to adapt my fix to original Trac. Here is diff:
http://hg.sandbox.lt/authopenid-plugin/rev/7f795537f6d2
comment:7 Changed 4 years ago by Dalius Dobravolskas
- Cc dalius@… added
comment:8 Changed 22 months ago by cboos
- Milestone changed from 2.0 to unscheduled
Milestone 2.0 deleted
comment:9 Changed 19 months ago by rblank
- Milestone triaging deleted
- Resolution set to wontfix
- Status changed from new to closed
check_auth_ip has been false by default for some time now, and solves the initial issue. I don't think adding subnet checking is worth the additional code and configuration complexity.



Assuming that the subnet is defined as 24.8 is network-specific.
This kind of hardcoded value cannot be used as-is.