Potential for ReDoS through string replace function

[matauth0]

Hi!

String.split() used in

liquidjs/src/builtin/filters/string.ts

Line 61 in 479c633

return stringify(v).split(pattern).join(replacement)

has a potential of being used to cause ReDoS since split will accept both string and regular expression. I'm not sure if this can be triggered with just regular usage in template as:
engine.parseAndRender(" {{ 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa!' | replace:/([a-z]+)+$/,'' }} ").then(console.log)
will result in some odd splitting but maybe there's some javascript magic that could result in replace pattern to be executed as regular expression?

Realistic (but still far fetched) scenario goes as follows:

const { Liquid } = require('liquidjs');
const engine = new Liquid();

const INPUT = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa!";
const BROKEN_REGEX = /([a-z]+)+$/;

// string filters vulnerable to regexp parameter: split, replace, replace_first, remove_first
const parameters = { input: INPUT, regex: BROKEN_REGEX };
const template = `{{ input | replace:regex,'' }}`;
const html = engine.parseAndRenderSync(template, parameters);
console.log('html:', html);

Credit for this vulnerability go to https://br.linkedin.com/in/leonardozanivan

I believe that the best way to handle this would be to raise CVE so users of liquidjs will get notified that it's time to upgrade. Additional argument to modify current behavior is to have 1:1 match with Shopify's implementation. Original implementation is using gsub (https://github.com/Shopify/liquid/blob/e83b1e415990894c9517f94a8c2020ff825da027/lib/liquid/standardfilters.rb#L261) but since second argument is cast to String (using to_s) there's does't seem to be a potential for triggering similar issue in Ruby implementation.

Thank you for the great library!

[harttle]

Thank you for your demo code and research on solutions.

will result in some odd splitting but maybe there's some javascript magic that could result in replace pattern to be executed as regular expression?

The arguments of replace filter is parsed as string literal or valid identifier (regexp syntax is not supported in Liquid templates). In your case the parser fails to parse replace:/([a-z]+)+$/,'' and results in undefined or/and "" so it's equivelant to
something like .replace("").join(undefined) which evaluates to a,a,a,a,a... (a live demo goes here).

It seems hard to construct a template string to trigger this vulnerability. But it's indeed possible, as in your code snippet, to construct a malicious data as the render context. I'll check all the string filters.

[harttle]

🎉 This issue has been resolved in version 9.17.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀