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

Security issue: Two users masquerading to the same user can be logged in as the other person #94

Closed
andyjeffries opened this issue Aug 26, 2021 · 5 comments
Assignees
Labels

Comments

@andyjeffries
Copy link

Let's say Admin A and Admin B are both masquerading as User C from their own machines.

A does it first (and

def find_owner_resource(masqueradable_resource)
stores A in the cache) then B does it (and it overwrites the cache with B).

Then A logs out on his machine and suddenly he's logged in as B (which potentially via Pundit or something has different authorisations that he shouldn't have)

I think using the Rails cache isn't the best idea for this, but maybe storing something in the Rails session?

@oivoodoo oivoodoo self-assigned this Aug 26, 2021
@oivoodoo oivoodoo added the high label Aug 26, 2021
@oivoodoo
Copy link
Owner

Hi @andyjeffries

it's a bit tricky to fix to be honest for now. Let's discuss probably, let's say I removed the session to skip any exposes of encoded data. Rails.cache is safe enough way to keep the data about the requested masquerading user.

I had this ticket #83 , going to review the code for now and think how to fix it.

@andyjeffries
Copy link
Author

OK, as an implementation idea, so how about you create a random token (SecureRandom.hex(20), a UUID or something) and you use that as a key in the Rails cache with the value being the original user's ID (or whatever). Then you set that token in the session. This isn't easily guessable. Then when leaving the masquerade, you lookup the token in the session, find that in the cache to get the user to return to, delete it from the session and continue as normal with your unmasquerade process.

Then you're not storing anything sensitive in the session, you aren't storing an easily guessable value and if an attacker gets hold of the whole session, they could unmasquerade anyway?

Completely up to you, just throwing a suggestion out there :-)

@oivoodoo
Copy link
Owner

@andyjeffries looking good. it doesn’t break anything. thank you. I will try to apply it today.

@oivoodoo
Copy link
Owner

Hi @andyjeffries Please review PR if you have time: #95

@andyjeffries
Copy link
Author

I've just noticed I'm using 1.3.11 and it's still happening for me.

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

No branches or pull requests

2 participants