Edgewall Software
Modify

Opened 12 years ago

Closed 11 years ago

#5910 closed defect (fixed)

have an option to set the Secure flag for trac_session cookies

Reported by: dkg-debian.org@… Owned by: Remy Blank
Priority: normal Milestone: 0.11.2
Component: general Version:
Severity: normal Keywords: cookie secure security patch
Cc: Branch:
Release Notes:
API Changes:

Description

If you run trac entirely under HTTPS (e.g. at https://example.com/), there's no reason why you'd want the trac_session cookie to be sent in the clear (e.g. if users point their browsers to http://example.com/ by accident). The solution to this is to be able to set the secure flag on the session cookie, which asks the browser to only transmit the cookie when access is made via TLS.

I realize that not everyone who runs trac under HTTPS wants this (they may prefer to have a session persist across both HTTP and HTTPS accesses), but for those of us who run trac permanently behind TLS, it is a shame that end user's web browsers will freely transmit their session identifier in the clear if a wrong address is typed into the address bar.

While sessions being tied to IP addresses is a mitigating factor here, it doesn't solve the most common case of cookie-sniffing, which is by a close peer on the LAN. In today's NAT'ed LANs it is much more likely that neighbors (i.e. office-mates, etc.) will share a public IP address. This makes the session re-usable by those most able to sniff the local network.

A simple setting in trac.ini would be great.

If this is already an option, please let me know how! i've searched the documentation and the source code and couldn't sort it out.

Thanks again for all your work on trac.

Attachments (2)

5910-secure-auth-cookie-r7510.patch (893 bytes ) - added by Remy Blank 11 years ago.
Patch against 0.11-stable adding an option to set the secure flag on the trac_auth cookie
5910-secure-cookies-r7511.patch (2.5 KB ) - added by Remy Blank 11 years ago.
Patch against 0.11-stable adding an option to set the secure flag on all cookies

Download all attachments as: .zip

Change History (15)

comment:1 by dkg-debian.org@…, 12 years ago

Keywords: security added

The following diff seems to set all cookies to use the secure flag:

Index: trac/web/api.py                         
===================================================================
--- trac/web/api.py     (revision 6068)
+++ trac/web/api.py     (working copy)
@@ -505,6 +505,7 @@
                            .replace(';', '%3B') \
                            .replace(',', '%3C')
             self.outcookie[name]['path'] = path
+            self.outcookie[name]['secure'] = True

         cookies = self.outcookie.output(header='')
         for cookie in cookies.splitlines():

While this is exactly what i need for some trac sites i'm responsible for, it's demonstrably wrong for people who expect to use trac without putting it behind HTTPS. Better would be something like a secure_cookies setting in the [trac] section of trac.ini.

I tried to implement this this, but i couldn't convince a BoolOption() call to initialize a property the Request object (maybe because it isn't a Component?). I'd appreciate seeing this option available in 0.11.

Let me know if there's anything else i can to to make this change (or a similar one) more likely to be adopted. I've added the security keyword to this ticket because there's no way to have full cryptographic security against network attackers without this fix.

comment:2 by Christian Boos, 12 years ago

Keywords: consider added
Milestone: 0.11.1

comment:3 by Remy Blank, 11 years ago

Owner: changed from Jonas Borgström to Remy Blank

The patch above sets the secure flag on all cookies (i.e. trac_auth, trac_session and trac_form_token). Is this really needed? AFAIK, only the trac_auth cookie is used for authentication, so hijacking any of the others would not give you the permissions of the hijacked user.

The patch below adds an option [trac] secure_auth_cookie that sets the secure flag on the trac_auth cookie if true (default: false).

I would need somebody more familiar with authentication and session management in Trac to check if this is the correct way to solve the issue.

by Remy Blank, 11 years ago

Patch against 0.11-stable adding an option to set the secure flag on the trac_auth cookie

comment:4 by Daniel Kahn Gillmor <dkg@…>, 11 years ago

If i can read trac_session and trac_form_token, i'm pretty sure i would be able to take over a user's current session. Even if this means that i can't take over their account directly, it still allows me to do some significant damage.

If an installation is designed to be run behind https all the time anyway, why *wouldn't* you want to keep the cookies behind TLS?

in reply to:  4 ; comment:5 by Remy Blank, 11 years ago

Replying to Daniel Kahn Gillmor <dkg@…>:

If i can read trac_session and trac_form_token, i'm pretty sure i would be able to take over a user's current session. Even if this means that i can't take over their account directly, it still allows me to do some significant damage.

I would be interested in knowing how you would do that. AFAICT, the only thing you might be able to do is change the user's preferences (which might already be reason enough, I agree), maybe not even that.

If an installation is designed to be run behind https all the time anyway, why *wouldn't* you want to keep the cookies behind TLS?

I don't claim this first patch is the final solution. Let me explain the thoughts that led to this:

  • We want to protect sensitive information from being sent out accidentally in the clear. Which information really is sensitive?
  • To be able to execute actions with the privileges of the hijacked user, you need to authenticate as that user. On a "normal" install, this means either authenticating as that user (not a concern here) or hijacking her authentication cookie trac_auth. So my answer to the question above was "the trac_auth cookie".
  • Would there be a benefit to marking only trac_auth as secure? I would say so, for sites that allow both HTTP and HTTPS access. You could allow anonymous access to the site through HTTP and restrict authenticated access to HTTPS. By keeping the trac_session cookie non-secure, you allow anonymous users to have sessions, and therefore to keep their preferences.

Maybe we could use a three-state option: "no" → all cookies non-secure, "auth" → trac_auth secure, and "all" → all cookies secure. How does that sound?

Again, I'm no expert in Trac authentication and session management, so I would like to have some feedback about this before proceeding further.

in reply to:  5 ; comment:6 by Daniel Kahn Gillmor <dkg@…>, 11 years ago

Replying to rblank:

I would be interested in knowing how you would do that. AFAICT, the only thing you might be able to do is change the user's preferences (which might already be reason enough, I agree), maybe not even that.

For example, i could close a ticket, or leave a comment as the user in question by forging a request form using the trac_form_token, and present it to the user's browser during subsequent requests as though it was, say, the google search box.

  • We want to protect sensitive information from being sent out accidentally in the clear. Which information really is sensitive?

What is really sensitive is the control over the user account, not the specific password used, or even the ability to authenticate.

  • To be able to execute actions with the privileges of the hijacked user, you need to authenticate as that user. On a "normal" install, this means either authenticating as that user (not a concern here) or hijacking her authentication cookie trac_auth. So my answer to the question above was "the trac_auth cookie".

Given access to the trac_form_token cookie, an attacker controlling the network from which the user connects can perform cross-site request forgery attacks, as described above.

  • Would there be a benefit to marking only trac_auth as secure? I would say so, for sites that allow both HTTP and HTTPS access. You could allow anonymous access to the site through HTTP and restrict authenticated access to HTTPS. By keeping the trac_session cookie non-secure, you allow anonymous users to have sessions, and therefore to keep their preferences.

I might be misunderstanding things: is trac_auth the equivalent of trac_session, only it is used for authenticated sessions? If so, and you were also wrapping the trac_form_token in secure for authenticated sessions, this might be a reasonable approach. I still think you'd do better to just wrap all cookies, though.

Maybe we could use a three-state option: "no" → all cookies non-secure, "auth" → trac_auth secure, and "all" → all cookies secure. How does that sound?

If the variable name is secure_cookies, and you really want to be able to support the anonymous sessions you describe over cleartext, i think the possible values would be never, authenticated_only (covering trac_auth and trac_form_token, though only trac_form_token for authenticated sessions, and not for anonymous ones), and always.

BTW: i don't really fully understand the difference between trac_auth and trac_session; if there is documentation about the intended semantics, and anyone can point me to a link, i'd be happy to read it.

in reply to:  6 comment:7 by Remy Blank, 11 years ago

Replying to Daniel Kahn Gillmor <dkg@…>:

For example, i could close a ticket

That would not be possible. To close a ticket, you need the permission to close the ticket, so you need to be authenticated as a user that has this permission. This is only possible if you have the trac_auth cookie.

I might be misunderstanding things: is trac_auth the equivalent of trac_session, only it is used for authenticated sessions?

No, trac_auth is used exclusively to retrieve the login username on pages that don't require authentication, typically all pages except /login. The username is used for all permission checks.

trac_session links to the session data of the user (both authenticated and anonymous), with things like preferences, last ticket query, default form data for e.g. timeline, … I would have to check, but I think you can't get to an authenticated user's session data if you are not authenticated as that user, so that cookie is actually already "protected" by securing trac_auth.

trac_form_token I am not familiar with, and neither CSRF for that matter, so I'll leave this to be answered by somebody who knows how trac_form_token works.

comment:8 by Daniel Kahn Gillmor <dkg@…>, 11 years ago

trac_form_token and CSRF protection stem are documented and discussed in #4049, fwiw. A compromise of trac_form_token opens the user to impersonation, so that cookie needs to be protected. Depending on what data can be stored in the trac_session, it might also need to be protected; i'm not sure.

FWIW, i don't see a trac_session cookie on Trac systems that i am authenticated to. When i log out of those systems, i lose the trac_auth cookie and gain a trac_session cookie, though. This doesn't seem to meet the description of the two cookies above, but i could very well be misinterpreting something (either in your comment, or in my browser's cookie jar).

in reply to:  8 comment:9 by Remy Blank, 11 years ago

Replying to Daniel Kahn Gillmor <dkg@…>:

A compromise of trac_form_token opens the user to impersonation, so that cookie needs to be protected. Depending on what data can be stored in the trac_session, it might also need to be protected; i'm not sure.

Thanks for the reference. If trac_form_token must be protected, then it doesn't really make sense to leave trac_session unprotected, as you couldn't submit a form through HTTP anyway.

All right, you win ;-) I'll make another patch that adds [trac] secure_cookies as a boolean option, and protects all cookies. Give me a few hours.

by Remy Blank, 11 years ago

Patch against 0.11-stable adding an option to set the secure flag on all cookies

comment:10 by Remy Blank, 11 years ago

Keywords: patch added; consider removed
Milestone: 0.11.30.11.2

The patch above adds the boolean option [trac] secure_cookies, that sets the secure flag on all cookies if true.

Please test the patch on your installation. If successful, and if there are no objections, I'll commit to 0.11-stable for release in 0.11.2.

comment:11 by Remy Blank, 11 years ago

There don't seem to be any objections. Daniel, have you had a chance to try the patch?

comment:12 by Daniel Kahn Gillmor <dkg@…>, 11 years ago

I just tested the patch by applying it against the debian 0.11.1 packages. It works for me. Thanks for doing this, rblank.

comment:13 by Remy Blank, 11 years ago

Resolution: fixed
Status: newclosed

Patch applied in [7552].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Remy Blank 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.