Skip to content

Commit

Permalink
Improved: Improve ViewHandler interface (OFBIZ-13179) (#858)
Browse files Browse the repository at this point in the history
We extend *AbstractViewHandler* with a new method to override *prepareViewContext*.
For each view handler implementation this will allow to control context used for rendering, applying Scriptlet token detection for security purpose.

A new class *SecuredFreemarker* has been created to manage freemarker specific controls, outside global *SecurityUtil* class.

We also add a new parameter *secure-context* (set true by default) to view-map xml element to indicate that this view allow unsecure rendering, this implies the view-map to required authentication.

Thanks to Gil Portenseigne for help
  • Loading branch information
nmalin committed Nov 28, 2024
1 parent bf95ae1 commit cadcbec
Show file tree
Hide file tree
Showing 16 changed files with 314 additions and 169 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ public static String cms(HttpServletRequest request, HttpServletResponse respons
if (statusCode == HttpServletResponse.SC_OK || hasErrorPage) {
// create the template map
MapStack<String> templateMap = MapStack.create();
ScreenRenderer.populateContextForRequest(templateMap, null, request, response, servletContext);
ScreenRenderer.populateContextForRequest(templateMap, null, request, response, servletContext, true);
templateMap.put("statusCode", statusCode);

// make the link prefix
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.nio.ByteBuffer;
import java.sql.Timestamp;
import java.text.ParseException;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
Expand All @@ -44,9 +45,11 @@
import org.apache.ofbiz.entity.GenericValue;
import org.apache.ofbiz.entity.util.EntityQuery;
import org.apache.ofbiz.entity.util.EntityUtilProperties;
import org.apache.ofbiz.security.SecuredFreemarker;
import org.apache.ofbiz.service.GenericServiceException;
import org.apache.ofbiz.service.LocalDispatcher;
import org.apache.ofbiz.service.ServiceUtil;
import org.apache.ofbiz.webapp.control.ConfigXMLReader;
import org.apache.ofbiz.webapp.view.AbstractViewHandler;
import org.apache.ofbiz.webapp.view.ViewHandlerException;
import org.apache.ofbiz.webapp.website.WebSiteWorker;
Expand All @@ -62,24 +65,36 @@ public void init(ServletContext context) throws ViewHandlerException {
rootDir = context.getRealPath("/");
https = (String) context.getAttribute("https");
}

@Override
public Map<String, Object> prepareViewContext(HttpServletRequest request, HttpServletResponse response, ConfigXMLReader.ViewMap viewMap) {
List<String> fields = List.of("contentId", "rootContentId", "mapKey",
"contentAssocTypeId", "fromDate", "dataResourceId",
"contentRevisionSeqId", "mimeTypeId");
Map<String, Object> context = new HashMap<>();
fields.forEach(field -> context.put(field, request.getParameter(field)));
return viewMap.isSecureContext()
? SecuredFreemarker.sanitizeParameterMap(context)
: context;
}

/**
* @see org.apache.ofbiz.webapp.view.ViewHandler#render(java.lang.String, java.lang.String, java.lang.String, java.lang.String,
* java.lang.String, javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse)
* @see org.apache.ofbiz.webapp.view.ViewHandler#render(String, String, String, String, String, HttpServletRequest, HttpServletResponse, Map)
*/
@Override
public void render(String name, String page, String info, String contentType, String encoding, HttpServletRequest request,
HttpServletResponse response) throws ViewHandlerException {
HttpServletResponse response, Map<String, Object> context) throws ViewHandlerException {
LocalDispatcher dispatcher = (LocalDispatcher) request.getAttribute("dispatcher");
HttpSession session = request.getSession();
GenericValue userLogin = (GenericValue) session.getAttribute("userLogin");
String contentId = request.getParameter("contentId");
String rootContentId = request.getParameter("rootContentId");
String mapKey = request.getParameter("mapKey");
String contentAssocTypeId = request.getParameter("contentAssocTypeId");
String fromDateStr = request.getParameter("fromDate");
String dataResourceId = request.getParameter("dataResourceId");
String contentRevisionSeqId = request.getParameter("contentRevisionSeqId");
String mimeTypeId = request.getParameter("mimeTypeId");
String contentId = (String) context.get("contentId");
String rootContentId = (String) context.get("rootContentId");
String mapKey = (String) context.get("mapKey");
String contentAssocTypeId = (String) context.get("contentAssocTypeId");
String fromDateStr = (String) context.get("fromDate");
String dataResourceId = (String) context.get("dataResourceId");
String contentRevisionSeqId = (String) context.get("contentRevisionSeqId");
String mimeTypeId = (String) context.get("mimeTypeId");
Locale locale = UtilHttp.getLocale(request);
String webSiteId = WebSiteWorker.getWebSiteId(request);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +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.security.SecuredFreemarker;
import org.apache.ofbiz.webapp.SeoConfigUtil;
import org.apache.ofbiz.webapp.control.ConfigXMLReader;
import org.apache.ofbiz.webapp.control.ConfigXMLReader.ControllerConfig;
Expand Down Expand Up @@ -116,7 +116,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
uri = uri + "?" + queryString;
}

if (SecurityUtil.containsFreemarkerInterpolation(httpRequest, httpResponse, uri)) {
if (SecuredFreemarker.containsFreemarkerInterpolation(httpRequest, httpResponse, uri)) {
return;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
/*******************************************************************************
* 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
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*******************************************************************************/
package org.apache.ofbiz.security;

import java.io.IOException;
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
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.UtilProperties;
import org.apache.ofbiz.base.util.UtilValidate;

public class SecuredFreemarker {
private static final String MODULE = SecuredFreemarker.class.getName();
private static final List<String> FTL_INTERPOLATION = List.of("%24%7B", "${", "%3C%23", "<#", "%23%7B", "#{", "%5B%3D", "[=", "%5B%23", "[#");

/*
* 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 (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 (containsFreemarkerInterpolation(stringToCheck)) { // 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;
}
return false;
}

/**
* Analyze if stringToCheck contains a freemarker template
* @param stringToCheck
* @return true if freemarker template is detected
*/
public static boolean containsFreemarkerInterpolation(String stringToCheck) {
return UtilValidate.isNotEmpty(stringToCheck)
&& FTL_INTERPOLATION.stream().anyMatch(stringToCheck::contains);
}

/**
* Analyse each entry contains on params. If a freemarker template is detected, sanatize it to escape any exploit
* @param params
* @return Map with all values sanitized
*/
public static Map<String, Object> sanitizeParameterMap(Map<String, Object> params) {
List<Map.Entry<String, Object>> unsafeEntries = params.entrySet().stream()
.filter(entry -> entry.getValue() instanceof String
&& containsFreemarkerInterpolation((String) entry.getValue()))
.toList();
if (!unsafeEntries.isEmpty()) {
Map<String, Object> paramsSanitize = new HashMap<>(params);
unsafeEntries.forEach(entry -> {
String sanitazedValue = (String) entry.getValue();
for (String interpolation : FTL_INTERPOLATION) {
sanitazedValue = sanitazedValue.replace(interpolation, "##");
}
paramsSanitize.put(entry.getKey(), sanitazedValue);
});
return paramsSanitize;
}
return params;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,14 @@
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;
Expand Down Expand Up @@ -172,67 +163,4 @@ 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;
}
}
}
10 changes: 10 additions & 0 deletions framework/webapp/dtd/site-conf.xsd
Original file line number Diff line number Diff line change
Expand Up @@ -784,6 +784,16 @@ under the License.
</xs:documentation>
</xs:annotation>
</xs:attribute>
<xs:attribute type="xs:boolean" name="secure-context" default="true">
<xs:annotation>
<xs:documentation>
If secure-context=false, we authorize to forward to the renderer some parameters with interpretable code.
This raise a potential risk of code injection. This can be usefully for some administration page to
configure templating (like ftl for email or dynamic screen template).
For this reason if secure-context=false, auth is set as true.
</xs:documentation>
</xs:annotation>
</xs:attribute>
<xs:attribute name="x-frame-options" default="sameorigin">
<xs:annotation>
<xs:documentation>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1044,6 +1044,7 @@ public static class ViewMap {
private String strictTransportSecurity;
private String description;
private boolean noCache = false;
private boolean secureContext = true;
private boolean securityAuth = false;

/**
Expand Down Expand Up @@ -1105,6 +1106,15 @@ public boolean isNoCache() {
return noCache;
}

/**
* Is secureContext boolean.
*
* @return the boolean
*/
public boolean isSecureContext() {
return secureContext;
}

/**
* Gets type.
* @return the type
Expand Down Expand Up @@ -1144,7 +1154,8 @@ public ViewMap(Element viewMapElement) {
this.info = viewMapElement.getAttribute("info");
this.contentType = viewMapElement.getAttribute("content-type");
this.noCache = "true".equals(viewMapElement.getAttribute("no-cache"));
this.securityAuth = "true".equals(viewMapElement.getAttribute("auth"));
this.secureContext = "true".equals(viewMapElement.getAttribute("secure-context"));
this.securityAuth = "true".equals(viewMapElement.getAttribute("auth")) || !this.secureContext;
this.encoding = viewMapElement.getAttribute("encoding");
this.xFrameOption = viewMapElement.getAttribute("x-frame-options");
this.strictTransportSecurity = viewMapElement.getAttribute("strict-transport-security");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,8 @@
import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.UtilValidate;
import org.apache.ofbiz.entity.GenericValue;
import org.apache.ofbiz.security.SecuredFreemarker;
import org.apache.ofbiz.security.SecuredUpload;
import org.apache.ofbiz.security.SecurityUtil;




/**
* A Filter used to specify an allowlist of allowed paths to the OFBiz application.
Expand Down Expand Up @@ -163,7 +160,7 @@ public void doFilter(HttpServletRequest req, HttpServletResponse resp, FilterCha

GenericValue userLogin = (GenericValue) session.getAttribute("userLogin");
if (!LoginWorker.hasBasePermission(userLogin, req)) { // Allows UEL and FlexibleString (OFBIZ-12602)
if (isSolrTest() && SecurityUtil.containsFreemarkerInterpolation(req, resp, uri)) {
if (isSolrTest() && SecuredFreemarker.containsFreemarkerInterpolation(req, resp, uri)) {
return;
}
}
Expand Down
Loading

0 comments on commit cadcbec

Please sign in to comment.