[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 c502a97  Improved: Prevent FreeMarker Template Injection (SSTI)
c502a97 is described below

commit c502a978a0138b3cc1906ddd915f0b9f50c3689c
Author: Jacques Le Roux <[hidden email]>
AuthorDate: Mon May 18 13:48:31 2020 +0200

    Improved: Prevent FreeMarker Template Injection (SSTI)
   
    (OFBIZ-11709)
   
    Fixes all the conflicts previously handled by hand (no merge was possible)
---
 .../ofbiz/base/util/template/FreeMarkerWorker.java | 230 ++++++++-------------
 1 file changed, 88 insertions(+), 142 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 9d6c67a..814031a 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,20 +23,18 @@ 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;
@@ -45,7 +43,6 @@ 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;
@@ -61,6 +58,7 @@ 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;
@@ -71,28 +69,24 @@ 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";
 
-    private static final String MODULE = FreeMarkerWorker.class.getName();
+    public static final String module = FreeMarkerWorker.class.getName();
 
-    public static final Version VERSION = Configuration.VERSION_2_3_30;
+    public static final Version version = Configuration.VERSION_2_3_28;
 
-    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> 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);
+    // 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);
 
     public static BeansWrapper getDefaultOfbizWrapper() {
-        return DEFAULT_OFBIZ_WRAPPER;
+        return defaultOfbizWrapper;
     }
 
     public static Configuration newConfiguration() {
-        return new Configuration(VERSION);
+        return new Configuration(version);
     }
 
     public static Configuration makeConfiguration(BeansWrapper wrapper) {
@@ -104,7 +98,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));
@@ -116,17 +110,6 @@ public final class FreeMarkerWorker {
             newConfig.setAutoImports(freemarkerImports);
         }
         newConfig.setLogTemplateExceptions(false);
-        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);
-        }
         String templateClassResolver = UtilProperties.getPropertyValue("security", "templateClassResolver",
                 "SAFER_RESOLVER");
         try {
@@ -134,32 +117,35 @@ public final class FreeMarkerWorker {
                     ClassUtil.forName("freemarker.core.TemplateClassResolver" + templateClassResolver)
                     .cast(templateClassResolver));
         } catch (ClassNotFoundException e) {
-            Debug.logError("No TemplateClassResolver." + templateClassResolver, MODULE);
+            Debug.logError("No TemplateClassResolver." + templateClassResolver, module);
+        }        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);
         }
+        newConfig.setNewBuiltinClassResolver(TemplateClassResolver.SAFER_RESOLVER);
         // Transforms properties file set up as key=transform name, property=transform class name
         ClassLoader loader = Thread.currentThread().getContextClassLoader();
-        transformsURL(loader).forEach(url -> {
-            Properties props = UtilProperties.getProperties(url);
-            if (props == null) {
-                Debug.logError("Unable to load properties file " + url, MODULE);
+        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);
             } 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);
+        return newConfig;
     }
 
     private static void loadTransforms(ClassLoader loader, Properties props, Configuration config) {
@@ -167,12 +153,12 @@ public final class FreeMarkerWorker {
             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).getDeclaredConstructor().newInstance());
+                config.setSharedVariable(key, loader.loadClass(className).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);
             }
         }
     }
@@ -183,22 +169,18 @@ 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 = CACHED_TEMPLATES.get(templateName);
+            template = cachedTemplates.get(templateName);
         }
         if (template == null) {
-            MultiTemplateLoader templateLoader = (MultiTemplateLoader) DEFAULT_OFBIZ_CONFIG.getTemplateLoader();
-            StringTemplateLoader stringTemplateLoader = (StringTemplateLoader) templateLoader.getTemplateLoader(1);
+            StringTemplateLoader stringTemplateLoader = (StringTemplateLoader)((MultiTemplateLoader)defaultOfbizConfig.getTemplateLoader()).getTemplateLoader(1);
             Object templateSource = stringTemplateLoader.findTemplateSource(templateName);
             if (templateSource == null || stringTemplateLoader.getLastModified(templateSource) < lastModificationTime) {
                 stringTemplateLoader.putTemplate(templateName, templateString, lastModificationTime);
@@ -210,11 +192,11 @@ public final class FreeMarkerWorker {
     }
 
     public static void clearTemplateFromCache(String templateLocation) {
-        CACHED_TEMPLATES.remove(templateLocation);
+        cachedTemplates.remove(templateLocation);
         try {
-            DEFAULT_OFBIZ_CONFIG.removeTemplateFromCache(templateLocation);
-        } catch (Exception e) {
-            Debug.logInfo("Template not found in Fremarker cache with name: " + templateLocation, MODULE);
+            defaultOfbizConfig.removeTemplateFromCache(templateLocation);
+        } catch(Exception e) {
+            Debug.logInfo("Template not found in Fremarker cache with name: " + templateLocation, module);
         }
     }
 
@@ -224,8 +206,7 @@ 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,
@@ -269,7 +250,7 @@ public final class FreeMarkerWorker {
      * @return A <code>Configuration</code> instance.
      */
     public static Configuration getDefaultOfbizConfig() {
-        return DEFAULT_OFBIZ_CONFIG;
+        return defaultOfbizConfig;
     }
 
     /**
@@ -278,11 +259,10 @@ 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, CACHED_TEMPLATES, DEFAULT_OFBIZ_CONFIG);
+        return getTemplate(templateLocation, cachedTemplates, defaultOfbizConfig);
     }
 
-    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);
@@ -296,8 +276,7 @@ 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) {
@@ -306,7 +285,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;
@@ -332,7 +311,7 @@ public final class FreeMarkerWorker {
                 }
             }
         } catch (TemplateModelException e) {
-            Debug.logInfo(e.getMessage(), MODULE);
+            Debug.logInfo(e.getMessage(), module);
         }
         return UtilGenerics.<T>cast(obj);
     }
@@ -342,7 +321,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;
         }
 
@@ -353,7 +332,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;
@@ -367,7 +346,7 @@ public final class FreeMarkerWorker {
 
     @SuppressWarnings("unchecked")
     public static <T> T unwrap(Object o) {
-        Object returnObj;
+        Object returnObj = null;
 
         if (o == TemplateModel.NOTHING) {
             returnObj = null;
@@ -375,8 +354,6 @@ public final class FreeMarkerWorker {
             returnObj = o.toString();
         } else if (o instanceof BeanModel) {
             returnObj = ((BeanModel) o).getWrappedObject();
-        } else {
-            returnObj = null;
         }
 
         return (T) returnObj;
@@ -386,10 +363,9 @@ public final class FreeMarkerWorker {
         Map<String, Object> templateRoot = new HashMap<>();
         Set<String> varNames = null;
         try {
-            varNames = UtilGenerics.cast(env.getKnownVariableNames());
+            varNames = UtilGenerics.checkSet(env.getKnownVariableNames());
         } catch (TemplateModelException e1) {
-            String msg = "Error getting FreeMarker variable names, will not put pass current context on to sub-content";
-            Debug.logError(e1, msg, MODULE);
+            Debug.logError(e1, "Error getting FreeMarker variable names, will not put pass current context on to sub-content", module);
         }
         if (varNames != null) {
             for (String varName: varNames) {
@@ -399,27 +375,26 @@ 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.cast(o));
+                o = UtilMisc.makeMapWritable(UtilGenerics.checkMap(o));
             } else if (o instanceof List<?>) {
-                o = UtilMisc.makeListWritable(UtilGenerics.cast(o));
+                o = UtilMisc.makeListWritable(UtilGenerics.checkList(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.cast(o));
+                o = UtilMisc.makeMapWritable(UtilGenerics.checkMap(o));
             } else if (o instanceof List<?>) {
-                o = UtilMisc.makeListWritable(UtilGenerics.cast(o));
+                o = UtilMisc.makeListWritable(UtilGenerics.checkList(o));
             }
             saveMap.put(key, o);
         }
@@ -431,10 +406,10 @@ public final class FreeMarkerWorker {
             String key = entry.getKey();
             Object o = entry.getValue();
             if (o instanceof Map<?, ?>) {
-                context.put(key, UtilMisc.makeMapWritable(UtilGenerics.cast(o)));
+                context.put(key, UtilMisc.makeMapWritable(UtilGenerics.checkMap(o)));
             } else if (o instanceof List<?>) {
                 List<Object> list = new ArrayList<>();
-                list.addAll(UtilGenerics.cast(o));
+                list.addAll(UtilGenerics.checkList(o));
                 context.put(key, list);
             } else {
                 context.put(key, o);
@@ -477,9 +452,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);
@@ -495,13 +470,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;
     }
 
     /*
@@ -519,52 +494,23 @@ 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;
         }
     }
 
     /**
-     * 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
+     * OFBiz specific TemplateExceptionHandler.
      */
-    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);
+    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);
+            }
         }
     }
 }