Improved: Prevent Freemarker interpolation in fields (OFBIZ-12594)

JacquesLeRoux · JacquesLeRoux · commit 843b1c7e71aa · 2022-04-04T09:15:47.000+02:00
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

Conflicts handled by hand
  SeoContextFilter.java
  ControlFilter.java

When I worked with Mathieu I did not measure how it will be hard sometimes to
backport later :/
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
@@ -48,6 +48,7 @@
 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.control.ConfigXMLReader;
 import org.apache.ofbiz.webapp.control.ConfigXMLReader.ControllerConfig;
 import org.apache.ofbiz.webapp.control.ControlFilter;
@@ -112,7 +113,11 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
             String queryString = URLEncodedUtils.format(params, Charset.forName("UTF-8"));
             uri = uri + "?" + queryString;
         }
-        
+
+        if (SecurityUtil.containsFreemarkerInterpolation(httpRequest, httpResponse, uri)) {
+            return;
+        }
+
         boolean forwarded = forwardUri(httpResponse, uri);
         if (forwarded) {
             return;
@@ -167,7 +172,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
             if (pathItemList != null) {
                 viewName = pathItemList.get(0);
             }
-            
+
             String requestUri = UtilHttp.getRequestUriFromTarget(httpRequest.getRequestURI());
 
             // check to make sure the requested url is allowed
@@ -231,7 +236,6 @@ public void destroy() {
 
     /**
      * Forward a uri according to forward pattern regular expressions. Note: this is developed for Filter usage.
-     * 
      * @param uri String to reverse transform
      * @return String
      */
diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties
@@ -253,3 +253,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
@@ -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;
@@ -56,7 +66,7 @@ public final class SecurityUtil {
      *
      * @param delegator
      * @param userLoginId
-     * @return
+     * @return boolean
      */
     public static boolean hasUserLoginAdminPermission(Delegator delegator, String userLoginId) {
         if (UtilValidate.isEmpty(userLoginId)) return false;
@@ -80,7 +90,7 @@ public static boolean hasUserLoginAdminPermission(Delegator delegator, String us
      * @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<>();
@@ -165,4 +175,68 @@ public static boolean authenticateUserLoginByJWT(Delegator delegator, String use
         }
         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
@@ -32,6 +32,8 @@
 import javax.servlet.http.HttpServletResponse;
 
 import org.apache.ofbiz.base.util.Debug;
+import org.apache.ofbiz.entity.GenericValue;
+import org.apache.ofbiz.security.SecurityUtil;
 
 /*
  * A Filter used to specify a whitelist of allowed paths to the OFBiz application.
@@ -132,6 +134,11 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
             if (offset == -1) {
                 offset = requestUri.length();
             }
+            if (!GenericValue.getStackTraceAsString().contains("ControlFilterTests")
+                    && SecurityUtil.containsFreemarkerInterpolation(httpRequest, httpResponse, requestUri)) {
+                return;
+            }
+
             while (!allowedPaths.contains(requestUri.substring(0, offset))) {
                 offset = requestUri.indexOf("/", offset + 1);
                 if (offset == -1) {
