This is an automated email from the ASF dual-hosted git repository.
mthl pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 889769593e1fedbf9bd9092550031c72e021d1cf
Author: Samuel Trégouët <samuel.trego...@nereide.fr>
AuthorDate: Mon Nov 4 22:42:02 2019 +0100
Improved: Remove ‘ServiceEventHandler#checkSecureParameter’
(OFBIZ-11260)
There is no security reason justifying to prevent the invocation of
service event handlers with URI parameters if we accept request body
parameters. As a consequence it is preferable to unconditionally
accept URI parameters and remove the
‘service.http.parameters.require.encrypted’ configuration property.
---
build.gradle | 2 +-
framework/webapp/config/url.properties | 4 --
.../ofbiz/webapp/event/ServiceEventHandler.java | 57 ----------------------
.../webapp/event/ServiceMultiEventHandler.java | 5 --
4 files changed, 1 insertion(+), 67 deletions(-)
diff --git a/build.gradle b/build.gradle
index cfcceb1..3259468 100644
--- a/build.gradle
+++ b/build.gradle
@@ -307,7 +307,7 @@ checkstyle {
// the sum of errors that were present before introducing the
// ‘checkstyle’ tool present in the framework and in the official
// plugins.
- tasks.checkstyleMain.maxErrors = 37781
+ tasks.checkstyleMain.maxErrors = 37780
// Currently there are a lot of errors so we need to temporarily
// hide them to avoid polluting the terminal output.
showViolations = false
diff --git a/framework/webapp/config/url.properties
b/framework/webapp/config/url.properties
index 1b6923d..8daf72c 100644
--- a/framework/webapp/config/url.properties
+++ b/framework/webapp/config/url.properties
@@ -44,7 +44,3 @@ cookie.domain=
# Exclude jsessionid for User-Agents (separated by comma's)
link.remove_lsessionid.user_agent_list =
googlebot,yahoo,msnbot,mediapartners-google
-
-# Should HTTP parameters sent to services require encryption?
-# This is generally advised for more secure webapps as it makes it more
difficult to spoof requests (XSRF) that change data.
-service.http.parameters.require.encrypted=Y
diff --git
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceEventHandler.java
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceEventHandler.java
index 0e28340..0b6040b 100644
---
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceEventHandler.java
+++
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceEventHandler.java
@@ -23,7 +23,6 @@ import java.util.LinkedList;
import java.util.List;
import java.util.Locale;
import java.util.Map;
-import java.util.Set;
import java.util.TimeZone;
import javax.servlet.ServletContext;
@@ -35,9 +34,7 @@ import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.UtilGenerics;
import org.apache.ofbiz.base.util.UtilHttp;
import org.apache.ofbiz.base.util.UtilValidate;
-import org.apache.ofbiz.entity.Delegator;
import org.apache.ofbiz.entity.GenericValue;
-import org.apache.ofbiz.entity.util.EntityUtilProperties;
import org.apache.ofbiz.service.DispatchContext;
import org.apache.ofbiz.service.GenericServiceException;
import org.apache.ofbiz.service.LocalDispatcher;
@@ -47,7 +44,6 @@ import org.apache.ofbiz.service.ServiceAuthException;
import org.apache.ofbiz.service.ServiceValidationException;
import org.apache.ofbiz.webapp.control.ConfigXMLReader.Event;
import org.apache.ofbiz.webapp.control.ConfigXMLReader.RequestMap;
-import org.apache.ofbiz.webapp.control.ControlActivationEventListener;
import org.apache.ofbiz.widget.renderer.VisualTheme;
/**
@@ -124,8 +120,6 @@ public class ServiceEventHandler implements EventHandler {
Map<String, Object> rawParametersMap =
UtilHttp.getCombinedMap(request);
Map<String, Object> multiPartMap =
UtilGenerics.cast(request.getAttribute("multiPartMap"));
- Set<String> urlOnlyParameterNames =
UtilHttp.getUrlOnlyParameterMap(request).keySet();
-
// we have a service and the model; build the context
Map<String, Object> serviceContext = new HashMap<>();
for (ModelParam modelParam: model.getInModelParamList()) {
@@ -176,9 +170,6 @@ public class ServiceEventHandler implements EventHandler {
// check the request parameters
if (UtilValidate.isEmpty(value)) {
- ServiceEventHandler.checkSecureParameter(requestMap,
urlOnlyParameterNames, name, session,
- serviceName, dctx.getDelegator());
-
// if the service modelParam has allow-html="any" then get
this direct from the
// request instead of in the parameters Map so there will
be no canonicalization
// possibly messing things up
@@ -321,52 +312,4 @@ public class ServiceEventHandler implements EventHandler {
return responseString;
}
- public static void checkSecureParameter(RequestMap requestMap, Set<String>
urlOnlyParameterNames, String name,
- HttpSession session, String serviceName, Delegator delegator)
throws EventHandlerException {
- // special case for security: if this is a request-map defined as
secure in
- // controller.xml then only accept body parameters coming in, ie don't
allow the
- // insecure URL parameters
- // NOTE: the RequestHandler will check the HttpSerletRequest security
to make
- // sure it is secure if the request-map -> security -> https=true,
- // but we can't just look at the request.isSecure() method here
because it is
- // allowed to send secure requests for request-map with https=false
- if (requestMap != null && requestMap.securityHttps) {
- if (urlOnlyParameterNames.contains(name)) {
- String errMsg = "Found URL parameter [" + name + "] passed to
secure (https) request-map with uri ["
- + requestMap.uri + "] with an event that calls service
[" + serviceName
- + "]; this is not allowed for security reasons! The
data should be encrypted by making it "
- + "part of the request body " + "(a form field)
instead of the request URL."
- + " Moreover it would be kind if you could create a
Jira sub-task of "
- + "https://issues.apache.org/jira/browse/OFBIZ-2330 "
- + "(check before if a sub-task for this error does not
exist)."
- + " If you are not sure how to create a Jira issue
please have a look before at "
- +
"https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices";
- + " Thank you in advance for your help.";
- Debug.logError("=============== " + errMsg + "; In session ["
- + ControlActivationEventListener.showSessionId(session)
- + "]; Note that this can be changed using the
service.http.parameters.require.encrypted "
- + "property in the url.properties file",
- module);
-
- // the default here is true, so anything but N/n is true
- boolean requireEncryptedServiceWebParameters =
!EntityUtilProperties.propertyValueEqualsIgnoreCase(
- "url", "service.http.parameters.require.encrypted",
"N", delegator);
-
- // NOTE: this forces service call event parameters to be in
the body and not in
- // the URL! can be issues with existing links, like Delete
links or whatever,
- // and those need to be changed to forms!
- if (requireEncryptedServiceWebParameters) {
- throw new EventHandlerException(errMsg);
- }
- }
- // NOTTODO: may want to allow parameters that map to entity PK
fields to be in
- // the URL, but that might be a big security hole since there are
certain
- // security sensitive entities that are made of only PK fields, or
that only
- // need PK fields to function (like UserLoginSecurityGroup)
- // NOTTODO: we could allow URL parameters when it is not a POST
(ie when
- // !request.getMethod().equalsIgnoreCase("POST")), but that would
open a
- // security hole where sensitive parameters can be passed on the
URL in a
- // GET/etc and bypass this security constraint
- }
- }
}
diff --git
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceMultiEventHandler.java
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceMultiEventHandler.java
index 8fb56f3..55c8929 100644
---
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceMultiEventHandler.java
+++
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceMultiEventHandler.java
@@ -24,7 +24,6 @@ import java.util.LinkedList;
import java.util.List;
import java.util.Locale;
import java.util.Map;
-import java.util.Set;
import java.util.TimeZone;
import javax.servlet.ServletContext;
@@ -169,8 +168,6 @@ public class ServiceMultiEventHandler implements
EventHandler {
throw new EventHandlerException(e);
}
- Set<String> urlOnlyParameterNames =
UtilHttp.getUrlOnlyParameterMap(request).keySet();
-
// big try/finally to make sure commit or rollback are run
boolean beganTrans = false;
String returnString = null;
@@ -230,8 +227,6 @@ public class ServiceMultiEventHandler implements
EventHandler {
if (value == null) {
String name = paramName + curSuffix;
-
ServiceEventHandler.checkSecureParameter(requestMap, urlOnlyParameterNames,
name, session, serviceName, dctx.getDelegator());
-
String[] paramArr =
request.getParameterValues(name);
if (paramArr != null) {
if (paramArr.length > 1) {
