Disable advanced freemarker features

[ChangdongLi]

many open source projects depend on Freemarker. They initialize Freemarker by calling the default Configuration constructor. e.g. JODReports. The default constructor - new Configuration() has built API enabled and can execute commands and can create some new instances. This can cause a critical security issue by default.

This code change allows disabling those feature by calling Configuration.setDefaultNewBuiltinClassResolver(TemplateClassResolver.ALLOWS_NOTHING_RESOLVER); Configuration.setDefaultAPIBuiltinEnabled(false); Configuration.setExternalCommandsAllowed(false); Note those need to run before any new Configuration() code.

[ddekany]

If you haven't, please read this first: https://freemarker.apache.org/docs/app_faq.html#faq_template_uploading_security

As you see, if you can't trust template authors (which is not the situation FreeMarker was originally designed for - think about a better alternative to 2000-s JSP, where you could just have java code in your "templates"), then making templates safe is a lot more work than flipping some configuration settings, unless you are very, very lucky. The most important bit is that people want to call the "legitimate" Java API-s of the objects that were exposed to the template. I'm not talking about ?api here, but about how "generic" java objects, aka POJO-s, are normally exposed. Like in user.name (probably calls User.getName()), users.forRole('admin'), etc., you utilize that. Then, they will also access the objects that these methods return, and so on, potentially a huge graph of objects. Plus, even when the class of a deliberately exposed POJO is developed, as these domain classes are usually not just for the templates, the developer doesn't constantly consider that any public method they adds will be callable from templates. So, I think, in most real world scenarios, it's not feasible to keep API access under control without whitelisting. The whitelist will be specific to the application, and, of course, it's far from a boolean switch. It's not possible to make a simple safety on/off switch.

Regarding frameworks that doesn't let you configure FreeMarker, yet doesn't care to configure it properly itself. What can I say... they just didn't care then. In any real world project, you will want to configure FreeMarker, even if the security aspect doesn't matter. Given how tricky fencing malicious template authors is (see linked FAQ entry), we have no much chance in forcing security on such a framework.

What can we still do? If we go for extremes, then, we could introduce some static method where you can set up a callback, and where you can modify the new Configuration before it's returned from the constructor. But that's a horrible thing to do. That hook will affect all libraries that internally use FreeMarker (in case there happens to be more in the same application), not just the one you intended to target. You could target by look into the stack trace, or via thread local storage, if you are lucky though. You also can't know what settings will the framework set after the constructor, and hence, after your callback. Sure, then we could have an API to capture those setter calls as well, so you can override them... now that starts to be really awkward. Also it can be far from obvious for others, that these hooks exist somewhere, causing these mysterious changes, which can cause lot of confusion. So, I really hate the idea, but that's the most I can imagine for "forcing" security.

So, I'm afraid, realistically, the framework authors, who "own" the Configuration, has to care about security, and mostly, about the safety of the data-model. If they they do care about the data-model, probably they will also care about configuring the other aspects (disabling this and that), as that's really the easier part. If they try to do that, and have difficulties, then, like in the recent versions, we may add features to help there. But they have to "opt in".

[ChangdongLi]

Hi ddekany, thanks for your detailed response. we already sanitized the templates and used a hardcoded FreeMarker version which has those advanced features removed. it doesn't allow executing external commands and initializing new instances. We just hope the official Freemarker itself can have a solution to disable those features even other frameworks didn't think or care about security at that time. You don't need to force security for other frameworks authors. They may stop maintaining those as you can image. This pull request just gives the end-user the chance to disable those features simply although This pull request is not perfect. In the meantime, I will review those frameworks we used. thanks.

[ddekany]

Just to restate the problem. Official FreeMarker does have features to disable these on the level of the Configuration object. Your problem is that sometimes you don't have control over the Configuration instance. So, you started rolling your own in-house FreeMarker fork, that just doesn't have some dangerous features, and what you need is a way to achieve the same without actual forking.

If we just have some static method, where you can fiddle with the Configuration instance, that perhaps can be too easily used for attacks. If others manage to call that static method (yes, then you already have some big problem, but still), they can make a previously secure system insecure. Also, it's not always easy to guarantee that your static method call happens before the relevant Configuration instance was created. Such uncertainty is especially undesirable when security depends on it. So maybe, we rather should have class with a fixed, predefined name, let's say org.apache.freemarker.monkeypatch.FeatureDisabler, and that's where you can do your thing. Because it's a documented, fixed place, it's also easier to figure out if such magic happens. The only problem is that as the presence of this calls is optional in FreeMarker, the system will still run if somehow it's not there anymore. For that though, you can refer to FeatureDisabler in your application own startup code, so your application will fail if that class is gone for some reason.

Though I'm still not sure how do you intend to ensure that the data-model content itself is safe. Well, you might as well specify a whitelist in org.apache.freemarker.monkeypatch, though I'm not sure how realistic that is.

[ChangdongLi]

⁣you are right. The solution you mentioned is better than that pull request.  The data model content in our application is a hash map. Its keys are fixed strings. The template doesn't allow to refer to other keys. Will this data model content be safe? So far combined  with the  features disabled via hard coding, our penetration tester said okay. If we will be able to disable some features in the future official FreeMarker version via the solution you mentioned, Will this be enough for security? Thanks

On 20 Jun. 2020, 23:13, at 23:13, ddekany [email protected] wrote:

Just to restate the problem. Official FreeMarker does have features to disable these on the level of the Configuration object. Your problem is that sometimes you don't have control over the Configuration instance. So, you started rolling your own in-house FreeMarker fork, that just doesn't have some dangerous features, and what you need is a way to achieve the same without actual forking.

If we just have some static method, where you can fiddle with the Configuration instance, that perhaps can be too easily used for attacks. If others manage to call that static method (yes, then you already have some big problem, but still), they can make a previously secure system insecure. Also, it's not always easy to guarantee that your static method call happens before the relevant Configuration instance was created. Such uncertainty is especially undesirable when security depends on it. So maybe, we rather should have class with a fixed, predefined name, let's say org.apache.freemarker.monkeypatch.FeatureDisabler, and that's where you can do your thing. Because it's a documented, fixed place, it's also easier to figure out if such magic happens. The only problem is that as the presence of this calls is optional in FreeMarker, the system will still run if somehow it's not there anymore. For that though, you can refer to FeatureDisabler in your application own startup code, so your application will fail if that class is gone for some reason.

Though I'm still not sure how do you intend to ensure that the data-model content itself is safe. Well, you might as well specify a whitelist in org.apache.freemarker.monkeypatch, though I'm not sure how realistic that is.

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/apache/freemarker/pull/68#issuecomment-646993026

[ChangdongLi]

BTW you can view the hardcoded version here https://github.com/ChangdongLi/freemarker/commit/99d7fa016d0d5677620b5d189727519175aaf153

Best Regards,

Danny

On Sun, 21 Jun 2020 at 00:27, Danny Li [email protected] wrote:

you are right. The solution you mentioned is better than that pull request. The data model content in our application is a hash map. Its keys are fixed strings. The template doesn't allow to refer to other keys. Will this data model content be safe? So far combined with the features disabled via hard coding, our penetration tester said okay. If we will be able to disable some features in the future official FreeMarker version via the solution you mentioned, Will this be enough for security? Thanks On 20 Jun. 2020, at 23:13, ddekany [email protected] wrote:

Just to restate the problem. Official FreeMarker does have features to disable these on the level of the Configuration object. Your problem is that sometimes you don't have control over the Configuration instance. So, you started rolling your own in-house FreeMarker fork, that just doesn't have some dangerous features, and what you need is a way to achieve the same without actual forking.

If we just have some static method, where you can fiddle with the Configuration instance, that perhaps can be too easily used for attacks. If others manage to call that static method (yes, then you already have some big problem, but still), they can make a previously secure system insecure. Also, it's not always easy to guarantee that your static method call happens before the relevant Configuration instance was created. Such uncertainty is especially undesirable when security depends on it. So maybe, we rather should have class with a fixed, predefined name, let's say org.apache.freemarker.monkeypatch.FeatureDisabler, and that's where you can do your thing. Because it's a documented, fixed place, it's also easier to figure out if such magic happens. The only problem is that as the presence of this calls is optional in FreeMarker, the system will still run if somehow it's not there anymore. For that though, you can refer to FeatureDisabler in your application own startup code, so your application will fail if that class is gone for some reason.

Though I'm still not sure how do you intend to ensure that the data-model content itself is safe. Well, you might as well specify a whitelist in org.apache.freemarker.monkeypatch, though I'm not sure how realistic that is.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/apache/freemarker/pull/68#issuecomment-646993026, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANACRXVK4NDAKSPB2QWXYTRXSYXJANCNFSM4OCN6PFA .

[ddekany]

Depends on what kind of values you have in that HashMap. If they are maps/lists/strings/number/boolean/dates, then it should be safe as far as the model is concerned. (Normally, if you don't intend to expose POJO-s, you should set config.objectWrapper to a SimpleObjectWrapper.) Also note that .locale_object is a POJO that's always exposed, though I'm not aware of attack vectors there, assuming FreeMarker is up to date (or has a properly restricting ObjectWrapper). Other than that, TemplateLoader-s can be a problem, as they are accessible through #include/#import, and DoS attacks (see these in the linked FAQ entry).

As of your version, Execute can't be obtained since ?new is disabled. But, I guess you still prefer disabling these models, and then disable ObjectConstructor should be disabled as well.

[ChangdongLi]

Thanks. In our application, the values are strings.I will try to harden the code as you suggested. BTW our penetration tester followed https://ackcent.com/blog/in-depth-freemarker-template-injection/ Maybe some features could be disabled by default in the future FreeMarker

On 21 Jun. 2020, 01:53, at 01:53, ddekany [email protected] wrote:

Depends on what kind of values you have in that HashMap. If they are maps/lists/strings/number/boolean/dates, then it should be safe as far as the model is concerned. (Normally, if you don't intend to expose POJO-s, you should set config.objectWrapper to a SimpleObjectWrapper.) Also note that .locale_object is a POJO that's always exposed, though I'm not aware of attack vectors there, assuming FreeMarker is up to date (or has a properly restricting ObjectWrapper). Other than that, TemplateLoader-s can be a problem, as they are accessible through #include/#import, and DoS attacks (see these in the linked FAQ entry).

As of your version, Execute can't be obtained since ?new is disabled. But, I guess you still prefer disabling these models, and then disable ObjectConstructor should be disabled as well.

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/apache/freemarker/pull/68#issuecomment-647012684

[ddekany]

To be safe by default in use cases where template authors aren't trusted, the default object wrapper had to be extremely restrictive. You wouldn't be able to access anything but basic functionality of Map-s, Collection-s, arrays, strings, numbers, booleans and dates. We couldn't expose anything about POJO-s (unless method where explicitly whitelisted, or annotated as template accessible). I'm not sure if that would make sense as a default, as it's unusable in the usage pattern that FreeMarker was originally made for. (Also as a default it would be highly backward incompatible, but that's a different topic. But it can't be on out of the box in 2.x.x for sure.)

The attacks described in said blog post doesn't work out-of-the-box on FreeMarker 2.3.30, as some key methods used there were blacklisted, as they are very unlikely to be used (and then still can be turned back on). Also, AFAIR, the whole attack there starts with a privilege escalation attack (not related to FreeMarker), so users who are not supposed to can edit templates. Anyway, I asked them to link to the relevant FAQ entry at least, to no avail.

[intellijboy]

牛逼

[ddekany]

Can we close this, or there's more to it?

Note that 2.3.30 has added MemberAccessPolicy-es, which are very useful for restricting what templates can do.

[ddekany]

Closing this due to no further follow up.