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

Check cookies_serializer option in rails #1316

Closed
korrs opened this issue Feb 19, 2019 · 11 comments

Comments

@korrs
Copy link

commented Feb 19, 2019

Rails apps can use insecure serialization for cookies, that uses marshalling! This vulnerability still has a high risk and i think brakeman should notify us about this problem.

I suggest to add check which would read rails option(Rails.application.config.action_dispatch.cookies_serializer) and would show a warning if option is equal to :marshal. I can make pr if this kind of check possible to add in a brakeman.

@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Feb 20, 2019

Hi @korrs - seems like a good idea.

Is the premise that using marshalled cookies allows an attacker to send malicious cookies that may execute code when deserialized? Do you have a proof of concept, or is it just theoretically a problem?

@korrs

This comment has been minimized.

Copy link
Author

commented Feb 20, 2019

@presidentbeef i'm almost sure this vulnerability still to be actual, i didn't try to send malicious cookies, but i read about it from this article.

@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Feb 20, 2019

Thank you for the link. However, that article is about generating session cookies via leaked secret tokens - I don't think it matters what serialization technique is used.

@korrs

This comment has been minimized.

Copy link
Author

commented Feb 21, 2019

Why does it's not matter? Maybe another vectors exist for attacks if you have a secret token but this vulnerability possible to use too).

@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Feb 22, 2019

How does changing the serialization to JSON address the issue raised in that article?

@korrs

This comment has been minimized.

Copy link
Author

commented Feb 27, 2019

@presidentbeef, do you mean that ActiveSupport::JSON not rescue from this attack and this deserialization method not safe too?

@korrs

This comment has been minimized.

Copy link
Author

commented Mar 1, 2019

I look at implementaion of decoding in ActiveSupport::JSON(which used for deserealizing of cookies if json option selected) and i can't see how possible to execute this attack.

@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Apr 26, 2019

So my thought here is we should warn about using Marshal for serializing cookies.

The blog post just threw me off because it was more about stealing secret keys, which becomes more dangerous if the application uses Marshal for session cookies or any encrypted cookie, really. The problem in the original request is actually broader than that, and I thought perhaps someone had posted about a generic attack using any cookie using Marshal.

eoinkelly added a commit to ODNZSL/nzsl-online that referenced this issue Jul 25, 2019

Br3nda added a commit to ODNZSL/nzsl-online that referenced this issue Jul 25, 2019

@gingerlime

This comment has been minimized.

Copy link

commented Jul 26, 2019

Sorry for barging in, but we were having some trouble applying this setting and not using :hybrid and I wanted to check the best course of action ...

First of all, just to make sure I get things straight, this is really a "defense-in-depth" scenario, right? because if someone has access to your secret keys, then lots of bad stuff can happen. (I agree that remote code execution would be particularly bad when your keys get compromised).

Implementing this change however going straight from :marshal to :json seems to lead to this error

JSON::ParserError unexpected token at [iI"**************;'

and then the user gets a 500, and retrying won't help either (the cookie has an old marshalled data...)

I guess the only viable solution is changing secret_key_base and invalidating everyone's cookies, right?

@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Jul 26, 2019

Hi @gingerlime - you are right this is a "defense-in-depth" setting. But consider CVE-2019-5418 had a bigger impact if using Marshal for cookies.
:hybrid is intended to help migrate from Marshal to JSON. The reason Brakeman warns about :hybrid is because I suspect there are applications using :hybrid and never switching to :json. So in your case, you can use :hybird and then when you are comfortable that existing sessions have been switched to JSON, then you can change the setting to :json.
Hope that helps.

@gingerlime

This comment has been minimized.

Copy link

commented Jul 26, 2019

Thanks for getting back to me @presidentbeef. Do you mean CVE-2019-5420 ?

Re moving to :hybrid and then :json... however long we might leave it on, there's always a chance of someone with an old cookie later on coming in. If they then get a 500 and we get noise from JSON parse errors, then it won't really help. I think we'd have to reset our secret_key_base sooner or later anyway. I can't think of any other solution, but perhaps you or someone else has a suggestion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.