Gettext parsing eval strings and that may cause slowness and security issue

[MichaelHoste]

In my ruby project, if I have a string like:

"#{puts("hello")}}"

Then when I'll parse it with Gettext, I got hello in my console. It's because the string is executed in this line: https://github.com/ruby-gettext/gettext/blob/master/lib/gettext/tools/parser/ruby.rb#L38

That's not so bad but if I have a string like this

"remove a file from disk #{FileUtils.rm('file.txt')}"

or

"remove users from DB #{User.destroy_all}"

it's already way more damaging.

Another issue with this is that it's slow down the parsing. It could be a slow request in the database (it's how I noticed that issue in the first place) or it could be a loop:

"slow request #{User.complex_scope.pluck(:id)}"

"slow loop #{100000000.times { |i| rand(i) }}"

I'm not sure why eval is called on this line. Is is really needed? What is the purpose? I would feel way more confortable if Gettext didn't try to execute my code when parsing the files.

EDIT : please take note that it's not even gettext string like _("") but only simple strings on the code. So I'm not sure why Gettext is trying to execute them.

[MichaelHoste]

I found a way to make all the tests pass without using eval.

It needs to replace:

begin
  s = eval(s)
rescue Exception
  # Do nothing.
end

with:

s = s.strip                 # remove extra spaces around quotes
s = s[1..-1]                # remove first quote
s = s[0...-1]               # remove ending quote
s = s.gsub("\\\"", '"')     # unescape quotes
s = s.gsub("\\n", "\n")     # unescape new lines
s = s.gsub(/\n(\s)*\"/, '') # unescape literal concatenation with continuation line
s = s.gsub("\\#", "#")      # for assert_target "\#", ['fixtures/_.rb:88', 'fixtures/_.rb:92'] test

It's ugly but it works without executing the code.

My guess is that there will be some side effects (some non-unescaped stuff that are not in the tests). So another solution would be a better unescape method. Do you have any thoughts?

If you still want to use eval One quick fix would be to use this:

if !s.include?('#{')
  s = eval(s)
end

It still executes the string but not when #{ is found in it. It's ugly but it's a lot safer.

What do you think? Do you want me to do a pull request?

[calvincorreli]

Another idea: gsub any #{...} with #{''} before eval'ing?

[kou]

Thanks for your report. I replaced eval with Ripper.

[MichaelHoste]

Thanks a lot for your responsiveness @kou!

I feel better now that the strings are not evaluated anymore. And the parsing seems a bit faster too. I'm glad Ripper did the job. Thanks a lot (and thanks @calvincorreli for finding the issue in the first place).

[calvincorreli]

Yeah, that was super fast. Thank you!

[MichaelHoste]

@kou: it seems to have at least one regression with this change.

In the code of one of my projects, that line is not correctly escaped anymore:

_('Blabla d\'indexation blabla.')

Before, the PO contained "Blabla d'indexation blabla" (and it made sense) but now it appears as "Blabla d\\'indexation blabla."

I guess \' should be escaped too (maybe a lot of other stuff too but I'm not sure what needs to be escaped to be complete)

[kou]

Good catch. I've fixed it.

[MichaelHoste]

I tried the new version on my project and it works great.

Thank you for your quick reaction, it's really appreciated!