You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@freemarker.apache.org by GitBox <gi...@apache.org> on 2020/06/19 06:23:15 UTC
[GitHub] [freemarker] ChangdongLi opened a new pull request #68: Disable advanced freemarker features
ChangdongLi opened a new pull request #68:
URL: https://github.com/apache/freemarker/pull/68
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [freemarker] ChangdongLi commented on pull request #68: Disable advanced freemarker features
Posted by GitBox <gi...@apache.org>.
ChangdongLi commented on pull request #68:
URL: https://github.com/apache/freemarker/pull/68#issuecomment-646983915
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [freemarker] ChangdongLi commented on pull request #68: Disable advanced freemarker features
Posted by GitBox <gi...@apache.org>.
ChangdongLi commented on pull request #68:
URL: https://github.com/apache/freemarker/pull/68#issuecomment-647052944
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 <no...@github.com> 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [freemarker] ddekany commented on pull request #68: Disable advanced freemarker features
Posted by GitBox <gi...@apache.org>.
ddekany commented on pull request #68:
URL: https://github.com/apache/freemarker/pull/68#issuecomment-647102391
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, 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [freemarker] ddekany commented on pull request #68: Disable advanced freemarker features
Posted by GitBox <gi...@apache.org>.
ddekany commented on pull request #68:
URL: https://github.com/apache/freemarker/pull/68#issuecomment-646900936
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".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [freemarker] ddekany commented on pull request #68: Disable advanced freemarker features
Posted by GitBox <gi...@apache.org>.
ddekany commented on pull request #68:
URL: https://github.com/apache/freemarker/pull/68#issuecomment-647012684
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [freemarker] ddekany edited a comment on pull request #68: Disable advanced freemarker features
Posted by GitBox <gi...@apache.org>.
ddekany edited a comment on pull request #68:
URL: https://github.com/apache/freemarker/pull/68#issuecomment-647102391
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [freemarker] kakaluote444 commented on pull request #68: Disable advanced freemarker features
Posted by GitBox <gi...@apache.org>.
kakaluote444 commented on pull request #68:
URL: https://github.com/apache/freemarker/pull/68#issuecomment-752291236
牛逼
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [freemarker] ChangdongLi commented on pull request #68: Disable advanced freemarker features
Posted by GitBox <gi...@apache.org>.
ChangdongLi commented on pull request #68:
URL: https://github.com/apache/freemarker/pull/68#issuecomment-647002703
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 <no...@github.com> 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [freemarker] ddekany commented on pull request #68: Disable advanced freemarker features
Posted by GitBox <gi...@apache.org>.
ddekany commented on pull request #68:
URL: https://github.com/apache/freemarker/pull/68#issuecomment-646993026
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [freemarker] ChangdongLi commented on pull request #68: Disable advanced freemarker features
Posted by GitBox <gi...@apache.org>.
ChangdongLi commented on pull request #68:
URL: https://github.com/apache/freemarker/pull/68#issuecomment-647003296
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 <go...@gmail.com> 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 <no...@github.com> 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>
>> .
>>
>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org