Added expiration to sessions #1494

heartsucker · 2016-12-11T14:51:14Z

A quick fix to #880. Also, this should be compatible with with the fix @fowlslegs is proposing in #204 (comment).

It works by manually setting a session expiration time in the cookie's data and then checks it before each request either extending it or logging the user out. Sessions are set to expire after 30 minutes.

psivesely · 2016-12-15T00:42:47Z

Sorry, but I already have implemented similar functionality in my branch, so I'm going to close this one.

psivesely · 2016-12-15T00:43:15Z

Good PR though 😸

psivesely · 2016-12-15T01:04:14Z

If you want to connect with us more on Gitter that would be awesome. We're trying to increase organization via Github comments, labels, & assignments in order to make collaboration with outside contributors easier without the need to rely much on either tools, but it's clearly an imperfect system, and still an organization-in-progress. Just noting this because we've closed a couple of your PRs that addressed "help wanted" issues that more organization and/or communication would've saved you from writing in the first place.

heartsucker · 2017-09-07T16:29:48Z

Oh hai everyone. I'm back with this PR again. Rebased on develop.

Pros: sessions only good for 30 minutes

Cons: Exact login times leaked in the cookies. This only really matters for sources and could be mitigated with @fowlslegs suggestion about using some sort of AEAD to encrypt/authenticate the session.

ghost · 2017-09-07T16:38:54Z

@heartsucker it would be good to have the timeout configurable for test purposes. The test could verify that the session expires after say 0.1 sec ?

trevortimm · 2017-09-07T16:44:27Z

@heartsucker can you flesh out a little more exactly how the timing of log in leaked to the cookies could potentially affect (or not affect) sources?

ghost · 2017-09-07T16:55:20Z

@heartsucker this implementation looks fine. I'm a little surprised this does not already exist in flask since it's a common pattern.

heartsucker · 2017-09-07T19:24:52Z

@trevortimm There's a discussion on #204 that covers a lot of this. At the end of the day, if someone has the cookie, they have the codename which means they win. So having the exact time of a session isn't terribly important. Thus, the con is really a very minor one that I'm more pointing out as a way of making note that we have very minimally expanded the amount of information an attacker could receive rather than stating it as an argument against this PR.

On the source side, this mostly just protects against leaving a computer unattended. This is likely in that a long upload may mean they have to leave the computer unattended for a long time and popping their session could be very useful here.

heartsucker · 2017-09-07T19:26:16Z

Also, this is strange. I don't get test failures locally. Travis did just update their trusty image today, so that could be related?

ghost · 2017-09-07T20:17:57Z

@heartsucker looking at flask/sessions.py it seems possible to set permanent_session_lifetime to 30 minutes to get the same result. Am I missing something ? The permanent is the name of the variable is troubling ;-)

garrettr · 2017-09-07T20:32:22Z

looking at flask/sessions.py it seems possible to set permanent_session_lifetime to 30 minutes to get the same result. Am I missing something?

@dachary Yes, see #880 (comment).

Note that it's been a while since I've audited Firefox/Tor Browser's cookie handling code, and doing an up-to-date review is probably worthwhile. Also note that Tor Browser does things differently from Firefox, usually with an eye towards minimizing privacy risks/data leakage, so they may have recent (since 2015) patches that would help with this problem.

It would also be good for somebody to clearly articulate the threat model in this case (and yes, it's my bad for not having done so already). Here's a rough sketch:

When a user logs in to the source interface, they are given a signed session cookie that permits them to log in to their account.
This cookie does not expire. Cookie expiration is usually implemented with the Expires: key for HTTP cookies), but setting an expiration changes the cookie from a session cookie to a persistent cookie. The last time I audited the Firefox cookie code (in 2015), session cookies are persisted in RAM and cleared when the browser exits, while persistent cookies are persisted on disk and deleted once they expire (originally discussed in Require session timeout for Source Interface and Document Interface #880 and others).
We generally try to minimize the chance of the Source's web browser persisting anything to disk, because we cannot reliably wipe the data in a way that prevents recovery through forensic disk analysis.
- Note that there are a lot of problems with this approach in general. Since we do not control the copy of Tor Browser used by the Source, nor their operating system, we can only make a best effort.
On the other hand, @heartsucker and others are correct that cookies without an expiration are effectively "lifetime passes" for access to a Source's account, so if an adversary were somehow able to obtain one, it would give them unlimited access to the Source's account. This is a good place to threat model, discuss how an adversary might realistically obtain a Source's session cookie, and then discuss the best way to mitigate that risk.

heartsucker · 2017-09-08T10:14:42Z

@dachary Missed your earlier comment. Yes, this could be configured or patched for testing. This was more meant as "reopen for review of functionality" thread and then if we decide this is something we want, I can polish it up.

ghost · 2017-09-08T10:21:14Z

@heartsucker FWIW 💯 on implementing this session timeout :-)

heartsucker · 2017-09-20T14:03:36Z

So the threat model with this feature would prevent capturing a cookie from being an unlimited access token, but the problem is that a source's codename is stored in the session. Once this is implemented, we can leave just a session token in the session use that for the current login behavior.

My proposal would be to merge this (already useful for the journalist) and then work on a scheme to alter the login behavior of the source.

heartsucker · 2017-09-21T17:12:25Z

Proposed during the engineering meeting: look at Flask-Login as it's better supported than it was in the past and therefore is a candidate to offload code to something with more eyes than just SD. However, this depends on how Tor Browser handles cookies. It is only an option if TB treats all cookies as session cookies. Writing anything to disk is not acceptable.

redshiftzero · 2017-09-28T16:58:09Z

Needs a rebase but if everyone agrees with my logic here, then I think this is a worthwhile incremental improvement.

heartsucker · 2017-10-02T18:05:17Z

Pinging @redshiftzero for final review on this one.

redshiftzero · 2017-10-02T18:33:33Z

securedrop/journalist.py

+        flash('You have been logged out due to inactivity', 'error')
+
+    session['expires'] = datetime.utcnow() + \
+        timedelta(minutes=getattr(config, 'SESSION_EXPIRATION_MINUTES', 30))


It's good that this has a default of 30 minutes, but otherwise where is this config option defined? (x-linking with #1966). Or are you just assuming that it isn't defined?

Correct. I'm assuming it isn't defined, and I use getattr everywhere. Should someone really want to override it, it will pick it up. The unit/integration tests assume it's undefined and have checks to del the field in the finally block to not affect other tests.

Excellent. Let's also add it into config.py.example as well so that we have all the possible config options documented in one place for the migration that will take place for #1966.

redshiftzero

Tested, and this works as advertised 😎:

A few comments inline for your perusal

redshiftzero · 2017-10-04T21:47:56Z

securedrop/translations/ar/LC_MESSAGES/messages.po

@@ -0,0 +1,1195 @@
+# Arabic translations for SecureDrop.


I suspect this file unintentionally snuck into a commit :)

redshiftzero · 2017-10-04T21:49:41Z

securedrop/journalist.py

+        flash('You have been logged out due to inactivity', 'error')
+
+    session['expires'] = datetime.utcnow() + \
+        timedelta(minutes=getattr(config, 'SESSION_EXPIRATION_MINUTES', 30))


Excellent. Let's also add it into config.py.example as well so that we have all the possible config options documented in one place for the migration that will take place for #1966.

redshiftzero · 2017-10-04T23:54:19Z

securedrop/tests/test_source.py


-        logger.assert_called_once_with(


Wait where did this logger assertion go?

Lost in the rebase D:

redshiftzero · 2017-10-05T00:21:37Z

securedrop/journalist.py

@@ -65,6 +65,13 @@ def get_source(filesystem_id):
 @app.before_request
 def setup_g():
    """Store commonly used values in Flask's special g object"""
+    if 'expires' in session and datetime.utcnow() >= session['expires']:
+        session.clear()
+        flash('You have been logged out due to inactivity', 'error')


gettext('You have been logged out due to inactivity') 🇩🇪 🇸🇦 🇫🇷 🇳🇴

redshiftzero · 2017-10-05T00:23:39Z

securedrop/source_app/__init__.py

@@ -76,6 +77,15 @@ def setup_g():
        g.html_lang = i18n.locale_to_rfc_5646(g.locale)
        g.locales = i18n.get_locale2name()

+        if 'expires' in session and datetime.utcnow() >= session['expires']:
+            session.clear()
+            flash('You have been logged out due to inactivity', 'error')


gettext here too

(:eyes: on #2391)

heartsucker · 2017-10-05T15:21:14Z

@redshiftzero Things are fixed, and I pushed enough times to kill transient CircleCI errors. :D

redshiftzero

Approving this! This is a good incremental improvement - and note it also fixes #2290, which is a confusing error message due to CSRF tokens expiring (they last 24 hours) that users have been seeing.

I do think we should add a notification to make clear to sources that even if the source session has expired, they still need to restart or exit Tor Browser. I'll make a followup issue for this since the functionality here is solid.

redshiftzero added the PR: Needs review label Dec 12, 2016

psivesely closed this Dec 15, 2016

heartsucker deleted the session-timeouts branch January 25, 2017 14:53

redshiftzero mentioned this pull request Aug 19, 2017

Require session timeout for Source Interface and Document Interface #880

Closed

heartsucker restored the session-timeouts branch September 7, 2017 16:03

heartsucker reopened this Sep 7, 2017

heartsucker force-pushed the session-timeouts branch from 0fbf174 to 97c601f Compare September 7, 2017 16:28

heartsucker requested a review from redshiftzero September 7, 2017 16:32

ghost added the feature label Sep 7, 2017

heartsucker force-pushed the session-timeouts branch 5 times, most recently from bad6431 to 25e1fde Compare October 1, 2017 16:02

heartsucker force-pushed the session-timeouts branch 2 times, most recently from ab239f6 to 103904f Compare October 1, 2017 17:12

redshiftzero reviewed Oct 2, 2017

View reviewed changes

heartsucker force-pushed the session-timeouts branch from 103904f to a53684e Compare October 3, 2017 11:01

heartsucker requested a review from a user October 3, 2017 11:01

redshiftzero self-assigned this Oct 4, 2017

redshiftzero suggested changes Oct 5, 2017

View reviewed changes

redshiftzero removed their assignment Oct 5, 2017

heartsucker force-pushed the session-timeouts branch from a53684e to 344ffb2 Compare October 5, 2017 08:41

added session expiration to journalist interface

27d1983

heartsucker force-pushed the session-timeouts branch 2 times, most recently from 55c7ae9 to a38e303 Compare October 5, 2017 12:03

heartsucker added 4 commits October 5, 2017 15:47

added session expiration to source interface

51f3008

refactor tests to work around issue freedomofpress#1444

47cb1df

added session expiration tests

d3fbac0

added SESSION_EXPIRATION_MINUTES to example config

5090c3d

heartsucker force-pushed the session-timeouts branch from a38e303 to 5090c3d Compare October 5, 2017 13:48

redshiftzero self-assigned this Oct 5, 2017

redshiftzero approved these changes Oct 5, 2017

View reviewed changes

redshiftzero removed their assignment Oct 5, 2017

redshiftzero merged commit d57f141 into freedomofpress:develop Oct 5, 2017

redshiftzero mentioned this pull request Oct 5, 2017

Create a better session expiry page #2290

Closed

heartsucker deleted the session-timeouts branch October 7, 2017 08:15

psivesely mentioned this pull request Apr 18, 2018

Replace per-source PGP keys with PBKDF encrypted/generated ed25519 keys #3281

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added expiration to sessions #1494

Added expiration to sessions #1494

heartsucker commented Dec 11, 2016

psivesely commented Dec 15, 2016

psivesely commented Dec 15, 2016

psivesely commented Dec 15, 2016

heartsucker commented Sep 7, 2017

ghost commented Sep 7, 2017

trevortimm commented Sep 7, 2017

ghost commented Sep 7, 2017

heartsucker commented Sep 7, 2017

heartsucker commented Sep 7, 2017

ghost commented Sep 7, 2017

garrettr commented Sep 7, 2017

heartsucker commented Sep 8, 2017

ghost commented Sep 8, 2017

heartsucker commented Sep 20, 2017

heartsucker commented Sep 21, 2017

redshiftzero commented Sep 28, 2017

heartsucker commented Oct 2, 2017

redshiftzero Oct 2, 2017

heartsucker Oct 2, 2017

redshiftzero Oct 4, 2017

redshiftzero left a comment

redshiftzero Oct 4, 2017

redshiftzero Oct 4, 2017

redshiftzero Oct 4, 2017

heartsucker Oct 5, 2017

redshiftzero Oct 5, 2017

redshiftzero Oct 5, 2017

heartsucker commented Oct 5, 2017

redshiftzero left a comment

Added expiration to sessions #1494

Added expiration to sessions #1494

Conversation

heartsucker commented Dec 11, 2016

psivesely commented Dec 15, 2016

psivesely commented Dec 15, 2016

psivesely commented Dec 15, 2016

heartsucker commented Sep 7, 2017

ghost commented Sep 7, 2017

trevortimm commented Sep 7, 2017

ghost commented Sep 7, 2017

heartsucker commented Sep 7, 2017

heartsucker commented Sep 7, 2017

ghost commented Sep 7, 2017

garrettr commented Sep 7, 2017

heartsucker commented Sep 8, 2017

ghost commented Sep 8, 2017

heartsucker commented Sep 20, 2017

heartsucker commented Sep 21, 2017

redshiftzero commented Sep 28, 2017

heartsucker commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redshiftzero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heartsucker commented Oct 5, 2017

redshiftzero left a comment

Choose a reason for hiding this comment