Sessions are not encrypted and leak user_id
#603
Comments
|
UUIDs are inferior to random strings that can be obtained from
On top of that, even if the implementation uses a CSPRNG, UUID4 is limited to 122-bits, wasting entropy to metadata bits, and wastes space with its hex and hyphenated encoding. Related to #528 |
|
Are we trying to have "not at all guessable" or "not countable" because the latter is much more flexible, and if we use alternate IDs for sessions, we can have them be whatever high entropy value we want. |
|
I'd like to solve this problem with a better designed session cookie. Namely, an encrypted session cookie which contains the current user_id and a session_token used for validation. This will allow us to keep the current user_id, but hide it from the user, and provide session validation with a new session_token column value. This approach should be simpler, since we won't have to change how the user_ids are created. And this will be more flexible, since we'll likely want to encrypt other things in the session cookie in future. stepsThese are the steps I'm preparing to follow to get this done.
exampleThe implementation will look something like this in practice: loginsession["encrypted_cookie"] = encrypt_cookie(user_id, session_token, <other-values?>)
user.session_token = session_token
db.session.commit()endpointsuser_id, session_token, <other-values?> = decrypt_cookie(session)
if (user := db.session.get(User, user_id)) is None:
flash("🫥 User not found. Please log in again.")
return redirect(url_for("login"))
if not bytes_are_equal(session_token, user.session_token):
flash("⛔️ Invalid session token. Please log in again.")
return redirect(url_for("login")) |
Just to be clear, you mean closing that one and adding one change at a time?
If we want to support multiple distinct sessions that can be separately invalidated, we want a
This is a tall order for a single PR and each of these should probably be made their own ticket so implementation can be discussed separately. For example, handling cookies like this: session["encrypted_cookie"] = encrypt_cookie(user_id, session_token, <other-values?>)Is not idiomatic. We can override the default session interface so this happens transparently throughout the app.
I'm not sure what this even means or is in reference to. Can you elaborate (or just make it its own ticket and dump knowledge there). |
Git patch files can be downloaded and applied on any HEAD: ``git apply volumes/user_id_session_cookie_change_locations.patch`` This file was produced by saving the relevant diff: ``git diff > volumes/user_id_session_cookie_change_locations.patch``
I'll be using #411 as a reference guide. So eventually it will be closed, once it's no longer needed.
Yeah, that's a solid idea. In this case, would a log out invalidate all sessions by the same user, or just the current session?
I really like the idea of overriding the default session object. The two approaches address different notions of a small PR. Overriding the session object may lead to fewer lines of code and may leave endpoints untouched, but it will affect the behavior of every session object interaction. Adding an encrypted field that's handled through an abstracting function leaves the existing behavior of all other fields untouched, but will need an implementation swap at the endpoints. I'm open to either approach. Though overriding the default session object comes with higher risk, since we'd have to be fully acquainted with how flask handles the object to make sure we aren't failing to implement a required interface, or consider some subtle security implication. As a security auditor, it would be easier to reason about an encrypted field, than a reimplementation of a core object in a third party library. So I could see clearly what needed to be changed, I found the locations and uploaded it as a patch file. There's 41 locations in endpoints which would need a swap of implementation if an abstracting function were used. That means the swap would lead to a PR with maybe 80 lines of code changed.
There's a section in #411's summary titled Input Canonicalization with links. This is a way to encode distinct bytes-type values such that they can be encrypted and later decoded unambiguously. Though, I'm also considering There are two routes which inject the user_id in the URL. This behavior should be changed so that it no longer injects this data. This can be handled in a separate PR as well. |
So maybe the MVP is just:
Full list of things related to this that I think are standard would be:
It doesn't. En/decryption happens outside the endpoints so in an endpoint we use it like this: from flask import session
@app.route("/foo")
def foo():
if val := session.get("wat"):
return val
session["wat"] = 'hello world'
return 'wat: new'This is complete invisible and we don't have to worry about manually encrypting the field or keeping tabs on what fields are or aren't encrypted. The sessions are HTTP-only, so there's no reason to leave any fields unencrypted. Another option is to just set a session ID and pull the session itself from Redis (or PG or whatever, there's many options) so that the cookie is only the key to the ephemeral store. If we use Redis and set the lifetime for the sessions, we can avoid the need for a reaper process. Though I haven't done
There are tons of examples and Flask endorses this. There are many examples of session interfaces from well-maintained Flask extensions. Relevant Flask docs: https://flask.palletsprojects.com/en/3.0.x/api/#flask.sessions.SessionInterface
If we do it simply (just add en/decryption to current object, no Redis), I'm pretty sure it's something like 10 significant lines of code for the entire thing. But I'm less worried about auditability of the interface because many bugs come from people using interfaces wrong. Every dev has to remember to do the en/decryption on the relevant endpoints or they have to remember to assign fields to some imported
I feel like this kinda proves my above point? That's a lot of place where changes have to be explicit instead of abstracted away from devs.
This comes back to a previous point which is do IDs used in the app (URLs, form fields, etc.) need to be "cryptographically secure totally random unguessable" or just "not an int you can easily count." I say the latter because all auth checks should be explicit every time in which case an int is as secure as a UUID (with the exception about leaking counts). This also helps prevent table bloat. Also this is a fairly big/annoying change, so that alone should be limited to a single PR because it would need significant migration tests as well. |
|
Also just as a note, a lot of our auth logic is based around us doing things like this: @app.route("/foo")
@authentication_required
def foo():
user_id = session.get('user_id', None)
user = db.session.get(User, user_id)
if user is None:
# abort/redirect logic
return user.idThis has a double auth check: If we use from flask_login import login_required, current_user
@app.route("/foo")
@login_required
def foo():
return current_user.idThis isn't strictly related to this ticket in the sense that |
Is your feature request related to a problem? Please describe.
Flask sessions are not encrypted, only signed/HMAC'd. This leaks user counts.
Describe the solution you'd like
Ideally both, but at least one of:
The text was updated successfully, but these errors were encountered: