have an option to set the Secure flag for trac_session cookies

[dkg-debian.org@…]

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.

[dkg-debian.org@…]

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.

[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.

[Remy Blank]

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

[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.

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?

[Remy Blank]

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.

[Daniel Kahn Gillmor <dkg@…>]

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.

[Remy Blank]

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.

[Daniel Kahn Gillmor <dkg@…>]

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).

[Remy Blank]

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.

[Remy Blank]

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

[Remy Blank]

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.

[Remy Blank]

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

[Daniel Kahn Gillmor <dkg@…>]

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

[Remy Blank]

Patch applied in [7552].