From 40e6217ac1a85cc5ed592873ae49db01d3005da4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gina=20H=C3=A4u=C3=9Fge?= Date: Thu, 1 Sep 2022 12:45:57 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20Make=20session=20handling=20more?= =?UTF-8?q?=20secure?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🔒️ Tie remember_me cookie to password hash That way when the password gets changed, existing cookies automatically get invalidated. * 🔒️ Invalidate user sessions on password change This will require a user to log in again after their password is changed, either by themselves or through an admin. This is a slight change in behaviour compared to before, but makes sure password changes are enforced across all existing sessions. * ♻️ remember_key_for_user => signature_key_for_user * 🔒️ Invalidate sessions after 15min of inactivity The server has been keeping track of login session for a while, but now it also only considers such (active login) sessions fresh that have been active within the past 15min. That way, a session cookie's lifetime is now also restricted server side. An active socket connection suffices to keep the session alive, as do any other requests using the cookie. As the session tracking happens in-memory, all sessions will become invalid on server restart. However, if the remember me cookie is set, after server restart a passive login will succeed. * 🔒️ Limit remember_me validity to 90 days So far that was only done through the cookie duration, and with a year to boot. Now we encode the creation date into the cookie and check against that, if it exceeds the cookie duration the cookie is considered unset. Will prevent reuse of ancient remember me cookies (and force the user to login at least once every 90d). * 💚 Fix unit tests New cookie handling requires a faked flask config object. Related: https://huntr.dev/bounties/d27d232b-2578-4b32-b3b4-74aabdadf629/ --- src/octoprint/access/users.py | 37 +++++-- src/octoprint/server/__init__.py | 21 +++- src/octoprint/server/api/__init__.py | 4 + src/octoprint/server/util/flask.py | 98 +++++++++++++++++-- src/octoprint/server/util/sockjs.py | 16 ++- .../static/js/app/viewmodels/usersettings.js | 6 +- .../dialogs/usersettings/access.jinja2 | 3 + tests/server/util/test_flask.py | 18 +++- 8 files changed, 175 insertions(+), 28 deletions(-) diff --git a/src/octoprint/access/users.py b/src/octoprint/access/users.py index c04313b70d..c0109753eb 100644 --- a/src/octoprint/access/users.py +++ b/src/octoprint/access/users.py @@ -127,11 +127,9 @@ def _cleanup_sessions(self): for session, user in list(self._session_users_by_session.items()): if not isinstance(user, SessionUser): continue - if user.created + (24 * 60 * 60) < time.monotonic(): + if user.touched + (15 * 60) < time.monotonic(): self._logger.info( - "Cleaning up user session {} for user {}".format( - session, user.get_id() - ) + f"Cleaning up user session {session} for user {user.get_id()}" ) self.logout_user(user, stale=True) @@ -176,6 +174,9 @@ def check_password(self, username, password): # old hash doesn't match either, wrong password return False + def signature_key_for_user(self, username, salt=None): + return self.create_password_hash(username, salt=salt) + def add_user(self, username, password, active, permissions, groups, overwrite=False): pass @@ -227,21 +228,28 @@ def remove_user(self, username): del self._sessionids_by_userid[username] def validate_user_session(self, userid, session): + self._cleanup_sessions() + if session in self._session_users_by_session: user = self._session_users_by_session[session] return userid == user.get_id() return False - def find_user(self, userid=None, session=None): + def find_user(self, userid=None, session=None, fresh=False): + self._cleanup_sessions() + if session is not None and session in self._session_users_by_session: user = self._session_users_by_session[session] if userid is None or userid == user.get_id(): + user.touch() return user return None def find_sessions_for(self, matcher): + self._cleanup_sessions() + result = [] for user in self.get_all_users(): if matcher(user): @@ -249,7 +257,9 @@ def find_sessions_for(self, matcher): session_ids = self._sessionids_by_userid[user.get_id()] for session_id in session_ids: try: - result.append(self._session_users_by_session[session_id]) + session_user = self._session_users_by_session[session_id] + session_user.touch() + result.append(session_user) except KeyError: # unknown session after all continue @@ -780,6 +790,14 @@ def change_user_password(self, username, password): self._dirty = True self._save() + self._trigger_on_user_modified(user) + + def signature_key_for_user(self, username, salt=None): + if username not in self._users: + raise UnknownUser(username) + user = self._users[username] + return UserManager.create_password_hash(username + user._passwordHash, salt=salt) + def change_user_setting(self, username, key, value): if username not in self._users: raise UnknownUser(username) @@ -845,10 +863,10 @@ def remove_user(self, username): self._dirty = True self._save() - def find_user(self, userid=None, apikey=None, session=None): + def find_user(self, userid=None, apikey=None, session=None, fresh=False): user = UserManager.find_user(self, userid=userid, session=session) - if user is not None: + if user is not None or (session and fresh): return user if userid is not None: @@ -1383,8 +1401,7 @@ def __init__(self, user): wrapt.ObjectProxy.__init__(self, user) self._self_session = "".join("%02X" % z for z in bytes(uuid.uuid4().bytes)) - self._self_created = time.monotonic() - self._self_touched = time.monotonic() + self._self_created = self._self_touched = time.monotonic() @property def session(self): diff --git a/src/octoprint/server/__init__.py b/src/octoprint/server/__init__.py index 926b0c572b..133b0937e9 100644 --- a/src/octoprint/server/__init__.py +++ b/src/octoprint/server/__init__.py @@ -130,7 +130,7 @@ loginFromApiKeyRequestHandler, requireLoginRequestHandler, ) -from octoprint.server.util.flask import PreemptiveCache +from octoprint.server.util.flask import PreemptiveCache, validate_session_signature from octoprint.settings import settings VERSION = __version__ @@ -187,12 +187,25 @@ def load_user(id): else: sessionid = None + if session and "usersession.signature" in session: + sessionsig = session["usersession.signature"] + else: + sessionsig = "" + if sessionid: - user = userManager.find_user(userid=id, session=sessionid) + # session["_fresh"] is False if the session comes from a remember me cookie, + # True if it came from a use of the login dialog + user = userManager.find_user( + userid=id, session=sessionid, fresh=session.get("_fresh", False) + ) else: user = userManager.find_user(userid=id) - if user and user.is_active: + if ( + user + and user.is_active + and (not sessionid or validate_session_signature(sessionsig, id, sessionid)) + ): return user return None @@ -1366,7 +1379,9 @@ def _setup_app(self, app): app.config["TEMPLATES_AUTO_RELOAD"] = True app.config["JSONIFY_PRETTYPRINT_REGULAR"] = False + app.config["REMEMBER_COOKIE_DURATION"] = 90 * 24 * 60 * 60 # 90 days app.config["REMEMBER_COOKIE_HTTPONLY"] = True + # REMEMBER_COOKIE_SECURE will be taken care of by our custom cookie handling # we must not set this before TEMPLATES_AUTO_RELOAD is set to True or that won't take app.debug = self._debug diff --git a/src/octoprint/server/api/__init__.py b/src/octoprint/server/api/__init__.py index f63096f348..a3bc893394 100644 --- a/src/octoprint/server/api/__init__.py +++ b/src/octoprint/server/api/__init__.py @@ -38,6 +38,7 @@ limit, no_firstrun_access, passive_login, + session_signature, ) from octoprint.settings import settings as s from octoprint.settings import valid_boolean_trues @@ -312,6 +313,9 @@ def login(): user = octoprint.server.userManager.login_user(user) session["usersession.id"] = user.session + session["usersession.signature"] = session_signature( + username, user.session + ) g.user = user login_user(user, remember=remember) diff --git a/src/octoprint/server/util/flask.py b/src/octoprint/server/util/flask.py index 52356336fa..244bd8e207 100644 --- a/src/octoprint/server/util/flask.py +++ b/src/octoprint/server/util/flask.py @@ -5,11 +5,13 @@ __copyright__ = "Copyright (C) 2014 The OctoPrint Project - Released under terms of the AGPLv3 License" import functools +import hashlib +import hmac import logging import os import threading import time -from datetime import datetime +from datetime import datetime, timedelta from typing import Union import flask @@ -23,6 +25,9 @@ import webassets.updater import webassets.utils from cachelib import BaseCache +from flask import current_app +from flask_login import COOKIE_NAME as REMEMBER_COOKIE_NAME +from flask_login.utils import decode_cookie, encode_cookie from werkzeug.local import LocalProxy from werkzeug.utils import cached_property @@ -422,6 +427,49 @@ def host_to_server_and_port(host, scheme): # ~~ request and response versions +def encode_remember_me_cookie(value): + from octoprint.server import userManager + + name = value.split("|")[0] + try: + remember_key = userManager.signature_key_for_user( + name, salt=current_app.config["SECRET_KEY"] + ) + timestamp = datetime.utcnow().timestamp() + return encode_cookie(f"{name}|{timestamp}", key=remember_key) + except Exception: + pass + + return "" + + +def decode_remember_me_cookie(value): + from octoprint.server import userManager + + parts = value.split("|") + if len(parts) == 3: + name, created, _ = parts + + try: + # valid signature? + signature_key = userManager.signature_key_for_user( + name, salt=current_app.config["SECRET_KEY"] + ) + cookie = decode_cookie(value, key=signature_key) + if cookie: + # still valid? + if ( + datetime.fromtimestamp(float(created)) + + timedelta(seconds=current_app.config["REMEMBER_COOKIE_DURATION"]) + > datetime.utcnow() + ): + return encode_cookie(name) + except Exception: + pass + + raise ValueError("Invalid remember me cookie") + + class OctoPrintFlaskRequest(flask.Request): environment_wrapper = staticmethod(lambda x: x) @@ -437,10 +485,23 @@ def cookies(self): result = {} desuffixed = {} for key, value in cookies.items(): - if key.endswith(self.cookie_suffix): - desuffixed[key[: -len(self.cookie_suffix)]] = value - else: - result[key] = value + + def process_value(k, v): + if k == current_app.config.get( + "REMEMBER_COOKIE_NAME", REMEMBER_COOKIE_NAME + ): + return decode_remember_me_cookie(v) + return v + + try: + if key.endswith(self.cookie_suffix): + key = key[: -len(self.cookie_suffix)] + desuffixed[key] = process_value(key, value) + else: + result[key] = process_value(key, value) + except ValueError: + # ignore broken cookies + pass result.update(desuffixed) return result @@ -471,7 +532,7 @@ def cookie_suffix(self): class OctoPrintFlaskResponse(flask.Response): - def set_cookie(self, key, *args, **kwargs): + def set_cookie(self, key, value="", *args, **kwargs): # restrict cookie path to script root kwargs["path"] = flask.request.script_root + kwargs.get("path", "/") @@ -490,9 +551,13 @@ def set_cookie(self, key, *args, **kwargs): # set secure if necessary kwargs["secure"] = settings().getBoolean(["server", "cookies", "secure"]) + # tie account properties to remember me cookie (e.g. current password hash) + if key == current_app.config.get("REMEMBER_COOKIE_NAME", REMEMBER_COOKIE_NAME): + value = encode_remember_me_cookie(value) + # add request specific cookie suffix to name flask.Response.set_cookie( - self, key + flask.request.cookie_suffix, *args, **kwargs + self, key + flask.request.cookie_suffix, value=value, *args, **kwargs ) def delete_cookie(self, key, path="/", domain=None): @@ -608,6 +673,9 @@ def login(u): ) if hasattr(u, "session"): flask.session["usersession.id"] = u.session + flask.session["usersession.signature"] = session_signature( + u.get_id(), u.session + ) flask.g.user = u eventManager().fire(Events.USER_LOGGED_IN, payload={"username": u.get_id()}) @@ -1832,3 +1900,19 @@ def default(self, obj): return JsonEncoding.encode(obj) except TypeError: return flask.json.JSONEncoder.default(self, obj) + + +##~~ Session signing + + +def session_signature(user, session): + from octoprint.server import userManager + + key = userManager.signature_key_for_user(user, salt=current_app.config["SECRET_KEY"]) + return hmac.new( + key.encode("utf-8"), session.encode("utf-8"), hashlib.sha512 + ).hexdigest() + + +def validate_session_signature(sig, user, session): + return hmac.compare_digest(sig, session_signature(user, session)) diff --git a/src/octoprint/server/util/sockjs.py b/src/octoprint/server/util/sockjs.py index 88bcacf94a..83134540ca 100644 --- a/src/octoprint/server/util/sockjs.py +++ b/src/octoprint/server/util/sockjs.py @@ -21,9 +21,10 @@ import octoprint.vendor.sockjs.tornado.util from octoprint.access.groups import GroupChangeListener from octoprint.access.permissions import Permissions -from octoprint.access.users import LoginStatusListener +from octoprint.access.users import LoginStatusListener, SessionUser from octoprint.events import Events from octoprint.settings import settings +from octoprint.util import RepeatedTimer from octoprint.util.json import dumps as json_dumps from octoprint.util.version import get_python_version_string @@ -173,6 +174,10 @@ def __init__( self._subscriptions_active = False self._subscriptions = {"state": False, "plugins": [], "events": []} + self._keep_alive = RepeatedTimer( + 60, self._keep_alive_callback, condition=lambda: self._authed + ) + @staticmethod def _get_remote_address(info): forwarded_for = info.headers.get("X-Forwarded-For") @@ -180,6 +185,13 @@ def _get_remote_address(info): return forwarded_for.split(",")[0] return info.ip + def _keep_alive_callback(self): + if not self._authed: + return + if not isinstance(self._user, SessionUser): + return + self._user.touch() + def __str__(self): if self._remoteAddress: return f"{self!r} connected to {self._remoteAddress}" @@ -716,6 +728,8 @@ def _on_login(self, user): ) self._authed = True + self._keep_alive.start() + for name, hook in self._authed_hooks.items(): try: hook(self, self._user) diff --git a/src/octoprint/static/js/app/viewmodels/usersettings.js b/src/octoprint/static/js/app/viewmodels/usersettings.js index 4588241b6d..1ac1775b90 100644 --- a/src/octoprint/static/js/app/viewmodels/usersettings.js +++ b/src/octoprint/static/js/app/viewmodels/usersettings.js @@ -88,7 +88,7 @@ $(function () { self.userSettingsDialog.trigger("beforeSave"); - function process() { + function saveSettings() { var settings = { interface: { language: self.interface_language() @@ -110,7 +110,7 @@ $(function () { self.access_currentPassword() ) .done(function () { - process(); + saveSettings(); }) .fail(function (xhr) { if (xhr.status === 403) { @@ -118,7 +118,7 @@ $(function () { } }); } else { - process(); + saveSettings(); } }; diff --git a/src/octoprint/templates/dialogs/usersettings/access.jinja2 b/src/octoprint/templates/dialogs/usersettings/access.jinja2 index 9cd750b6ff..6bc6d9a445 100644 --- a/src/octoprint/templates/dialogs/usersettings/access.jinja2 +++ b/src/octoprint/templates/dialogs/usersettings/access.jinja2 @@ -24,6 +24,9 @@ {{ _('Passwords do not match') }} +

{% trans %} + Please note that you will be logged out immediately after changing your password and asked to login again. + {% endtrans %}

{{ _('API Key') }} diff --git a/tests/server/util/test_flask.py b/tests/server/util/test_flask.py index 114220c26d..b7c8f6a44d 100644 --- a/tests/server/util/test_flask.py +++ b/tests/server/util/test_flask.py @@ -10,6 +10,7 @@ import unittest from unittest import mock +import flask from ddt import data, ddt, unpack from octoprint.server.util.flask import ( @@ -424,6 +425,9 @@ class OctoPrintFlaskRequestTest(unittest.TestCase): def setUp(self): self.orig_environment_wrapper = OctoPrintFlaskRequest.environment_wrapper + self.app = flask.Flask("testapp") + self.app.config["SECRET_KEY"] = "secret" + def tearDown(self): OctoPrintFlaskRequest.environment_wrapper = staticmethod( self.orig_environment_wrapper @@ -470,7 +474,8 @@ def test_cookies(self): request = OctoPrintFlaskRequest(environ) - cookies = request.cookies + with self.app.app_context(): + cookies = request.cookies self.assertDictEqual( { "postfixed": "postfixed_value", @@ -495,6 +500,9 @@ def setUp(self): self.settings = mock.MagicMock() self.settings_getter.return_value = self.settings + self.app = flask.Flask("testapp") + self.app.config["SECRET_KEY"] = "secret" + def tearDown(self): self.settings_patcher.stop() @@ -545,13 +553,14 @@ def test_cookie_set_and_delete( # test set_cookie with mock.patch("flask.Response.set_cookie") as set_cookie_mock: - response.set_cookie("some_key", "some_value", **kwargs) + with self.app.app_context(): + response.set_cookie("some_key", "some_value", **kwargs) # set_cookie should have key and path values adjusted set_cookie_mock.assert_called_once_with( response, "some_key" + expected_suffix, - "some_value", + value="some_value", path=expected_path_set, secure=secure, samesite=expected_samesite, @@ -560,7 +569,8 @@ def test_cookie_set_and_delete( # test delete_cookie with mock.patch("flask.Response.set_cookie") as set_cookie_mock: with mock.patch("flask.Response.delete_cookie") as delete_cookie_mock: - response.delete_cookie("some_key", **kwargs) + with self.app.app_context(): + response.delete_cookie("some_key", **kwargs) # delete_cookie internally calls set_cookie - so our delete_cookie call still uses the non modified # key and path values, set_cookie will translate those (as tested above)