Avoid resetting the session key with each call to put/4 #72
Conversation
This test fails, exposing a bug. As things stand, Redbird accidentally moves to a new redis key each time `Plug.Session.REDIS.put/4` is called. This stems from calling `Redbird.Key.sign_key/2` unconditionally (as of 299906f), which results in signing the already-signed user-supplied key. We end up with a key with arbitrary levels of nested signatures. This is bad in its own right (redbird isn't supposed to move to a new key each `put`), but also leads to the key growing without bound, as each new layer of signature results in a somewhat-longer key. Eventually, this overflows the maximum length of 4096 bytes which plug is willing to allow in a cookie, leading to a Plug.Conn.CookieOverflowError.
The function previously known as Redbird.Key.deletable?/2 is now Redbird.Key.accessible?/2, and is used in put/4, get/3, and delete/2. A key is now signed exactly once, immediately after being generated, and verified each time it is used. This fixes the bug described in the previous commit, where the session key is re-signed with each call to put/4, causing it to grow without bound and eventually overflow.
|
@foxbenjaminfox thanks so much for opening this PR! I'll give it a look as soon as I can. @lancejjohnson I know you worked on this, so I also wanted to reach out in case you could give it a look. |
There was a problem hiding this comment.
@foxbenjaminfox thank you so much for opening this PR. I had a question about whether or not we should be verifying the key when trying to put it. Maybe we do? I'm honestly not sure. So I'd love to hear your thoughts on it.
| put(conn, Key.generate(), data, init_options) | ||
| key = | ||
| Key.generate() | ||
| |> Key.sign_key(conn) |
There was a problem hiding this comment.
Ah, yes. This is a good catch 👍
I think this is where we're supposed to sign the key since it's the first time it's generated.
lib/redbird/plug/session/redis.ex
Outdated
| key | ||
| |> Key.sign_key(conn) | ||
| |> set_key_with_retries(Value.serialize(data), session_expiration(init_options), 1) | ||
| if Key.accessible?(key, conn) do |
There was a problem hiding this comment.
Do you know if we need to make this check here?
Looking at Plug.Session.Store's docs, I see that we're supposed to generate the key when its value is nil (which is what you're doing in the function above). But I think after that we can just take the key that Plug is giving us and setting it.
At the very least, I don't think we should be returning the key in the else clause below since it hasn't passed the Key.accessible? check. In that case, we're not actually setting the key, but by returning it, we're incorrectly telling Plug that we've storing the session.
So, do you know if there's harm in just having the put function have these two function heads?
def put(conn, nil, data, init_options)
key = Key.generate() |> Key.sign_key(conn)
put(conn, key, data, init_options)
end
def put(_conn, key, data, init_options) do
set_key_with_retries(key, Value.serialize(data), session_expiration(init_options), 1)
endThere was a problem hiding this comment.
The key that Plug gives us is user-controlled—it's whatever the user sent in the cookie (or nil, if the user didn't send the cookie.) This of course is why the signature needs to be verified as part of get—but I'm pretty sure the same reasoning applies to put. If we don't verify the signature, it seems to me that it would allow the user to set arbitrary keys in redis, which would be a pretty bad security flaw in its own right.
At the very least, I don't think we should be returning the key in the else clause below since it hasn't passed the Key.accessible? check. In that case, we're not actually setting the key, but by returning it, we're incorrectly telling Plug that we've storing the session.
We have only two options here: we can either return some key, or we can throw an exception. plug puts the value we return directly into the cookie—we don't exactly have a clean way to signal an error.
But we could raise an exception, if you think that's better. Failing silently on a non-valid key is what we do in delete/3—it would make sense to be consistent about it.
(Actually, for full consistency, if we want an invalid key to throw an error on put/4 and delete/3, wouldn't it make sense to throw an error also on get/3, instead of just returning nil as if the key was valid but the value was unset?)
Come to think of it, another option would be to treat an invalid (badly-signed, or unsigned) key exactly like how we treat nil, and generate a new key to use. That's probably the best solution here.
There was a problem hiding this comment.
@foxbenjaminfox thanks so much for all that information.
I'm pretty sure the same reasoning applies to put. If we don't verify the signature, it seems to me that it would allow the user to set arbitrary keys in redis, which would be a pretty bad security flaw in its own right.
That makes sense. So let's keep the verification step in put just like you have it now.
Come to think of it, another option would be to treat an invalid (badly-signed, or unsigned) key exactly like how we treat nil, and generate a new key to use. That's probably the best solution here.
I like this idea. I don't know if that's somewhat unexpected from the end-user's perspective, but on the other hand, the end user should only care that they get a key back where we've stored the session. So I think that idea sounds great.
So let's go ahead and treat an invalid key exactly like we treat nil. Once that change is in place, I think this is good to go.
There was a problem hiding this comment.
So let's go ahead and treat an invalid key exactly like we treat nil. Once that change is in place, I think this is good to go.
6cc04fd does this by having put/4 call itself with nil as the second argument if it can't validate the key.
| @@ -57,6 +57,29 @@ defmodule RedbirdTest do | |||
| assert get_session(conn, :foo) == "bar" | |||
| end | |||
|
|
|||
| test "it reuses the session key between requests" do | |||
There was a problem hiding this comment.
Thank you for adding this test! 🎉 It was helpful for me to be able to run it locally to understand the failure you're talking about. 👍
Plug.Crypto defaults to a validity period of 24 hours. Thus we effectively expire sessions after 24 hours, irrespective of redbird's configured expiration time. By passing our expiration time through as the max_age when verifying the signature, we can avoid expiring sessions early when redbird is configured to keep session for longer than 24 hours (as indeed it is by default.)
The redis key sent in the cookie can't be trusted if it's unsigned, or if it's signed incorrectly. We need to ensure that we don't allow malicious users to set arbitrary redis keys. Therefore, if the signature can't be validate we start over with a new generated key in the same way as when the key isn't specified.
|
799ab47 fixes a related issue that I ran into while testing this issue, but it arguably could go into a PR of its own. If you'd prefer that I split it out into a separate PR I will.
The fix is simple: I made sure that the |
There was a problem hiding this comment.
This looks great. 🎉 Thanks so much for all the work @foxbenjaminfox. And thank you for offering to split the max_age work into a separate one, but I think it's fine for all of it to go in a single PR.
|
Just pushed this in |
After upgrading to the new v0.7.0 of redbird, I found I consistently got
Plug.Conn.CookieOverflowErrors after a few requests.This is strange—the session key is supposed to be stable, yet it was changing and growing in size with each request.
When I examined the overly-large value being set in the cookie, I saw that it was something signed with
Plug.Crypto.sign, whose payload is a value also signed byPlug.Crypto.sign, and so forth, quite a few levels deep.In other words: The redis key gets re-signed by
Redbird.Crypto.signwith every call toput/4—even if that key was already signed, leading to a new (larger) key with each request!The fix is to sign the key exactly once, right after generating it, then verify that the key is signed properly in
put/4in the same way as inget/3ordelete/2.