Dependencies using Liqp may affect template execution due to shared global state

[kohlschuetter]

Currently, there are several places in the API that may have unsafe side-effects on otherwise unrelated code that shares Liqp running in the same VM. This is due to global scope that allows modifications around Insertions and Filters, which are stored as static HashMaps shared across all callers.

In the worst case this could be seen as a security issue because unrelated code may interfere and override a certain insertion (tag or block), leading to information disclosure or data manipulation.

The bug may also just trigger a ConcurrentModificationException when these modifications occur at an inopportune time.

[kohlschuetter]

Proposed fix in https://github.com/bkiers/Liqp/pull/241

[msangel]

My opinion on this: let the user[target developer] decide: either use own instance either shared one via static methods (inside of static methods just modify shared TemplateParser.DEFAULT instance)

So, just took your implementation BUT left the old API interface as is: not mark methods as deprecated and undo methods removal(if any).

(and maybe use ConcurrentHashMap instead of HashMap where concurrency cases may appear, as well as use CopyOnWriteArrayList instead of ArrayList for the same)

Main decision will be made by @bkiers, as he might decide to go away from static context too, with:

• pros: better thread-save and secure software
• cons: strong breaking backward compatibility, removal of easy handy static methods (now we always need to create templator instance)

[kohlschuetter]

Do we want to leave users exposed to this security risk? Sure, but let's please mark these methods as deprecated (this is what my patch does).

Note that the bug is not about thread-safety. It's enough if a dependency initializes Liqp after your own code does.

[msangel]

Some kind of equivocation here. Security will be better when the user will have the possibility to create a clean default templator instance with custom tags/filters and without impact from static context and so this will solve the issue described above.

Marking static methods deprecated will them just forced to use their own instance approach everywhere. Having static access on place is not that bad. Yes, the static context may be altered by other libraries, BUT eventually, that is what people may want. Like in the very first sample of README.md with filter usage:

// first register your custom filter
Filter.registerFilter(new Filter("b"){
    // omitted
});

// use your filter
Template template = Template.parse("{{ wiki | b }}");

[kohlschuetter]

Unfortunately, Filter.registerFilter and Tag.registerTag overwrite any previously registered Filter and Tag, that's the problem.

I think the situation could be somehow fixed by not allowing Filter.registerFilter/Tag.registerTag to overwrite existing entries. However that breaks backwards compatibility.

In my use case I need to replace the include tag with a custom one that sources input differently, from a controlled source using more security protections. I cannot use the the existing include tag, and I cannot risk that some library that my own library depends on eventually turns to Liqp and provides their own include tag as well.

The example from the README is deceptively simple, until it explodes in your face.

[msangel]

Unfortunately, Filter.registerFilter and Tag.registerTag overwrite any previously registered Filter and Tag, that's the problem.

So my proposition on this is actually: allow override in static context BUT not in the default [abstract] context(that is used as a prototype for new templator instances). Each new templator should get an untouched original configuration, while static context will be accessible for all possible modifications (like it is now).

For achieving this need just a few changes on top of your commits.

It still will break backward compatibility, but not that hard.

[kohlschuetter]

So my proposition on this is actually: allow override in static context BUT not in the default [abstract] context(that is used as a prototype for new templator instances). Each new templator should get an untouched original configuration, while static context will be accessible for all possible modifications (like it is now).

I think this is what this current patch does.

It still will break backward compatibility, but not that hard.

I've provided the patch with the test cases separated. This way you can verify that the current patch actually does not break backward compatibility.

Also see src/test/java/liqp/InsertionTest.java where I added a test for this behavior (testNestedCustomTagsAndBlocksDeprecated):

    Block.registerBlock( ... ) 

    Tag.registerTag(new Tag("simple") {
              @Override
              public Object render(TemplateContext context, LNode... nodes) {
                  return "(sim)";
              }
          });

    String templateString = "(...) {% simple %} (...)";

    try {
        // The default TemplateParser is not affected by the global registrations above
        TemplateParser.DEFAULT.parse(templateString);
        fail("Should have thrown a LiquidException");
    } catch (LiquidException e) {
        // expected
    }

    // The old API is affected (beware of dangerous side effects)
    Template template = Template.parse(templateString);

Where's the disconnect? What am I missing?

[msangel]

You are right. Two points:

1. Regarding recommendations of "old" and "new" API. Marking something as deprecated means it will be removed in the future. But I don't see any strong reason to remove static methods at all in any future as they are quite handy.

2. Also, if we are already moving down this road, we can do even more: remove all mutable static fields in all internals and in static methods internally just delegate all calls to STATIC instance. This will simplify the code a lot. So we will have: TemplateParser.DEFAULT as prototype and TemplateParser.STATIC for static calls and modifications.

[kohlschuetter]

How amenable are you to API changes? Since I don't require old API, I'm all-in for simplifying things.

On the other hand, I'm not sure the proposed TemplateParser.STATIC would be a route I'm willing to follow.

The ultimate goal of having builds and immutable data structures is to eliminate any checks at execution time that can be performed during setup.

In a not-so-distant future, I could see that TempleParser actually has different implementations depending on the builder configuration. Allowing state changes through STATIC as you propose may actually make it more difficult to end up with a clean design.

The patch I proposed, as an alternative, leaves the existing route intact, marks everything with You've been warned stickers (aka deprecation warnings), and slowly moves away from the whole static concept for those who care.

[bkiers]

Marking something as deprecated means it will be removed in the future.

Yes, as a consumer of a library, I'd assume any deprecated methods to be gone sooner than later. I'd prefer to have them not be marked as deprecated, unless they really are going to be ditched in the near future. Having them along-side a more secure way of parsing and rendering Liquid source files is also fine by me.

I am not too sure what PR https://github.com/bkiers/Liqp/pull/241 is kept from being merged though. From reading the comments and code, the proposed changes are still backwards compatible, but @msangel has some reservations of the existing (static) methods being deprecated (which reservations I also have). Would removing the deprecated annotations, and properly documenting the difference between the 2 ways of rendering/parsing in the README be sufficient? That way the old way will still be available (without the deprecation warnings) and a more secure route would also be possible.

As you might know, I am not actively developing on Liqp, and @msangel is pretty much the driving dev-force behind this library. So I'll leave this in @msangel's more than capable hands.

[kohlschuetter]

Thanks for commenting, @bkiers!

I'd be more than happy to see the now-deprecated static-scope methods be removed sooner than later. They're inherently unsafe.

I propose to remove the deprecated methods at the next chance where breaking API changes occur (or once we turn to parallel unit tests, which are also affected). — As you see in PR #241, I've taken great care to avoid that for now.

I'm afraid I cannot rely on a Liqp version that still has them built in without a clear deprecation/removal strategy.

The problem is that if my library depends on a vulnerable Liqp version, any dependencies still using the unsafe path may feed back unsafe data into my system. These problems are super-annoying not only because they're dangerous but also because they're usually very hard to debug.

[kohlschuetter]

Closing, since 88f7d1c was merged.