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 <[email protected]>
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=
