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 requirements class before loading marshalled requirements #4651

Merged
merged 2 commits into from Jul 25, 2021

Conversation

nobu
Copy link
Contributor

@nobu nobu commented Jun 4, 2021

Mitigate the security risk:
https://devcraft.io/2021/01/07/universal-deserialisation-gadget-for-ruby-2-x-3-x.html

What was the end-user or developer problem that led to this PR?

Unexpected method (Kernel#system in the article) can be called by just loading marshaled data.

What is your fix for the problem, implemented in this PR?

Check the classes of @requirements and its elements.

Maybe fix_syck_default_key_in_requirements is no longer used and can be removed, but not sure.

Make sure the following tasks are checked

@deivid-rodriguez
Copy link
Member

Hi @nobu, thanks!

I think this is a good opportunity to move #3797 forward. I do think fix_syck_default_key_in_requirements can be removed at this point. There was some theoretical backwards compatibility concerns, but we never really clarified them, so if this is a security risk I think we can just remove the vulnerable code.

@hsbt What do you think?

@deivid-rodriguez
Copy link
Member

@nobu After having another look at this, even if we remove fix_syck_default_key_in_requirements, we are still vulnerable to this, with the difference that we would only need to check that @requirements is an Array upon marshal loading. Correct?

@nobu
Copy link
Contributor Author

nobu commented Jul 18, 2021

Correct, this doesn't fix the risk totally, but just "mitigate" for old ruby versions.

@deivid-rodriguez deivid-rodriguez force-pushed the check-requirements-classes branch 2 times, most recently from 8621c7c to 662b9c6 Compare July 22, 2021 09:34
@deivid-rodriguez
Copy link
Member

I do believe that we can remove all syck traces from rubygems at this point, and it does simplify this patch, so I included removing the fix_syck_default_key_in_requirements method in this PR.

@deivid-rodriguez deivid-rodriguez changed the title Check requirements classes Check requirements class before loading marshalled requirements Jul 22, 2021
deivid-rodriguez and others added 2 commits July 23, 2021 16:28
After reading [this blog
post](https://blog.rubygems.org/2011/08/31/shaving-the-yaml-yak.html),
published almost 10 years ago already, my understanding is that this
problem could come up in two ways:

* Rubygems.org serving corrupted gemspecs". As far as I understand this
was fixed in rubygems.org a lot time ago, since
rubygems/rubygems.org#331.

* Clients having a ten years old gemspec cache with some of these bad
gemspecs. In this case, there's no easy solution but I think ten years
is enough and rebuilding the cache should do the trick.

So, I think it's time we remove this.
@deivid-rodriguez
Copy link
Member

I do believe this shouldn't break anything, so I'll take this opportunity to clean things up and improve security at the same time. I don't think this is really exploitable so I tagged it just as an enhancement.

@deivid-rodriguez
Copy link
Member

Thanks @nobu!

@deivid-rodriguez deivid-rodriguez merged commit a75f579 into rubygems:master Jul 25, 2021
deivid-rodriguez added a commit that referenced this pull request Jul 30, 2021
Check requirements class before loading marshalled requirements

(cherry picked from commit a75f579)
deivid-rodriguez added a commit that referenced this pull request Jul 30, 2021
Check requirements class before loading marshalled requirements

(cherry picked from commit a75f579)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants