Edgewall Software

Opened 8 years ago

Closed 4 years ago

#3211 closed defect (wontfix)

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:


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 8 years ago by eblot

  • Milestone 0.9.6 deleted

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 8 years ago by coderanger

This can already be controled with the check_auth_ip option.

comment:3 Changed 8 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 8 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 8 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 7 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 7 years ago by Dalius Dobravolskas

  • Cc dalius@… added

comment:8 Changed 4 years ago by cboos

  • Milestone changed from 2.0 to unscheduled

Milestone 2.0 deleted

comment:9 Changed 4 years 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
as closed The owner will remain jonas.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from jonas to the specified user.

E-mail address and user name can be saved in the Preferences.

Note: See TracTickets for help on using tickets.