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

Fix CVE-2023-31606 (ReDOS possible in the sanitize_html function) #75

Merged
merged 2 commits into from
Nov 2, 2023

Conversation

korny
Copy link
Contributor

@korny korny commented Jun 28, 2023

A potential fix for #73 (https://github.com/e23e/CVE-2023-31606#readme).

The use of a possessive quantifier effectively prevents backtracking in the second group (which matches the tag):

    text.gsub!( /<(\/*)([A-Za-z]\w*+)([^>]*?)(\s?\/?)>/ ) do |m|

In this case, the \w*+ part (and therefore, the whole group ([A-Za-z]\w*+)) will not backtrack. The rest of this regex is not subject to ReDoS (it's linear). So, we can use this version; it should work in Ruby 1.9 and up.

Copy link

@merbinr merbinr left a comment

Choose a reason for hiding this comment

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

Works fine, LGTM

@doconnor-clintel
Copy link

doconnor-clintel commented Jul 11, 2023

@korny - guessing the maintainer of this gem is MIA.

Would it be worth forking the gem, renaming slightly, publishing a new version? (ScarletCloth?)
That way there's a migration pathway for others affected; so they don't have to rely on git branches.

Weirdly, v4.3.2...sofatutor:redcloth:fix-CVE-2023-31606 has a few extra differences which result in

$ bundle exec rake assets:precompile
rake aborted!
LoadError: cannot load such file -- redcloth_scan
Couldn't load redcloth_scan

Though it's not obvious to me why anything there would make it skip trying to compile:
https://github.com/jgarber/redcloth#label-Compiling

@heliocola
Copy link
Collaborator

@korny , @doconnor-clintel : do you know if the patch has been published to rubygems?

@joshuasiler
Copy link
Contributor

I'm no longer able to maintain this repo but am happy to turn it over to someone with interest and inclination. Feel free to reach out if you are interested.

@heliocola
Copy link
Collaborator

@joshuasiler : 👋

@joshuasiler
Copy link
Contributor

I just checked the repo, and it turns out that while I have read/write access, I do not have admin access to the repo. So I am unable to transfer admin. Best bet would be to contact the original owner or fork the repo. Sorry I can't help further.

@jgarber
Copy link
Owner

jgarber commented Oct 30, 2023 via email

@digitalmoksha
Copy link

I’m admin and would be happy to give out additional access. It’s been on my to-do list to look at this PR but I haven’t gotten to it, yet.

@jgarber that would be great if that's a possibility.

I work for GitLab (https://gitlab.com/digitalmoksha). We still use RedCloth - it’s not a huge component for us, but we do still use it. I also use it on a couple of older personal projects.

I would be interested in being able to maintain it, at the very least getting the security fixes published.

Do you think this would be a possibility? I don’t have a huge amount of time to devote to it, but I would rather see the security problems dealt with in the main gem rather than relying on forks. And there are a couple other fixes that I think would be useful for the community.

wdyt?

@heliocola
Copy link
Collaborator

@jgarber : rooting for you to pick either @digitalmoksha or myself as a new admins!
Or somebody else you have in mind that also has interest and time! 💟

@jgarber
Copy link
Owner

jgarber commented Nov 1, 2023

@digitalmoksha @heliocola Sent you both access. Thank you so much for stepping up! 🙏
Go to town with fixes and enhancements. I, too, would like people not to have to rely on forks! It's wonderful if RedCloth can still be useful to some people out there. Sorry I can't maintain it myself!

@digitalmoksha
Copy link

Thank you @jgarber 🙇

@heliocola
Copy link
Collaborator

Thank you @jgarber !

@heliocola heliocola merged commit 8b13276 into jgarber:master Nov 2, 2023
Repository owner deleted a comment from heliocola Nov 2, 2023
@jgarber
Copy link
Owner

jgarber commented Nov 2, 2023

@heliocola You’re a rubygems admin now. Thank you so much for getting these fixes to a release!

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.

7 participants