Opened 22 months ago

Closed 8 months ago

Last modified 6 months ago

#21537 closed enhancement (fixed)

Consider ignoring secure cookies for .onion addresses

Reported by: micah Owned by: gk
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-usability, TorBrowserTeam201804R, GeorgKoppen201804, tbb-backported
Cc: alex@…, pospeselr, mcs, brade, arthuredelstein, sysrqb, igt0, tbb-team Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by micah)

One of the main problem points with adding onion services to existing web services has been interaction with secure cookies. Its hard to setup onion services because you need to enable secure cookies some times (over regular network+TLS) and disable them other times (over .onion network, without TLS). Right now you have to make a trade-off: work well with .onions, or work well with everyone else. This is an unfortunate trade-off.

It is considered a best practice that every web developer is told to do, but its a best practice that doesn't work if you want to run an onion site. Running an onion site should not force you to violate established web application development best practices.

The idea of "secure cookies" is that they prevent you from leaking your cookie information over an insecure connection. There are a lot of ways you can leak your cookie info over an insecure connection:

. dont have hsts setup
. running an application server that sets the cookie before it redirects to https
. or your server is not setup to redirect everything to https

Using "secure cookies" allows the application (regardless of how it is run, or what intermediaries are in between), to make sure that the browser doesn't screw this up. It tells the browser to never submit the cookie over plaintext. Many frameworks have this set by default (such as Rails). Some applications, such as java/tomcat have as part of the stack the cookie setting that happens before that does the redirect to https.

The "secure cookies" spec is just a "suggestion" to the browser, so TBB is free to ignore them, and I think that maybe it should do so for .onion sites.

As an example, if a user goes to https://example.com the first response back sends back a cookie with nothing but a session id. If you then login, you now have a sessionid that is privileged and associated with your account. If you then close that tab, but then realize you needed to do something else, so you open a new tab and go to http://example.com (NB: no https). If the site did not mark the original cookies as 'secure', then the browser will submit in that initial first request the cookie it had previously saved and it will send it over the cleartext channel before the webserver can redirect to the secured site. With the secure cookies flag set, the browser will not send the cookie until the TLS connection is up. This doesn't matter if you are going over onion services because the connection is already wrapped in TLS, and it also doesn't matter if the site has HSTS, because the second visit will go to https by default in that scenario.

So what are the options?

. Ignore secure cookie flags for .onions
. Ignore tls verification for onions

Either one would increase the security properties of onion and non onions, unfortunately the second one would not be appreciated by sites that have actually paid for a valid .onion cert.

Pretty much every Rails application suffers with TBB because of this problem, I'm pretty sure other frameworks also suffer from this. Fixing this would fix a large number of tor problems related to this.

I'm unsure of the broader implications of this, which is why I wanted to open this for discussion.

Child Tickets

Change History (23)

comment:1 Changed 22 months ago by micah

Description: modified (diff)

comment:2 Changed 22 months ago by micah

Version: Tor: unspecified

comment:3 Changed 22 months ago by cypherpunks

This issue is also relevant to tpo's internal services, such as Trac (see #19963).

Last edited 17 months ago by cypherpunks (previous) (diff)

comment:4 Changed 22 months ago by gk

Keywords: tbb-usability added; secure cookies removed

I am fine with testing the "Ignore secure cookie flags for .onions"-idea.

comment:5 Changed 22 months ago by cypherpunks

comment:6 in reply to:  5 Changed 22 months ago by gk

Replying to cypherpunks:

ff52-esr-will-have?
https://bugzilla.mozilla.org/show_bug.cgi?id=976073

Upon a cursory read of that bug it occurs to me it fixes an orthogonal issue but not this bug.

comment:7 Changed 22 months ago by strugee

Cc: alex@… added

comment:8 in reply to:  4 Changed 9 months ago by gk

Keywords: TorBrowserTeam201803 GeorgKoppen201803 added

Replying to gk:

I am fine with testing the "Ignore secure cookie flags for .onions"-idea.

Or make .onions just work with secure cookies by not only checking for "https" but ".onion" as well.

comment:9 Changed 9 months ago by micah

To test this, I've set up a test site.

In a current (broken) TBB browser visit the following page:

http://cookie.revolt.org

You will see 'no cookie value set, refresh the page'. If you refresh the page, while on http, the cookie value will continue to *not* be set. That is because of secure cookies, and the connection not being on https. This is expected.

Now, visit https://cookie.revolt.org and then refresh the page, you will see a cookie value set.

Now click the 'reset cookies' link, and visit the onion link and refresh the page. You will see the behavior is exactly the same as the http connection, no cookie value gets set.

If TBB is fixed, then when you visit the onion link and refresh the page, it will set a cookie and show that it is set, just like in the https case above.

comment:10 in reply to:  9 Changed 9 months ago by gk

Keywords: TorBrowserTeam201804 GeorgKoppen201804 added; TorBrowserTeam201803 GeorgKoppen201803 removed

Replying to micah:

To test this, I've set up a test site.

In a current (broken) TBB browser visit the following page:

http://cookie.revolt.org

You will see 'no cookie value set, refresh the page'. If you refresh the page, while on http, the cookie value will continue to *not* be set. That is because of secure cookies, and the connection not being on https. This is expected.

Now, visit https://cookie.revolt.org and then refresh the page, you will see a cookie value set.

Now click the 'reset cookies' link, and visit the onion link and refresh the page. You will see the behavior is exactly the same as the http connection, no cookie value gets set.

If TBB is fixed, then when you visit the onion link and refresh the page, it will set a cookie and show that it is set, just like in the https case above.

Thanks for this test setup! I spent part of my Easter holidays coming up with a patch and tests for it. It seems I have something that fixes this bug without breaking anything else (so far). I'll clean up my patch a bit and post the patch for review shortly. I think it might make it into the next alpha for further testing.

comment:11 Changed 9 months ago by gk

Keywords: TorBrowserTeam201804R added; TorBrowserTeam201804 removed
Status: newneeds_review

Okay, bug_21537 (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=bug_21537) in my public tor browser repo has two patches for review:

1) The code changes (commit f346f7368523c296a1363247dd78b173a524fad8)
2) Updated cookie tests (commit 106d99883dca2c8906b4cb5cc56931ded33f0a3e)

FWIW: I followed the approach taken in #23439 and #21321 by treating cookies set with the secure flag by .onion domains as secure as well.

Last edited 9 months ago by gk (previous) (diff)

comment:12 Changed 8 months ago by pospeselr

Change looks good, only thing I'd suggest is moving the block at 3340 a couple lines up before the Telemetry::Accumulate call ( since the enum seems to be a question of cookie security, rather than http(s) ).

I also verified the hostURI that's passed in is already normalized, so we don't have to worry about case insensitive string compare.

comment:13 in reply to:  12 ; Changed 8 months ago by gk

Cc: pospeselr mcs brade arthuredelstein sysrqb igt0 added

Replying to pospeselr:

Change looks good, only thing I'd suggest is moving the block at 3340 a couple lines up before the Telemetry::Accumulate call ( since the enum seems to be a question of cookie security, rather than http(s) ).

I also verified the hostURI that's passed in is already normalized, so we don't have to worry about case insensitive string compare.

Thanks. I added the suggested change in bug_21537_v3 (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=bug_21537_v3). Let me know if that still looks good.

comment:14 in reply to:  13 ; Changed 8 months ago by arthuredelstein

Replying to gk:

Replying to pospeselr:

Change looks good, only thing I'd suggest is moving the block at 3340 a couple lines up before the Telemetry::Accumulate call ( since the enum seems to be a question of cookie security, rather than http(s) ).

I also verified the hostURI that's passed in is already normalized, so we don't have to worry about case insensitive string compare.

Thanks. I added the suggested change in bug_21537_v3 (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=bug_21537_v3). Let me know if that still looks good.

The code looks good to me, but I would suggest factoring out the security checks (which are repeated in three places) by creating a static function like:
bool IsSecureHost(nsIURI *aHostURI)
that returns true for both https and .onion URIs.

Last edited 8 months ago by arthuredelstein (previous) (diff)

comment:15 in reply to:  14 Changed 8 months ago by pospeselr

Replying to arthuredelstein:

Replying to gk:

Replying to pospeselr:

Change looks good, only thing I'd suggest is moving the block at 3340 a couple lines up before the Telemetry::Accumulate call ( since the enum seems to be a question of cookie security, rather than http(s) ).

I also verified the hostURI that's passed in is already normalized, so we don't have to worry about case insensitive string compare.

Thanks. I added the suggested change in bug_21537_v3 (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=bug_21537_v3). Let me know if that still looks good.

The code looks good to me, but I would suggest factoring out the security checks (which are repeated in three places) by creating a static function like:
bool IsSecureHost(nsIURI *aHostURI)
that returns true for both https and .onion URIs.

Yeah I'd agree with this.

comment:16 Changed 8 months ago by gk

Keywords: TorBrowserTeam201804 added; TorBrowserTeam201804R removed
Owner: changed from tbb-team to gk
Status: needs_reviewassigned

comment:17 Changed 8 months ago by gk

Keywords: TorBrowserTeam201804R added; TorBrowserTeam201804 removed
Status: assignedneeds_review

Okay, how is bug_21537_v4 (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=bug_21537_v4)? Returning a boolean value made this slightly more complicated as we actually have more than two values to consider due to the telemetry part. But as we don't want to have that anyway, I just ripped it out. We can revisit that when we think we want to upstream that patch but that's a bit in the future as I am not sure whether Mozilla would take it right now anyway. There is no easy way around the HTTPS = secure equation for cookies, I am afraid...

Last edited 8 months ago by gk (previous) (diff)

comment:18 Changed 8 months ago by gk

Cc: tbb-team added

comment:19 Changed 8 months ago by pospeselr

Looks good to me!

comment:20 in reply to:  19 Changed 8 months ago by arthuredelstein

Replying to pospeselr:

Looks good to me!

Me too!

comment:21 Changed 8 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks! Applied to tor-browser-57.3.0esr-8.0-1 (commits c70454fd10efeb9f4cabc69f94a9a7a633c10174 and 82cd8ae9a5de7c9f9fde591c29b0ccfa8b59d42f).

comment:22 Changed 8 months ago by gk

Keywords: tbb-backport added

comment:23 Changed 6 months ago by gk

Keywords: tbb-backported added; tbb-backport removed

Backported to tor-browser-52.8.0esr-7.5-1 (commit 1f33ee1778b0ad0f696977fbcbae67f72d34b99f).

Note: See TracTickets for help on using tickets.