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 regexp #6

Merged
merged 1 commit into from
Jan 15, 2022
Merged

fix regexp #6

merged 1 commit into from
Jan 15, 2022

Conversation

ooooooo-q
Copy link
Contributor

I used Regexploit to find a regular expression that could be ReDoS for erb.
It's not a security issue, and I think it's extremely rare that it could affect performance.

sample

require 'erb'

template = ERB.new <<EOF
<%#-*-#{' '* 3456}%>
  __ENCODING__ is <%= __ENCODING__ %>.
EOF
puts template.result
❯ time ruby redos.rb

  __ENCODING__ is UTF-8.
ruby redos.rb  41.21s user 0.18s system 99% cpu 41.541 total

Results of Regexploit

-\*-\s*(.*?)\s*-*-$
Pattern: -\*-\s*(.*?)\s*-*-$
---
Redos(starriness=3, prefix_sequence=SEQ{ [2d:-] [2a:*] [2d:-] }, redos_sequence=SEQ{ [SPACE]{0+} .{0+} [SPACE]{0+} [2d:-]{0+} [2d:-] }, repeated_character=[SPACE], killer=None)
Worst-case complexity: 3 ⭐⭐⭐ (cubic)
Repeated character: [SPACE]
Example: '-*-' + ' ' * 3456

/-\*-\s*([^\s].*?)\s*-*-$/
No ReDoS found.

@@ -724,7 +724,7 @@ def detect_magic_comment(s, enc = nil)
frozen = nil
s.scan(re) do
comment = $+
comment = $1 if comment[/-\*-\s*(.*?)\s*-*-$/]
comment = $1 if comment[/-\*-\s*([^\s].*?)\s*-*-$/]
Copy link
Member

Choose a reason for hiding this comment

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

Is it intentional that you changed the behavior of the regexp?

"-*--"[/-\*-\s*(.*?)\s*-*-$/] #=> "-*--"

"-*--"[/-\*-\s*([^\s].*?)\s*-*-$/] #=> nil

First of all, I believe the magic comment syntax is -*- xxx -*-, so it's weird that the last * is not escaped. Would just escaping it fix the ReDoS problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the change in regexp behavior was unintentional.

Even if escape last *, the ReDoS problem seems to remain.

# current
❯ time ruby -e '"-*-#{" "* 3456}"[/-\*-\s*(.*?)\s*-*-$/]'
ruby -e '"-*-#{" "* 3456}"[/-\*-\s*(.*?)\s*-*-$/]'  41.47s user 0.14s system 99% cpu 41.773 total

# escape last `*` 
❯ time ruby -e '"-*-#{" "* 3456}"[/-\*-\s*(.*?)\s*-\*-$/]'
ruby -e '"-*-#{" "* 3456}"[/-\*-\s*(.*?)\s*-\*-$/]'  30.50s user 0.12s system 99% cpu 30.741 total

# fix ReDoS
❯ time ruby -e '"-*-#{" "* 3456}"[/-\*-\s*([^\s].*?)\s*-*-$/]'
ruby -e '"-*-#{" "* 3456}"[/-\*-\s*([^\s].*?)\s*-*-$/]'  0.05s user 0.05s system 73% cpu 0.129 total

# fix ReDoS and escape last `*`
❯ time ruby -e '"-*-#{" "* 3456}"[/-\*-\s*([^\s].*?)\s*-\*-$/]'
ruby -e '"-*-#{" "* 3456}"[/-\*-\s*([^\s].*?)\s*-\*-$/]'  0.05s user 0.05s system 75% cpu 0.122 total

/-\*-\s*([^\s].*?)\s*-\*-$/ seems to be a regular expression with the correct intent.

Copy link
Member

Choose a reason for hiding this comment

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

I was only looking at this line, but looking at how comment is used, the change seems fine. So I'll merge your change, also addressing \* separately.

@k0kubun k0kubun merged commit 33100a0 into ruby:master Jan 15, 2022
matzbot pushed a commit to ruby/ruby that referenced this pull request Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants