ERB Injection vulnerability

[DanielHeath]

Just noticed that there's no escaping in
https://github.com/comfy/comfortable-mexican-sofa/blob/master/lib/comfortable_mexican_sofa/content/tags/helper.rb#L36

If I write:

cms:helper whitelisted_helper foo#{Kernel.exec('poweroff')}

it'll get turned into

<%= whitelisted_helper("foo#{Kernel.exec('poweroff')}") %>

When ERB interprets this, it'll execute poweroff.

[GBH]

Good catch. I think the solution is to wrap argument strings in single quotes to prevent interpolation.

[DanielHeath]

Wouldn't I be able to then do (eg)

cms:helper whitelisted_helper foo'+"#{Kernel.exec('poweroff')}"+'

to get

<%= whitelisted_helper('foo'+"#{Kernel.exec('poweroff')}"+'') %>

[GBH]

No I mean something like this:

cms:helper whitelisted_helper foo'+"#{Kernel.exec('poweroff')}"+'

would generate:

<%= whitelisted_helper('foo\'+"#{Kernel.exec(\'poweroff\')}"+\'') %>

[DanielHeath]

Ahh. As long as you escape closing singlequotes it'd be alright. Another alternative would be to encode (via base64 or similar) the argument strings, then generate a decode("base64_content").

That way you only have to worry about the 64 potentially troublesome characters instead of the entirety of unicode

[GBH]

Base64 is probably too confusing imo. It's definitely is a larger bug with how cms tags are getting parsed and initialized. Gotta make sure it's not possible to send something through content that gets interpolated down the line.

PR is welcome, but I'll be looking into this shortly.

[DanielHeath]

Base64 is confusing but you can easily be confident there's no way around it.

Ensuring a string can't be closed by blacklisting specific characters means that if you miss anything you have a bug.

[DanielHeath]

Alternatively, this is what liquid templates were built to solve.

[GBH]

Seems that only cms:partial and cms:helper are affected because those are the only tags where I construct content output in a way that strings are getting interpolated.

I looked at Liquid at one time. It does a lot of things I don't need and doesn't do a ton things that I do need. Tag params handling is something I added: https://github.com/GBH/liquid_tag_with_params but then I realised that I'd need to do some really ugly monkey-patching to make it parse content the way CMS does it now.

[GBH]

@DanielHeath Should be fixed now in master. Can you confirm that it fixes your issue?

[DanielHeath]

Yep :)