Edgewall Software
Modify

Opened 9 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:

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 (0)

Change History (9)

comment:1 Changed 9 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 9 years ago by coderanger

This can already be controled with the check_auth_ip option.

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

  • Cc dalius@… added

comment:8 Changed 5 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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
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.

Add Comment


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

 
Note: See TracTickets for help on using tickets.