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

Make CheckSymbolDoS an optional check #570

merged 1 commit into from Oct 30, 2014


None yet
2 participants
Copy link

commented Oct 23, 2014

but keep check for CVE-2013-1854 on by default.

I did some very simple benchmarking creating symbols (with ruby 2.1.2p95 (2014-05-08 revision 45877) [x86_64-linux]). After creating 10 million symbols, the memory usage was ~2.3GB. Assuming a single-threaded, single-process Rails app with 100ms response times creating one new symbol per request, that would take ~11.5 days (late at night, math might be wrong). I don't think this is a very effective DoS attack. If you have multiple Rails processes, then one falling over and restarting once every few weeks is not a huge deal.

Additionally, Ruby 2.2 will garbage collect symbols, so eventually this will be a problem of the past.

I'm curious if people think this is a serious enough security issue to warrant these kinds of warnings, or if it is okay to make this an optional check which people can run if they are worried about memory leaks.

Alternatively, the confidence/severity of these warnings could be set to low.

Make CheckSymbolDoS optional
by keep check for CVE-2013-1854 on by default

This comment has been minimized.

Copy link

commented Oct 24, 2014

I've heard of this happening once in the wild and it was a bug, not an attack. Turning it off by default would be totally reasonable, but if we want to infer ruby version, there are a number of ways to go. The is .ruby_version, the Gemfile, the syntax, the ruby version running breakman, etc...


This comment has been minimized.

Copy link
Owner Author

commented Oct 29, 2014

Even with versions of Ruby < 2.2, my opinion is that this is not a serious security issue given the volume of requests needed for a successful memory exhaustion attack. It doesn't seem like anyone has a strong opposing opinion.

presidentbeef added a commit that referenced this pull request Oct 30, 2014

Merge pull request #570 from presidentbeef/symbol_dos_is_optional
Make CheckSymbolDoS an optional check

@presidentbeef presidentbeef merged commit b9cf436 into master Oct 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed

Repository owner locked and limited conversation to collaborators Feb 16, 2016

@presidentbeef presidentbeef deleted the symbol_dos_is_optional branch Jul 22, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.