Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Secure key #67

Merged
merged 3 commits into from
Mar 22, 2021
Merged

Secure key #67

merged 3 commits into from
Mar 22, 2021

Conversation

lancejjohnson
Copy link
Contributor

@lancejjohnson lancejjohnson commented Feb 5, 2021

The current implementation contains two potential security
vulnerabilities. The Redis key assigned by Redbird could be manipulated
to potentially access other data in Redis and assign it as part of the
user's session information. Values stored in Redis are also being
deserialized using an Erlang function that can execute functions stored
as values.

This change closes those potential security vulnerabilities by
cryptographically signing the Redis key used by Redbird and by using a
deserialization function that safely handles Erlang terms including
functions. When a user places a key-value in Redis via Redbird, Redbird
generates a key, signs it, and optionally prepends the namespace. When
accessing the key from Redis, Redbird verifies the key, rejects keys
that have been tampered with, and safely deserializes the stored Erlang
terms. Likewise, Redbird only permits deletion of keys that are valid.

Copy link
Collaborator

@germsvel germsvel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lance, so much good work here! 🎉

Left a few comments. Let me know if anything is unclear or if I can help in any other way.

test/redbird/crypto_test.exs Show resolved Hide resolved
test/redbird_test.exs Outdated Show resolved Hide resolved
test/redbird_test.exs Outdated Show resolved Hide resolved
test/support/conn_case.ex Outdated Show resolved Hide resolved
lib/redbird/key.ex Outdated Show resolved Hide resolved
lib/redbird/plug/session/redis.ex Outdated Show resolved Hide resolved
lib/redbird/plug/session/redis.ex Outdated Show resolved Hide resolved
lib/redbird/key.ex Show resolved Hide resolved
test/redbird/crypto_test.exs Show resolved Hide resolved
test/redbird/crypto_test.exs Show resolved Hide resolved
Copy link

@mike-burns mike-burns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this solution makes sense, and I like the refactoring. I have one question about what might be a fallback.

lib/redbird/key.ex Outdated Show resolved Hide resolved
lib/redbird/key.ex Show resolved Hide resolved
The current implementation contains two potential security
vulnerabilities. The Redis key assigned by Redbird could be manipulated
to potentially access other data in Redis and assign it as part of the
user's session information. Values stored in Redis are also being
deserialized using an Erlang function that can execute functions stored
as values.

This change closes those potential security vulnerabilities by
cryptographically signing the Redis key used by Redbird and by using a
deserialization function that safely handles Erlang terms including
functions. When a user places a key-value in Redis via Redbird, Redbird
generates a key, signs it, and optionally prepends the namespace. When
accessing the key from Redis, Redbird verifies the key, rejects keys
that have been tampered with, and safely deserializes the stored Erlang
terms. Likewise, Redbird only permits deletion of keys that are valid.
@lancejjohnson lancejjohnson marked this pull request as ready for review March 15, 2021 20:32
Copy link
Collaborator

@germsvel germsvel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple of last minor comments. You don't have to make those changes if you don't want. This looks really good!

Thanks for doing all this work 🎉

lib/redbird/plug/session/redis.ex Outdated Show resolved Hide resolved
mix.exs Outdated Show resolved Hide resolved
Introduce a Value module that handles the serialization and
deserialization of the value store in Redis by Redbird.
@lancejjohnson
Copy link
Contributor Author

@germsvel I no longer have the ability to merge this PR and/or run the release script. Would you be willing to do that when you get a chance? Sorry for leaving this loose end.

@germsvel germsvel merged commit 299906f into master Mar 22, 2021
@germsvel germsvel deleted the secure-key branch March 22, 2021 12:48
@danadaldos
Copy link
Contributor

@germsvel @lancejjohnson Hey friends! We're also running into the issue described here: #72 and will be anxiously awaiting a resolution. In the meantime, would it be possible to update the description on this PR to the commit message here?:

The current implementation contains two potential security
vulnerabilities. The Redis key assigned by Redbird could be manipulated
to potentially access other data in Redis and assign it as part of the
user's session information. Values stored in Redis are also being
deserialized using an Erlang function that can execute functions stored
as values.

This change closes those potential security vulnerabilities by
cryptographically signing the Redis key used by Redbird and by using a
deserialization function that safely handles Erlang terms including
functions. When a user places a key-value in Redis via Redbird, Redbird
generates a key, signs it, and optionally prepends the namespace. When
accessing the key from Redis, Redbird verifies the key, rejects keys
that have been tampered with, and safely deserializes the stored Erlang
terms. Likewise, Redbird only permits deletion of keys that are valid.

@germsvel
Copy link
Collaborator

Just updated the PR description. Thanks for pointing that out @danadaldos!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants