You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by jl...@apache.org on 2022/04/04 06:50:55 UTC
[ofbiz-framework] branch trunk updated (fcf0e456b6 -> 2aeb282cdc)
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a change to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
from fcf0e456b6 Improved: no functional change, adds GitHub Actions build status emails.
new 5d4dcd2ed4 Improved: just a tiny comment change in security.properties
new 2aeb282cdc Improved: Prevent Freemarker interpolation in fields (OFBIZ-12594)
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../ofbiz/product/category/SeoContextFilter.java | 7 +-
framework/security/config/security.properties | 6 +-
.../org/apache/ofbiz/security/SecurityUtil.java | 78 +++++++++++++++++++++-
.../apache/ofbiz/webapp/control/ControlFilter.java | 9 ++-
4 files changed, 95 insertions(+), 5 deletions(-)
[ofbiz-framework] 01/02: Improved: just a tiny comment change in security.properties
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 5d4dcd2ed490eb61f8a95bef5fe62140f5af08cb
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Sat Apr 2 16:24:17 2022 +0200
Improved: just a tiny comment change in security.properties
Make clear that it's impossible to create a complete deniedWebShellTokens
---
framework/security/config/security.properties | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties
index f106f9b21f..03c6804e89 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -249,7 +249,7 @@ allowAllUploads=
#-- TODO.... to be continued with known webshell contents... a complete allow list is impossible anyway...
#--
#-- It could notably be improved by checking for all Javascripts payloads.
-#-- As listed at https://portswigger.net/web-security/cross-site-scripting/cheat-sheet,
+#-- But as listed at https://portswigger.net/web-security/cross-site-scripting/cheat-sheet,
#-- at 2022-02-25 there are 8929 of them considering all tags, all events and all browsers...!
#--
#-- "freemarker" should be OK, should not be used in Freemarker templates, not part of the syntax.
[ofbiz-framework] 02/02: Improved: Prevent Freemarker interpolation in fields (OFBIZ-12594)
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 2aeb282cdc792e4a30274bffb57d63f3829bcca7
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Mon Apr 4 08:04:39 2022 +0200
Improved: Prevent Freemarker interpolation in fields (OFBIZ-12594)
OFBIZ-12587 is a definitive solution to prevent any kind of Freemarker exploits.
But it's hard to realise because OFBiz exposes objects, like attributes from the
Servlet scopes. So in the meantime preventing Freemarker interpolation in fields
is a pragmatic solution.
This is an improvement but needs to be backported because it kinda affects
security
---
.../ofbiz/product/category/SeoContextFilter.java | 7 +-
framework/security/config/security.properties | 4 ++
.../org/apache/ofbiz/security/SecurityUtil.java | 78 +++++++++++++++++++++-
.../apache/ofbiz/webapp/control/ControlFilter.java | 9 ++-
4 files changed, 94 insertions(+), 4 deletions(-)
diff --git a/applications/product/src/main/java/org/apache/ofbiz/product/category/SeoContextFilter.java b/applications/product/src/main/java/org/apache/ofbiz/product/category/SeoContextFilter.java
index bf376cf863..eee52f8e99 100644
--- a/applications/product/src/main/java/org/apache/ofbiz/product/category/SeoContextFilter.java
+++ b/applications/product/src/main/java/org/apache/ofbiz/product/category/SeoContextFilter.java
@@ -48,11 +48,12 @@ import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.StringUtil;
import org.apache.ofbiz.base.util.UtilHttp;
import org.apache.ofbiz.base.util.UtilValidate;
+import org.apache.ofbiz.security.SecurityUtil;
+import org.apache.ofbiz.webapp.SeoConfigUtil;
import org.apache.ofbiz.webapp.control.ConfigXMLReader;
import org.apache.ofbiz.webapp.control.ConfigXMLReader.ControllerConfig;
import org.apache.ofbiz.webapp.control.ControlFilter;
import org.apache.ofbiz.webapp.control.WebAppConfigurationException;
-import org.apache.ofbiz.webapp.SeoConfigUtil;
import org.apache.oro.text.regex.Pattern;
import org.apache.oro.text.regex.Perl5Matcher;
@@ -115,6 +116,10 @@ public final class SeoContextFilter implements Filter {
uri = uri + "?" + queryString;
}
+ if (SecurityUtil.containsFreemarkerInterpolation(httpRequest, httpResponse, uri)) {
+ return;
+ }
+
boolean forwarded = forwardUri(httpResponse, uri);
if (forwarded) {
return;
diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties
index 03c6804e89..f546a4c595 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -281,3 +281,7 @@ allowedProtocols=localhost,127.0.0.1
#-- By default (OOTB) OFBiz is protected against Large File Denial of Service because build.gradle defines -Xmx1024M
#-- So you can at most upload a file around 500MB (see OFBIZ-11534 for more info)
#-- If you need to upload larger files then follow https://nightlies.apache.org/ofbiz/trunk/readme/html5/#passing-jvm-runtime-options-to-ofbiz
+
+#-- Prevent Freemarker exploits
+#-- eg: allowedURIsForFreemarkerInterpolation=createTextContentCms,updateTextContentCms,...
+allowedURIsForFreemarkerInterpolation=
diff --git a/framework/security/src/main/java/org/apache/ofbiz/security/SecurityUtil.java b/framework/security/src/main/java/org/apache/ofbiz/security/SecurityUtil.java
index 6eabff180c..8bd470e987 100644
--- a/framework/security/src/main/java/org/apache/ofbiz/security/SecurityUtil.java
+++ b/framework/security/src/main/java/org/apache/ofbiz/security/SecurityUtil.java
@@ -19,13 +19,23 @@
package org.apache.ofbiz.security;
+import java.io.IOException;
+import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
+
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import org.apache.http.client.utils.URLEncodedUtils;
+import org.apache.http.message.BasicNameValuePair;
import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.StringUtil;
+import org.apache.ofbiz.base.util.UtilHttp;
import org.apache.ofbiz.base.util.UtilMisc;
+import org.apache.ofbiz.base.util.UtilProperties;
import org.apache.ofbiz.base.util.UtilValidate;
import org.apache.ofbiz.entity.Delegator;
import org.apache.ofbiz.entity.GenericEntityException;
@@ -55,7 +65,7 @@ public final class SecurityUtil {
* Return true if given userLogin possess at least one of the adminPermission
* @param delegator
* @param userLoginId
- * @return
+ * @return boolean
*/
public static boolean hasUserLoginAdminPermission(Delegator delegator, String userLoginId) {
if (UtilValidate.isEmpty(userLoginId)) return false;
@@ -78,7 +88,7 @@ public final class SecurityUtil {
* @param delegator
* @param userLoginId
* @param toUserLoginId
- * @return
+ * @return List<String>
*/
public static List<String> hasUserLoginMorePermissionThan(Delegator delegator, String userLoginId, String toUserLoginId) {
ArrayList<String> returnList = new ArrayList<>();
@@ -161,4 +171,68 @@ public final class SecurityUtil {
}
return false;
}
+
+ /*
+ * Prevents Freemarker exploits
+ * @param req
+ * @param resp
+ * @param uri
+ * @throws IOException
+ */
+ public static boolean containsFreemarkerInterpolation(HttpServletRequest req, HttpServletResponse resp, String uri)
+ throws IOException {
+ String urisOkForFreemarker = UtilProperties.getPropertyValue("security", "allowedURIsForFreemarkerInterpolation");
+ List<String> urisOK = UtilValidate.isNotEmpty(urisOkForFreemarker) ? StringUtil.split(urisOkForFreemarker, ",")
+ : new ArrayList<>();
+ String uriEnd = uri.substring(uri.lastIndexOf("/") + 1, uri.length());
+
+ if (!urisOK.contains(uriEnd)) {
+ Map<String, String[]> parameterMap = req.getParameterMap();
+ if (uri.contains("ecomseo")) { // SeoContextFilter call
+ if (containsFreemarkerInterpolation(resp, uri)) {
+ return true;
+ }
+ } else if (!parameterMap.isEmpty()) { // ControlFilter call
+ List<BasicNameValuePair> params = new ArrayList<>();
+ parameterMap.forEach((name, values) -> {
+ for (String value : values) {
+ params.add(new BasicNameValuePair(name, value));
+ }
+ });
+ String queryString = URLEncodedUtils.format(params, Charset.forName("UTF-8"));
+ uri = uri + "?" + queryString;
+ if (SecurityUtil.containsFreemarkerInterpolation(resp, uri)) {
+ return true;
+ }
+ } else if (!UtilHttp.getAttributeMap(req).isEmpty()) { // Call with Content-Type modified by a MITM attack (rare case)
+ String attributeMap = UtilHttp.getAttributeMap(req).toString();
+ if (containsFreemarkerInterpolation(resp, attributeMap)) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
+ /**
+ * @param resp
+ * @param stringToCheck
+ * @throws IOException
+ */
+ public static boolean containsFreemarkerInterpolation(HttpServletResponse resp, String stringToCheck) throws IOException {
+ if (stringToCheck.contains("%24%7B") || stringToCheck.contains("${")
+ || stringToCheck.contains("%3C%23") || stringToCheck.contains("<#")
+ || stringToCheck.contains("%23%7B") || stringToCheck.contains("#{")
+ || stringToCheck.contains("%5B%3D") || stringToCheck.contains("[=")
+ || stringToCheck.contains("%5B%23") || stringToCheck.contains("[#")) { // not used OOTB in OFBiz, but possible
+
+ Debug.logError("===== Not saved for security reason, strings '${', '<#', '#{', '[=' or '[#' not accepted in fields! =====",
+ MODULE);
+ resp.sendError(HttpServletResponse.SC_FORBIDDEN,
+ "Not saved for security reason, strings '${', '<#', '#{', '[=' or '[#' not accepted in fields!");
+ return true;
+ } else {
+ return false;
+ }
+ }
}
diff --git a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
index bf9551f616..8c76628225 100644
--- a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
+++ b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
@@ -36,6 +36,8 @@ import org.apache.commons.lang.BooleanUtils;
import org.apache.commons.validator.routines.UrlValidator;
import org.apache.logging.log4j.ThreadContext;
import org.apache.ofbiz.base.util.Debug;
+import org.apache.ofbiz.entity.GenericValue;
+import org.apache.ofbiz.security.SecurityUtil;
/**
* A Filter used to specify an allowlist of allowed paths to the OFBiz application.
@@ -147,6 +149,11 @@ public class ControlFilter extends HttpFilter {
String uriWithContext = req.getRequestURI();
String uri = uriWithContext.substring(context.length());
+ if (!GenericValue.getStackTraceAsString().contains("ControlFilterTests")
+ && SecurityUtil.containsFreemarkerInterpolation(req, resp, uri)) {
+ return;
+ }
+
// Check if the requested URI is allowed.
if (allowedPaths.stream().anyMatch(uri::startsWith)) {
try {
@@ -165,7 +172,7 @@ public class ControlFilter extends HttpFilter {
}
if (Debug.infoOn()) {
Debug.logInfo("[Filtered request]: " + uriWithContext + " --> "
- + (redirectPath == null ? errorCode : redirectPath), MODULE);
+ + (redirectPath == null ? errorCode : redirectPath), MODULE);
}
}
}