[ofbiz-framework] branch release17.12 updated: Improved: Prevent FreeMarker Template Injection (SSTI)

This is an automated email from the ASF dual-hosted git repository.

jleroux pushed a commit to branch release17.12
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git


The following commit(s) were added to refs/heads/release17.12 by this push:
     new babd232  Improved: Prevent FreeMarker Template Injection (SSTI)
babd232 is described below

commit babd23282ee61f1b840899a3785e89da5f202131
Author: Jacques Le Roux <[hidden email]>
AuthorDate: Mon May 18 13:35:02 2020 +0200

    Improved: Prevent FreeMarker Template Injection (SSTI)
   
    (OFBIZ-11709)
   
    Some people may want to use another TemplateClassResolver than SAFER_RESOLVER
    This creates a new templateClassResolver security property and uses it in
    FreeMarkerWorker::makeConfiguration by default
   
    Conflicts all handled by hand (no merge possible)
---
 .../ofbiz/base/util/template/FreeMarkerWorker.java | 230 +++++++++++++--------
 framework/security/config/security.properties      |   7 +
 2 files changed, 153 insertions(+), 84 deletions(-)

diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/template/FreeMarkerWorker.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/template/FreeMarkerWorker.java
index 6c45127..9d6c67a 100644
--- a/framework/base/src/main/java/org/apache/ofbiz/base/util/template/FreeMarkerWorker.java
+++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/template/FreeMarkerWorker.java
@@ -1,4 +1,4 @@
-/*******************************************************************************
+/*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
  * distributed with this work for additional information
@@ -15,7 +15,7 @@
  * KIND, either express or implied.  See the License for the
  * specific language governing permissions and limitations
  * under the License.
- *******************************************************************************/
+ */
 package org.apache.ofbiz.base.util.template;
 
 import java.io.File;
@@ -23,18 +23,20 @@ import java.io.IOException;
 import java.io.Writer;
 import java.net.URL;
 import java.util.ArrayList;
-import java.util.Enumeration;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Properties;
 import java.util.Set;
 import java.util.TimeZone;
+import java.util.stream.Stream;
 
 import javax.servlet.ServletContext;
 import javax.servlet.http.HttpServletRequest;
 
+import org.apache.ofbiz.base.component.ComponentConfig;
 import org.apache.ofbiz.base.location.FlexibleLocation;
 import org.apache.ofbiz.base.util.Debug;
 import org.apache.ofbiz.base.util.StringUtil;
@@ -43,6 +45,7 @@ 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.base.util.cache.UtilCache;
+import org.apache.ofbiz.widget.model.ModelWidget;
 
 import freemarker.cache.MultiTemplateLoader;
 import freemarker.cache.StringTemplateLoader;
@@ -58,34 +61,38 @@ import freemarker.template.SimpleHash;
 import freemarker.template.SimpleScalar;
 import freemarker.template.Template;
 import freemarker.template.TemplateException;
-import freemarker.template.TemplateExceptionHandler;
 import freemarker.template.TemplateHashModel;
 import freemarker.template.TemplateModel;
 import freemarker.template.TemplateModelException;
 import freemarker.template.Version;
+import freemarker.template.utility.ClassUtil;
 
 /**
  * FreeMarkerWorker - Freemarker Template Engine Utilities.
  */
 public final class FreeMarkerWorker {
+    /** The template used to retrieved Freemarker transforms from multiple component classpaths. */
+    private static final String TRANSFORMS_PROPERTIES = "org/apache/ofbiz/%s/freemarkerTransforms.properties";
 
-    public static final String module = FreeMarkerWorker.class.getName();
+    private static final String MODULE = FreeMarkerWorker.class.getName();
 
-    public static final Version version = Configuration.VERSION_2_3_28;
+    public static final Version VERSION = Configuration.VERSION_2_3_30;
 
-    private FreeMarkerWorker () {}
+    private FreeMarkerWorker() { }
 
-    // use soft references for this so that things from Content records don't kill all of our memory, or maybe not for performance reasons... hmmm, leave to config file...
-    private static final UtilCache<String, Template> cachedTemplates = UtilCache.createUtilCache("template.ftl.general", 0, 0, false);
-    private static final BeansWrapper defaultOfbizWrapper = new BeansWrapperBuilder(version).build();
-    private static final Configuration defaultOfbizConfig = makeConfiguration(defaultOfbizWrapper);
+    // Use soft references for this so that things from Content records don't kill all of our memory,
+    // or maybe not for performance reasons... hmmm, leave to config file...
+    private static final UtilCache<String, Template> CACHED_TEMPLATES =
+            UtilCache.createUtilCache("template.ftl.general", 0, 0, false);
+    private static final BeansWrapper DEFAULT_OFBIZ_WRAPPER = new BeansWrapperBuilder(VERSION).build();
+    private static final Configuration DEFAULT_OFBIZ_CONFIG = makeConfiguration(DEFAULT_OFBIZ_WRAPPER);
 
     public static BeansWrapper getDefaultOfbizWrapper() {
-        return defaultOfbizWrapper;
+        return DEFAULT_OFBIZ_WRAPPER;
     }
 
     public static Configuration newConfiguration() {
-        return new Configuration(version);
+        return new Configuration(VERSION);
     }
 
     public static Configuration makeConfiguration(BeansWrapper wrapper) {
@@ -97,7 +104,7 @@ public final class FreeMarkerWorker {
         try {
             newConfig.setSharedVariable("EntityQuery", staticModels.get("org.apache.ofbiz.entity.util.EntityQuery"));
         } catch (TemplateModelException e) {
-            Debug.logError(e, module);
+            Debug.logError(e, MODULE);
         }
         newConfig.setLocalizedLookup(false);
         newConfig.setSharedVariable("StringUtil", new BeanModel(StringUtil.INSTANCE, wrapper));
@@ -109,48 +116,63 @@ public final class FreeMarkerWorker {
             newConfig.setAutoImports(freemarkerImports);
         }
         newConfig.setLogTemplateExceptions(false);
-        newConfig.setTemplateExceptionHandler(new FreeMarkerWorker.OFBizTemplateExceptionHandler());
+        boolean verboseTemplate = ModelWidget.widgetBoundaryCommentsEnabled(null)
+                || UtilProperties.getPropertyAsBoolean("widget", "widget.freemarker.template.verbose", false);
+        newConfig.setTemplateExceptionHandler(verboseTemplate
+                ? FreeMarkerWorker::handleTemplateExceptionVerbosily
+                : FreeMarkerWorker::handleTemplateException);
         try {
             newConfig.setSetting("datetime_format", "yyyy-MM-dd HH:mm:ss.SSS");
             newConfig.setSetting("number_format", "0.##########");
         } catch (TemplateException e) {
-            Debug.logError("Unable to set date/time and number formats in FreeMarker: " + e, module);
+            Debug.logError("Unable to set date/time and number formats in FreeMarker: " + e, MODULE);
+        }
+        String templateClassResolver = UtilProperties.getPropertyValue("security", "templateClassResolver",
+                "SAFER_RESOLVER");
+        try {
+            newConfig.setNewBuiltinClassResolver((TemplateClassResolver)
+                    ClassUtil.forName("freemarker.core.TemplateClassResolver" + templateClassResolver)
+                    .cast(templateClassResolver));
+        } catch (ClassNotFoundException e) {
+            Debug.logError("No TemplateClassResolver." + templateClassResolver, MODULE);
         }
-        newConfig.setNewBuiltinClassResolver(TemplateClassResolver.SAFER_RESOLVER);
         // Transforms properties file set up as key=transform name, property=transform class name
         ClassLoader loader = Thread.currentThread().getContextClassLoader();
-        Enumeration<URL> resources;
-        try {
-            resources = loader.getResources("freemarkerTransforms.properties");
-        } catch (IOException e) {
-            Debug.logError(e, "Could not load list of freemarkerTransforms.properties", module);
-            throw UtilMisc.initCause(new InternalError(e.getMessage()), e);
-        }
-        while (resources.hasMoreElements()) {
-            URL propertyURL = resources.nextElement();
-            Debug.logInfo("loading properties: " + propertyURL, module);
-            Properties props = UtilProperties.getProperties(propertyURL);
-            if (UtilValidate.isEmpty(props)) {
-                Debug.logError("Unable to locate properties file " + propertyURL, module);
+        transformsURL(loader).forEach(url -> {
+            Properties props = UtilProperties.getProperties(url);
+            if (props == null) {
+                Debug.logError("Unable to load properties file " + url, MODULE);
             } else {
+                Debug.logInfo("loading properties: " + url, MODULE);
                 loadTransforms(loader, props, newConfig);
             }
-        }
-
+        });
         return newConfig;
     }
 
+    /**
+     * Provides the sequence of existing {@code freemarkerTransforms.properties} files.
+     *
+     * @return a stream of resource location.
+     */
+    private static Stream<URL> transformsURL(ClassLoader loader) {
+        return ComponentConfig.components()
+                .map(cc -> String.format(TRANSFORMS_PROPERTIES, cc.getComponentName()))
+                .map(loader::getResource)
+                .filter(Objects::nonNull);
+    }
+
     private static void loadTransforms(ClassLoader loader, Properties props, Configuration config) {
         for (Object object : props.keySet()) {
             String key = (String) object;
             String className = props.getProperty(key);
             if (Debug.verboseOn()) {
-                Debug.logVerbose("Adding FTL Transform " + key + " with class " + className, module);
+                Debug.logVerbose("Adding FTL Transform " + key + " with class " + className, MODULE);
             }
             try {
-                config.setSharedVariable(key, loader.loadClass(className).newInstance());
+                config.setSharedVariable(key, loader.loadClass(className).getDeclaredConstructor().newInstance());
             } catch (Exception e) {
-                Debug.logError(e, "Could not pre-initialize dynamically loaded class: " + className + ": " + e, module);
+                Debug.logError(e, "Could not pre-initialize dynamically loaded class: " + className + ": " + e, MODULE);
             }
         }
     }
@@ -161,18 +183,22 @@ public final class FreeMarkerWorker {
      * @param context The context Map
      * @param outWriter The Writer to render to
      */
-    public static void renderTemplate(String templateLocation, Map<String, Object> context, Appendable outWriter) throws TemplateException, IOException {
+    public static void renderTemplate(String templateLocation, Map<String, Object> context, Appendable outWriter)
+            throws TemplateException, IOException {
         Template template = getTemplate(templateLocation);
         renderTemplate(template, context, outWriter);
     }
 
-    public static void renderTemplateFromString(String templateName, String templateString, Map<String, Object> context, Appendable outWriter, long lastModificationTime, boolean useCache) throws TemplateException, IOException {
+    public static void renderTemplateFromString(String templateName, String templateString,
+            Map<String, Object> context, Appendable outWriter, long lastModificationTime, boolean useCache)
+                    throws TemplateException, IOException {
         Template template = null;
         if (useCache) {
-            template = cachedTemplates.get(templateName);
+            template = CACHED_TEMPLATES.get(templateName);
         }
         if (template == null) {
-            StringTemplateLoader stringTemplateLoader = (StringTemplateLoader)((MultiTemplateLoader)defaultOfbizConfig.getTemplateLoader()).getTemplateLoader(1);
+            MultiTemplateLoader templateLoader = (MultiTemplateLoader) DEFAULT_OFBIZ_CONFIG.getTemplateLoader();
+            StringTemplateLoader stringTemplateLoader = (StringTemplateLoader) templateLoader.getTemplateLoader(1);
             Object templateSource = stringTemplateLoader.findTemplateSource(templateName);
             if (templateSource == null || stringTemplateLoader.getLastModified(templateSource) < lastModificationTime) {
                 stringTemplateLoader.putTemplate(templateName, templateString, lastModificationTime);
@@ -184,11 +210,11 @@ public final class FreeMarkerWorker {
     }
 
     public static void clearTemplateFromCache(String templateLocation) {
-        cachedTemplates.remove(templateLocation);
+        CACHED_TEMPLATES.remove(templateLocation);
         try {
-            defaultOfbizConfig.removeTemplateFromCache(templateLocation);
-        } catch(Exception e) {
-            Debug.logInfo("Template not found in Fremarker cache with name: " + templateLocation, module);
+            DEFAULT_OFBIZ_CONFIG.removeTemplateFromCache(templateLocation);
+        } catch (Exception e) {
+            Debug.logInfo("Template not found in Fremarker cache with name: " + templateLocation, MODULE);
         }
     }
 
@@ -198,7 +224,8 @@ public final class FreeMarkerWorker {
      * @param context The context Map
      * @param outWriter The Writer to render to
      */
-    public static Environment renderTemplate(Template template, Map<String, Object> context, Appendable outWriter) throws TemplateException, IOException {
+    public static Environment renderTemplate(Template template, Map<String, Object> context, Appendable outWriter)
+            throws TemplateException, IOException {
         // make sure there is no "null" string in there as FreeMarker will try to use it
         context.remove("null");
         // Since the template cache keeps a single instance of a Template that is shared among users,
@@ -242,7 +269,7 @@ public final class FreeMarkerWorker {
      * @return A <code>Configuration</code> instance.
      */
     public static Configuration getDefaultOfbizConfig() {
-        return defaultOfbizConfig;
+        return DEFAULT_OFBIZ_CONFIG;
     }
 
     /**
@@ -251,10 +278,11 @@ public final class FreeMarkerWorker {
      * @param templateLocation Location of the template - file path or URL
      */
     public static Template getTemplate(String templateLocation) throws IOException {
-        return getTemplate(templateLocation, cachedTemplates, defaultOfbizConfig);
+        return getTemplate(templateLocation, CACHED_TEMPLATES, DEFAULT_OFBIZ_CONFIG);
     }
 
-    public static Template getTemplate(String templateLocation, UtilCache<String, Template> cache, Configuration config) throws IOException {
+    public static Template getTemplate(String templateLocation, UtilCache<String, Template> cache, Configuration config)
+            throws IOException {
         Template template = cache.get(templateLocation);
         if (template == null) {
             template = config.getTemplate(templateLocation);
@@ -268,7 +296,8 @@ public final class FreeMarkerWorker {
         return getArg(args, key, templateContext);
     }
 
-    public static String getArg(Map<String, ? extends Object> args, String key, Map<String, ? extends Object> templateContext) {
+    public static String getArg(Map<String, ? extends Object> args, String key,
+            Map<String, ? extends Object> templateContext) {
         Object o = args.get(key);
         String returnVal = (String) unwrap(o);
         if (returnVal == null) {
@@ -277,7 +306,7 @@ public final class FreeMarkerWorker {
                     returnVal = (String) templateContext.get(key);
                 }
             } catch (ClassCastException e2) {
-                Debug.logInfo(e2.getMessage(), module);
+                Debug.logInfo(e2.getMessage(), MODULE);
             }
         }
         return returnVal;
@@ -303,7 +332,7 @@ public final class FreeMarkerWorker {
                 }
             }
         } catch (TemplateModelException e) {
-            Debug.logInfo(e.getMessage(), module);
+            Debug.logInfo(e.getMessage(), MODULE);
         }
         return UtilGenerics.<T>cast(obj);
     }
@@ -313,7 +342,7 @@ public final class FreeMarkerWorker {
         try {
             o = args.get(key);
         } catch (TemplateModelException e) {
-            Debug.logVerbose(e.getMessage(), module);
+            Debug.logVerbose(e.getMessage(), MODULE);
             return null;
         }
 
@@ -324,7 +353,7 @@ public final class FreeMarkerWorker {
             try {
                 ctxObj = args.get("context");
             } catch (TemplateModelException e) {
-                Debug.logInfo(e.getMessage(), module);
+                Debug.logInfo(e.getMessage(), MODULE);
                 return returnObj;
             }
             Map<String, ?> ctx = null;
@@ -338,7 +367,7 @@ public final class FreeMarkerWorker {
 
     @SuppressWarnings("unchecked")
     public static <T> T unwrap(Object o) {
-        Object returnObj = null;
+        Object returnObj;
 
         if (o == TemplateModel.NOTHING) {
             returnObj = null;
@@ -346,6 +375,8 @@ public final class FreeMarkerWorker {
             returnObj = o.toString();
         } else if (o instanceof BeanModel) {
             returnObj = ((BeanModel) o).getWrappedObject();
+        } else {
+            returnObj = null;
         }
 
         return (T) returnObj;
@@ -355,9 +386,10 @@ public final class FreeMarkerWorker {
         Map<String, Object> templateRoot = new HashMap<>();
         Set<String> varNames = null;
         try {
-            varNames = UtilGenerics.checkSet(env.getKnownVariableNames());
+            varNames = UtilGenerics.cast(env.getKnownVariableNames());
         } catch (TemplateModelException e1) {
-            Debug.logError(e1, "Error getting FreeMarker variable names, will not put pass current context on to sub-content", module);
+            String msg = "Error getting FreeMarker variable names, will not put pass current context on to sub-content";
+            Debug.logError(e1, msg, MODULE);
         }
         if (varNames != null) {
             for (String varName: varNames) {
@@ -367,26 +399,27 @@ public final class FreeMarkerWorker {
         return templateRoot;
     }
 
-    public static void saveContextValues(Map<String, Object> context, String [] saveKeyNames, Map<String, Object> saveMap) {
+    public static void saveContextValues(Map<String, Object> context, String[] saveKeyNames,
+            Map<String, Object> saveMap) {
         for (String key: saveKeyNames) {
             Object o = context.get(key);
             if (o instanceof Map<?, ?>) {
-                o = UtilMisc.makeMapWritable(UtilGenerics.checkMap(o));
+                o = UtilMisc.makeMapWritable(UtilGenerics.cast(o));
             } else if (o instanceof List<?>) {
-                o = UtilMisc.makeListWritable(UtilGenerics.checkList(o));
+                o = UtilMisc.makeListWritable(UtilGenerics.cast(o));
             }
             saveMap.put(key, o);
         }
     }
 
-    public static Map<String, Object> saveValues(Map<String, Object> context, String [] saveKeyNames) {
+    public static Map<String, Object> saveValues(Map<String, Object> context, String[] saveKeyNames) {
         Map<String, Object> saveMap = new HashMap<>();
         for (String key: saveKeyNames) {
             Object o = context.get(key);
             if (o instanceof Map<?, ?>) {
-                o = UtilMisc.makeMapWritable(UtilGenerics.checkMap(o));
+                o = UtilMisc.makeMapWritable(UtilGenerics.cast(o));
             } else if (o instanceof List<?>) {
-                o = UtilMisc.makeListWritable(UtilGenerics.checkList(o));
+                o = UtilMisc.makeListWritable(UtilGenerics.cast(o));
             }
             saveMap.put(key, o);
         }
@@ -398,10 +431,10 @@ public final class FreeMarkerWorker {
             String key = entry.getKey();
             Object o = entry.getValue();
             if (o instanceof Map<?, ?>) {
-                context.put(key, UtilMisc.makeMapWritable(UtilGenerics.checkMap(o)));
+                context.put(key, UtilMisc.makeMapWritable(UtilGenerics.cast(o)));
             } else if (o instanceof List<?>) {
                 List<Object> list = new ArrayList<>();
-                list.addAll(UtilGenerics.checkList(o));
+                list.addAll(UtilGenerics.cast(o));
                 context.put(key, list);
             } else {
                 context.put(key, o);
@@ -444,9 +477,9 @@ public final class FreeMarkerWorker {
             throw new IllegalArgumentException("Error in getSiteParameters, context/ctx cannot be null");
         }
         ServletContext servletContext = request.getSession().getServletContext();
-        String rootDir = (String)ctx.get("rootDir");
-        String webSiteId = (String)ctx.get("webSiteId");
-        String https = (String)ctx.get("https");
+        String rootDir = (String) ctx.get("rootDir");
+        String webSiteId = (String) ctx.get("webSiteId");
+        String https = (String) ctx.get("https");
         if (UtilValidate.isEmpty(rootDir)) {
             rootDir = servletContext.getRealPath("/");
             ctx.put("rootDir", rootDir);
@@ -462,13 +495,13 @@ public final class FreeMarkerWorker {
     }
 
     public static TemplateModel autoWrap(Object obj, Environment env) {
-       TemplateModel templateModelObj = null;
-       try {
-           templateModelObj = getDefaultOfbizWrapper().wrap(obj);
-       } catch (TemplateModelException e) {
-           throw new RuntimeException(e.getMessage());
-       }
-       return templateModelObj;
+        TemplateModel templateModelObj = null;
+        try {
+            templateModelObj = getDefaultOfbizWrapper().wrap(obj);
+        } catch (TemplateModelException e) {
+            throw new RuntimeException(e.getMessage());
+        }
+        return templateModelObj;
     }
 
     /*
@@ -486,23 +519,52 @@ public final class FreeMarkerWorker {
             try {
                 locationUrl = FlexibleLocation.resolveLocation(name);
             } catch (Exception e) {
-                Debug.logWarning("Unable to locate the template: " + name, module);
+                Debug.logWarning("Unable to locate the template: " + name, MODULE);
             }
-            return locationUrl != null && new File(locationUrl.getFile()).exists()? locationUrl: null;
+            return locationUrl != null && new File(locationUrl.getFile()).exists() ? locationUrl : null;
         }
     }
 
     /**
-     * OFBiz specific TemplateExceptionHandler.
+     * Handles template exceptions quietly.
+     * <p>
+     * This is done by suppressing the exception and replacing it by a generic char for quiet alert.
+     * Note that exception is still logged.
+     * <p>
+     * This implements the {@link freemarker.template.TemplateExceptionHandler} functional interface.
+     *
+     * @param te  the exception that occurred
+     * @param env  the runtime environment of the template
+     * @param out  this is where the output of the template is written
      */
-    static class OFBizTemplateExceptionHandler implements TemplateExceptionHandler {
-        public void handleTemplateException(TemplateException te, Environment env, Writer out) throws TemplateException {
-            try {
-                out.write(te.getMessage());
-                Debug.logError(te, module);
-            } catch (IOException e) {
-                Debug.logError(e, module);
-            }
+    private static void handleTemplateException(TemplateException te, Environment env, Writer out) {
+        try {
+            out.write(UtilProperties.getPropertyValue("widget", "widget.freemarker.template.exception.message", "∎"));
+            Debug.logError(te, MODULE);
+        } catch (IOException e) {
+            Debug.logError(e, MODULE);
+        }
+    }
+
+    /**
+     * Handles template exceptions verbosely.
+     * <p>
+     * This is done by suppressing the exception and keeping the rendering going on.  Messages
+     * present in the stack trace are sanitized before printing them to the output writer.
+     * Note that exception is still logged.
+     * <p>
+     * This implements the {@link freemarker.template.TemplateExceptionHandler} functional interface.
+     *
+     * @param te  the exception that occurred
+     * @param env  the runtime environment of the template
+     * @param out  this is where the output of the template is written
+     */
+    private static void handleTemplateExceptionVerbosily(TemplateException te, Environment env, Writer out) {
+        try {
+            out.write(te.getMessage());
+            Debug.logError(te, MODULE);
+        } catch (IOException e) {
+            Debug.logError(e, MODULE);
         }
     }
 }
diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties
index 2a044d6..a5f80a5 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -142,3 +142,10 @@ host-headers-allowed=localhost,127.0.0.1,demo-trunk.ofbiz.apache.org,demo-stable
 # -- By default the SameSite value in SameSiteFilter is strict. This allows to change it to lax if needed  
 SameSiteCookieAttribute=
 
+# -- Freemarker TemplateClassResolver option, see OFBIZ-11709.
+# -- By default OFBiz uses the SAFER_RESOLVER because OOTB it does not use any of the Freemarker classes
+# -- that SAFER_RESOLVER prevents: ObjectConstructor, Execute and JythonRuntime.
+# -- If you need to use one to these classes you need to change the TemplateClassResolver
+# -- to UNRESTRICTED_RESOLVER and look at MemberAccessPolicy. In any cases better read
+# -- https://freemarker.apache.org/docs/app_faq.html#faq_template_uploading_security
+templateClassResolver=