-
Notifications
You must be signed in to change notification settings - Fork 374
Fix: Harden FreeMarker against SSTI #705
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hi @angelozerr, thanks for your reply. |
| @@ -0,0 +1,146 @@ | |||
| # XDocReport FreeMarker SSTI Security Fixes | |||
|
|
|||
| ## Tổng quan | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use english
|
Ci build fails with your test https://github.com/opensagres/xdocreport/actions/runs/18632685970/job/53438957761#step:4:1797 |
7bcfff7 to
b4d312f
Compare
|
Hi @angelorerr, |
|
@AT190510-Cuong it is hard to follow your PR since code changes everytime. I have no idea what your PR fixes now? |
|
@angelozerr Thank you for the feedback, and I apologize again for the confusion. Issue:
Fix:
Code Changes:
|
|
@AT190510-Cuong I understand now what you are trying to do, but I don t think it is relevant for XDocReport project. Indeed I dont see any usecase where a user will write a mergefield with freemarker operator. And even if a report contains this syntax it is the resposability of the application which uses XDocReport which must take care of that. It is the same thing when you are writting a webapp application which uses freemarker to render html page. I am sorry to not accept your PR. It is not relevant for XDocReport project. |
|
Hi @angelozerr , Thanks for your response — I completely understand your point. I agree that XDocReport is designed to render variables like A similar case happened with Log4Shell (Log4j) — the library was only supposed to log simple variables like Likewise, I’ve seen a real-world example where an employee management web app allowed users to upload That’s why I believe having safe defaults, or at least a security note in the documentation, would greatly reduce the risk for downstream users. |
Good catch! Excellent usecase!
You have changed my mind. Please add again some tests and add more comments in your code by setting an exemple of malicious field. |
| // Security fix: Block dangerous class instantiation via ?new operator to prevent SSTI attacks | ||
| try | ||
| { | ||
| this.freemarkerConfiguration.setSetting( Configuration.NEW_BUILTIN_CLASS_RESOLVER_KEY, "safer" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this constant exists in any freemarker version? I - think you should catch Exception instead of catching TemplateException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@angelozerr Thanks for the question!
Configuration.NEW_BUILTIN_CLASS_RESOLVER_KEY was introduced in FreeMarker 2.3.30, so it is supported in the FreeMarker version currently used in XDocReport (2.3.32).
|
LGTM, let's see the CI build result. |
|
Hi @angelozerr , |
|
Thanks @AT190510-Cuong ! |
Summary
This pull request hardens the FreeMarker template rendering logic to prevent potential Server-Side Template Injection (SSTI) vulnerabilities.
Details
Impact
Prevents attackers from injecting malicious template expressions that could lead to arbitrary code execution.
References