Skip to content

Commit

Permalink
Improved: Improve validation method on service parameter (OFBIZ-13197) (
Browse files Browse the repository at this point in the history 
#869)

Since the Remote Code Execution (File Upload) Vulnerability fixed by OFBIZ-11948, the class GroovyBaseScript.groovy contains a dependency with a service definition 'createAnonFile' to control the security.

This solution works but break the dependency between each component and the mandatory for a service to protect it himself.

Normally a service can secure each parameter with element type-validate unfortunately, this element can call only method with one parameter. In your case the method to validate a file upload need to have the delegator.

To solve it, we improve the element type-validate to analyze the method call for validate the attribute value and pass the delegator or dispatcher if it detected.

Like this we can move the code present on GroovyBaseScript to the service definition and offer the possibility to create more complex validate method for custom site.
  • Loading branch information
@nmalin
nmalin authored Jan 6, 2025
1 parent 1ae5a30 commit b8ab904
Show file tree
Hide file tree
Showing 6 changed files with 124 additions and 102 deletions.
10 changes: 10 additions & 0 deletions applications/content/servicedef/services_data.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,16 @@
location="org.apache.ofbiz.content.data.DataServices" invoke="createFileNoPerm" auth="false">
<description>Create a File No Permission Required</description>
<implements service="createFile"/>
<override name="dataResourceName">
<type-validate class="org.apache.ofbiz.security.SecuredUpload" method="isValidFileName">
<fail-property resource="SecurityUiLabels" property="SupportedFileFormatsIncludingSvg"/>
</type-validate>
</override>
<override name="objectInfo">
<type-validate class="org.apache.ofbiz.security.SecuredUpload" method="isValidAllFile">
<fail-property resource="SecurityUiLabels" property="SupportedFileFormatsIncludingSvg"/>
</type-validate>
</override>
</service>
<service name="updateFile" engine="java"
location="org.apache.ofbiz.content.data.DataServices" invoke="updateFile" auth="true">
Expand Down
11 changes: 11 additions & 0 deletions framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,17 @@ public static boolean isValidFileName(String fileToCheck, Delegator delegator) t
return true;
}

/**
* @param fileToCheck
* @param delegator
* @return true if the file is valid
* @throws IOException
* @throws ImageReadException
*/
public static boolean isValidAllFile(String fileToCheck, Delegator delegator) throws IOException, ImageReadException {
return isValidFile(fileToCheck, "All", delegator);
}

/**
* @param fileToCheck
* @param fileType
Expand Down
21 changes: 0 additions & 21 deletions framework/service/src/main/groovy/org/apache/ofbiz/service/engine/GroovyBaseScript.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,27 +50,6 @@ abstract class GroovyBaseScript extends Script {
inputMap.locale = inputMap.locale ?: this.binding.hasVariable('locale')
? this.binding.getVariable('locale')
: this.binding.getVariable('parameters').locale
if (serviceName == 'createAnonFile') {
String fileName = inputMap.get('dataResourceName')
String fileNameAndPath = inputMap.get('objectInfo')
File file = new File(fileNameAndPath)
if (!fileName.isEmpty()) {
// Check the file name
if (!org.apache.ofbiz.security.SecuredUpload.isValidFileName(fileName, delegator)) {
String errorMessage = UtilProperties.getMessage('SecurityUiLabels', 'SupportedFileFormatsIncludingSvg', inputMap.locale)
throw new ExecutionServiceException(errorMessage)
}
// TODO we could verify the file type (here "All") with dataResourceTypeId. Anyway it's done with isValidFile()
// We would just have a better error message
if (file.exists()) {
// Check if a webshell is not uploaded
if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(fileNameAndPath, 'All', delegator)) {
String errorMessage = UtilProperties.getMessage('SecurityUiLabels', 'SupportedFileFormatsIncludingSvg', inputMap.locale)
throw new ExecutionServiceException(errorMessage)
}
}
}
}
Map serviceContext = dctx.makeValidContext(serviceName, ModelService.IN_PARAM, inputMap)
Map result = dispatcher.runSync(serviceName, serviceContext)
if (ServiceUtil.isError(result)) {
Expand Down
106 changes: 50 additions & 56 deletions framework/service/src/main/java/org/apache/ofbiz/service/ModelService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.AbstractMap;
import java.util.AbstractSet;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Iterator;
import java.util.LinkedHashMap;
Expand All @@ -33,6 +34,7 @@
import java.util.Locale;
import java.util.Map;
import java.util.NoSuchElementException;
import java.util.Optional;
import java.util.Set;
import java.util.TimeZone;
import java.util.TreeSet;
Expand Down Expand Up @@ -1112,23 +1114,27 @@ public void updateDefaultValues(Map<String, Object> context, String mode) {

/**
* Validates a Map against the IN or OUT parameter information
* @param context the context
* @param mode Test either mode IN or mode OUT
* @param locale the actual locale to use
*
* @param dispatcher
* @param context the context
* @param mode Test either mode IN or mode OUT
* @param locale the actual locale to use
*/
public void validate(Map<String, Object> context, String mode, Locale locale) throws ServiceValidationException {
validate(this.contextParamList, context, mode, locale);
public void validate(LocalDispatcher dispatcher, Map<String, Object> context, String mode, Locale locale) throws ServiceValidationException {
validate(dispatcher, this.contextParamList, context, mode, locale);
}

/**
* Validates a Map against the IN or OUT parameter information for a given list of modelParam
* this is used for recursive validation of map and list modelParam in service definition
*
* @param dispatcher Dispatcher where come from the validation call
* @param modelParamList List of paramList to validate
* @param context the context
* @param mode Test either mode IN or mode OUT
* @param locale the actual locale to use
* @param context the context
* @param mode Test either mode IN or mode OUT
* @param locale the actual locale to use
*/
public void validate(List<ModelParam> modelParamList, Map<String, Object> context, String mode, Locale locale)
public void validate(LocalDispatcher dispatcher, List<ModelParam> modelParamList, Map<String, Object> context, String mode, Locale locale)
throws ServiceValidationException {
// do not validate results with errors
if (mode.equals(OUT_PARAM) && resultServiceContainsError(context)) {
Expand Down Expand Up @@ -1160,8 +1166,8 @@ public void validate(List<ModelParam> modelParamList, Map<String, Object> contex
+ optionalValues.size() + " / " + optionalInfo.size(), MODULE);
}
try {
validate(requiredInfo, requiredValues, true, this, mode, locale);
validate(optionalInfo, optionalValues, false, this, mode, locale);
validate(dispatcher, requiredInfo, requiredValues, true, this, mode, locale);
validate(dispatcher, optionalInfo, optionalValues, false, this, mode, locale);
} catch (ServiceValidationException e) {
Debug.logError("[ModelService.validate] : {" + name + "} : (" + mode + ") Required test error: " + e, MODULE);
throw e;
Expand Down Expand Up @@ -1334,13 +1340,15 @@ private Map<String, ModelParam> prepareParamsMap(String mode, List<ModelParam> m
/**
* Check a Map against the IN parameter information, uses the validate() method for that
* Always called with only IN_PARAM, so to be called before the service is called with the passed context
* @param context the passed context
* @param locale the actual locale to use
*
* @param dispatcher
* @param context the passed context
* @param locale the actual locale to use
* @return boolean True is the service called with these IN_PARAM is valid
*/
public boolean isValid(Map<String, Object> context, Locale locale) {
public boolean isValid(LocalDispatcher dispatcher, Map<String, Object> context, Locale locale) {
try {
validate(context, IN_PARAM, locale);
validate(dispatcher, context, IN_PARAM, locale);
} catch (ServiceValidationException e) {
return false;
}
Expand All @@ -1349,12 +1357,15 @@ public boolean isValid(Map<String, Object> context, Locale locale) {

/**
* Validates a map of name, object types to a map of name, objects
*
* @param dispatcher
* @param modelParamMap The map of name, modelParam
* @param values The map to test its value types.
* @param reverse Test the maps in reverse.
* @param values The map to test its value types.
* @param reverse Test the maps in reverse.
*/
public void validate(Map<String, ModelParam> modelParamMap, Map<String, ?> values, boolean reverse, ModelService model,
String mode, Locale locale) throws ServiceValidationException {
public void validate(LocalDispatcher dispatcher, Map<String, ModelParam> modelParamMap, Map<String, ?> values,
boolean reverse, ModelService model, String mode, Locale locale)
throws ServiceValidationException {
if (modelParamMap == null || values == null) {
throw new ServiceValidationException("Cannot validate NULL maps", model);
}
Expand Down Expand Up @@ -1399,7 +1410,7 @@ public void validate(Map<String, ModelParam> modelParamMap, Map<String, ?> value
for (ModelParam.ModelParamValidator val: param.getValidators()) {
if (UtilValidate.isNotEmpty(val.getMethodName())) {
try {
if (!typeValidate(val, testObject)) {
if (!typeValidate(dispatcher, val, testObject)) {
String msg = val.getFailMessage(locale);
if (msg == null) {
msg = "The following parameter failed validation: [" + model.name + "." + key + "]";
Expand Down Expand Up @@ -1443,22 +1454,22 @@ public void validate(Map<String, ModelParam> modelParamMap, Map<String, ?> value
if (UtilValidate.isNotEmpty(childrenModelParams)
&& UtilValidate.isNotEmpty(values.get(paramName))) {
if (modelParamMap.get(paramName).getType().endsWith("Map")) {
validate(childrenModelParams,
UtilGenerics.cast(values.get(paramName)),
mode, locale);
validate(dispatcher,
childrenModelParams,
UtilGenerics.cast(values.get(paramName)), mode, locale);
} else if (modelParamMap.get(paramName).getType().endsWith("List")) {
List<Map<String, Object>> subParameters = UtilGenerics.cast(values.get(paramName));
if (UtilValidate.isNotEmpty(subParameters)) {
for (Map<String, Object> paramMap : subParameters) {
validate(childrenModelParams, paramMap, mode, locale);
validate(dispatcher, childrenModelParams, paramMap, mode, locale);
}
}
}
}
}
}

public static boolean typeValidate(ModelParam.ModelParamValidator vali, Object testValue) throws GeneralException {
public static boolean typeValidate(LocalDispatcher dispatcher, ModelParam.ModelParamValidator vali, Object testValue) throws GeneralException {
// find the validator class
Class<?> validatorClass = null;
try {
Expand All @@ -1471,45 +1482,28 @@ public static boolean typeValidate(ModelParam.ModelParamValidator vali, Object t
throw new GeneralException("Unable to load validation class [" + vali.getClassName() + "]");
}

boolean foundObjectParam = true;

Method validatorMethod = null;
try {
// try object type first
validatorMethod = validatorClass.getMethod(vali.getMethodName(), Object.class);
} catch (NoSuchMethodException e) {
foundObjectParam = false;
// next try string type
try {
validatorMethod = validatorClass.getMethod(vali.getMethodName(), String.class);
} catch (NoSuchMethodException e2) {
Debug.logWarning(e2, MODULE);
}
}

if (validatorMethod == null) {
Method validatorMethod;
List<Object> params = new LinkedList<>();
Optional<Method> validatorMethodOp = Arrays.stream(validatorClass.getDeclaredMethods())
.filter(method -> method.getName().equals(vali.getMethodName()))
.findFirst();
if (validatorMethodOp.isEmpty()) {
throw new GeneralException("Unable to find validation method [" + vali.getMethodName() + "] in class [" + vali.getClassName() + "]");
}

Object param;
if (!foundObjectParam) {
// convert to string
String converted;
try {
converted = (String) ObjectType.simpleTypeOrObjectConvert(testValue, "String", null, null);
} catch (GeneralException e) {
throw new GeneralException("Unable to convert parameter to String");
validatorMethod = validatorMethodOp.get();
for (Class<?> paramType: validatorMethod.getParameterTypes()) {
switch (paramType.getName()) {
case "org.apache.ofbiz.entity.Delegator" -> params.add(dispatcher.getDelegator());
case "org.apache.ofbiz.service.LocalDispatcher" -> params.add(dispatcher);
case "java.lang.String" -> params.add(ObjectType.simpleTypeOrObjectConvert(testValue, "String", null, null));
default -> params.add(vali);
}
param = converted;
} else {
// use plain object
param = testValue;
}

// run the validator
Boolean resultBool;
try {
resultBool = (Boolean) validatorMethod.invoke(null, param);
resultBool = (Boolean) validatorMethod.invoke(null, params.toArray());
} catch (ClassCastException e) {
throw new GeneralException("Validation method [" + vali.getMethodName() + "] in class [" + vali.getClassName()
+ "] did not return expected Boolean");
Expand Down
6 changes: 3 additions & 3 deletions framework/service/src/main/java/org/apache/ofbiz/service/ServiceDispatcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ public Map<String, Object> runSync(String localName, ModelService modelService,
try {
// FIXME without this line all simple test failed
context = ctx.makeValidContext(modelService.getName(), ModelService.IN_PARAM, context);
modelService.validate(context, ModelService.IN_PARAM, locale);
modelService.validate(getLocalDispatcher(localName), context, ModelService.IN_PARAM, locale);
} catch (ServiceValidationException e) {
Debug.logError(e, "Incoming context (in runSync : " + modelService.getName()
+ ") does not match expected requirements", MODULE);
Expand Down Expand Up @@ -520,7 +520,7 @@ public Map<String, Object> runSync(String localName, ModelService modelService,
}
try {
result = ctx.makeValidContext(modelService.getName(), ModelService.OUT_PARAM, result);
modelService.validate(result, ModelService.OUT_PARAM, locale);
modelService.validate(getLocalDispatcher(localName), result, ModelService.OUT_PARAM, locale);
} catch (ServiceValidationException e) {
rs.setEndStamp();
throw new GenericServiceException("Outgoing result (in runSync : " + modelService.getName()
Expand Down Expand Up @@ -755,7 +755,7 @@ public void runAsync(String localName, ModelService service, Map<String, ? exten
// validate the context
if (service.isValidate() && !isError && !isFailure) {
try {
service.validate(context, ModelService.IN_PARAM, locale);
service.validate(getLocalDispatcher(localName), context, ModelService.IN_PARAM, locale);
} catch (ServiceValidationException e) {
Debug.logError(e, "Incoming service context (in runAsync: " + service.getName()
+ ") does not match expected requirements", MODULE);
Expand Down
Loading

0 comments on commit b8ab904

Please sign in to comment.