Edgewall Software

Opened 12 years ago

Closed 8 years ago

#3211 closed defect (wontfix)

IP check too strict

Reported by: bbrazil Owned by: Jonas Borgström
Priority: low Milestone:
Component: general Version: devel
Severity: normal Keywords: consider
Cc: dalius@…
Release Notes:
API Changes:


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))
            cursor.execute("SELECT name FROM auth_cookie WHERE cookie=%s",

This is related to #1485

Attachments (0)

Change History (9)

comment:1 Changed 12 years ago by Emmanuel Blot

Milestone: 0.9.6

Assuming that the subnet is defined as 24.8 is network-specific.

This kind of hardcoded value cannot be used as-is.

comment:2 Changed 12 years ago by coderanger

This can already be controled with the check_auth_ip option.

comment:3 Changed 12 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 12 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 11 years ago by Christian Boos

Keywords: consider added
Milestone: 2.0
Priority: normallow

A patch would probably help to revive this issue…

comment:6 Changed 11 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=

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 11 years ago by Dalius Dobravolskas

Cc: dalius@… added

comment:8 Changed 8 years ago by Christian Boos

Milestone: 2.0unscheduled

Milestone 2.0 deleted

comment:9 Changed 8 years ago by Remy Blank

Milestone: triaging
Resolution: wontfix
Status: newclosed

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.

Modify Ticket

Change Properties
Set your email in Preferences
as closed The owner will remain Jonas Borgström.
The resolution will be deleted.
to The owner will be changed from Jonas Borgström to the specified user.

Add Comment

E-mail address and name can be saved in the Preferences .
Note: See TracTickets for help on using tickets.