This is an automated email from the ASF dual-hosted git repository. nmalin pushed a commit to branch trunk in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/trunk by this push: new b8ab904b4c Improved: Improve validation method on service parameter (OFBIZ-13197) (#869) b8ab904b4c is described below commit b8ab904b4c84acc9af36522cd6ab6c5ac9622932 Author: Nicolas Malin <nicolas.ma...@nereide.fr> AuthorDate: Mon Jan 6 18:04:48 2025 +0100 Improved: Improve validation method on service parameter (OFBIZ-13197) (#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. --- applications/content/servicedef/services_data.xml | 10 ++ .../org/apache/ofbiz/security/SecuredUpload.java | 11 +++ .../ofbiz/service/engine/GroovyBaseScript.groovy | 21 ---- .../org/apache/ofbiz/service/ModelService.java | 106 ++++++++++----------- .../apache/ofbiz/service/ServiceDispatcher.java | 6 +- .../apache/ofbiz/service/ModelServiceTest.groovy | 72 +++++++++----- 6 files changed, 124 insertions(+), 102 deletions(-) diff --git a/applications/content/servicedef/services_data.xml b/applications/content/servicedef/services_data.xml index c02b4a2123..c6a22a5f56 100644 --- a/applications/content/servicedef/services_data.xml +++ b/applications/content/servicedef/services_data.xml @@ -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"> diff --git a/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java b/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java index b1a06a76f7..8c88d46278 100644 --- a/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java +++ b/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java @@ -236,6 +236,17 @@ public class SecuredUpload { 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 diff --git a/framework/service/src/main/groovy/org/apache/ofbiz/service/engine/GroovyBaseScript.groovy b/framework/service/src/main/groovy/org/apache/ofbiz/service/engine/GroovyBaseScript.groovy index 6f10dbbe62..83ff751e47 100644 --- a/framework/service/src/main/groovy/org/apache/ofbiz/service/engine/GroovyBaseScript.groovy +++ b/framework/service/src/main/groovy/org/apache/ofbiz/service/engine/GroovyBaseScript.groovy @@ -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)) { diff --git a/framework/service/src/main/java/org/apache/ofbiz/service/ModelService.java b/framework/service/src/main/java/org/apache/ofbiz/service/ModelService.java index 8b8ae0bf58..621b45576c 100644 --- a/framework/service/src/main/java/org/apache/ofbiz/service/ModelService.java +++ b/framework/service/src/main/java/org/apache/ofbiz/service/ModelService.java @@ -24,6 +24,7 @@ import java.lang.reflect.Method; 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; @@ -33,6 +34,7 @@ import java.util.List; 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; @@ -1112,23 +1114,27 @@ public class ModelService extends AbstractMap<String, Object> implements Seriali /** * 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)) { @@ -1160,8 +1166,8 @@ public class ModelService extends AbstractMap<String, Object> implements Seriali + 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; @@ -1334,13 +1340,15 @@ public class ModelService extends AbstractMap<String, Object> implements Seriali /** * 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; } @@ -1349,12 +1357,15 @@ public class ModelService extends AbstractMap<String, Object> implements Seriali /** * 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); } @@ -1399,7 +1410,7 @@ public class ModelService extends AbstractMap<String, Object> implements Seriali 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 + "]"; @@ -1443,14 +1454,14 @@ public class ModelService extends AbstractMap<String, Object> implements Seriali 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); } } } @@ -1458,7 +1469,7 @@ public class ModelService extends AbstractMap<String, Object> implements Seriali } } - 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 { @@ -1471,45 +1482,28 @@ public class ModelService extends AbstractMap<String, Object> implements Seriali 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"); diff --git a/framework/service/src/main/java/org/apache/ofbiz/service/ServiceDispatcher.java b/framework/service/src/main/java/org/apache/ofbiz/service/ServiceDispatcher.java index 4e4728c1f5..e4acccf470 100644 --- a/framework/service/src/main/java/org/apache/ofbiz/service/ServiceDispatcher.java +++ b/framework/service/src/main/java/org/apache/ofbiz/service/ServiceDispatcher.java @@ -405,7 +405,7 @@ public final class ServiceDispatcher { 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); @@ -520,7 +520,7 @@ public final class ServiceDispatcher { } 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() @@ -755,7 +755,7 @@ public final class ServiceDispatcher { // 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); diff --git a/framework/service/src/test/groovy/org/apache/ofbiz/service/ModelServiceTest.groovy b/framework/service/src/test/groovy/org/apache/ofbiz/service/ModelServiceTest.groovy index 3955214c39..9fa8de9d6d 100644 --- a/framework/service/src/test/groovy/org/apache/ofbiz/service/ModelServiceTest.groovy +++ b/framework/service/src/test/groovy/org/apache/ofbiz/service/ModelServiceTest.groovy @@ -18,6 +18,10 @@ *******************************************************************************/ package org.apache.ofbiz.service + +import org.apache.ofbiz.entity.DelegatorFactory +import org.junit.Before + import static org.mockito.ArgumentMatchers.any import static org.mockito.Mockito.eq @@ -39,6 +43,15 @@ class ModelServiceTest { private static final UtilCache<String, Map<String, ModelService>> MODEL_SERVICE_MAP_BY_MODEL = UtilCache.createUtilCache(SERVICE_CACHE_NAME, 0, 0, false) private MockedStatic<UtilProperties> utilities + private LocalDispatcher dispatcher + + @Before + void initialize() { + System.setProperty("ofbiz.home", System.getProperty("user.dir")) + System.setProperty("derby.system.home", "./runtime/data/derby") + dispatcher = Mockito.mock(LocalDispatcher.class) + Mockito.when(dispatcher.getDelegator()).thenReturn(DelegatorFactory.getDelegator("default")) + } @BeforeEach void initMock() { @@ -61,7 +74,7 @@ class ModelServiceTest { </service>''' try { createModelService(serviceXml) - .validate([message: 'ok'], + .validate(dispatcher, [message: 'ok'], 'IN', Locale.default) } catch (ServiceValidationException ignored) { Assert.fail('Required parameters not validated') @@ -76,7 +89,7 @@ class ModelServiceTest { </service>''' try { createModelService(serviceXml) - .validate([message: 'ok'], + .validate(dispatcher, [message: 'ok'], 'IN', Locale.default) } catch (ServiceValidationException ignored) { Assert.fail('Optional parameter not validated') @@ -92,7 +105,7 @@ class ModelServiceTest { </service>''' try { createModelService(serviceXml) - .validate([message: 'ok'], + .validate(dispatcher, [message: 'ok'], 'IN', Locale.default) } catch (ServiceValidationException ignored) { Assert.fail('Optional parameter not validated') @@ -106,7 +119,7 @@ class ModelServiceTest { <attribute name="message" type="String" mode="IN"/> </service>''' createModelService(serviceXml) - .validate([message: null], + .validate(dispatcher, [message: null], 'IN', Locale.default) } @@ -118,7 +131,7 @@ class ModelServiceTest { </service>''' try { createModelService(serviceXml) - .validate([message: null], + .validate(dispatcher, [message: null], 'IN', Locale.default) } catch (ServiceValidationException ignored) { Assert.fail('Optional parameter not validated') @@ -132,7 +145,7 @@ class ModelServiceTest { <attribute name="message" type="String" mode="IN"/> </service>''' createModelService(serviceXml) - .validate([missing: 'ok'], + .validate(dispatcher, [missing: 'ok'], 'IN', Locale.default) } @@ -146,7 +159,7 @@ class ModelServiceTest { </service>''' try { createModelService(serviceXml) - .validate([header: [headerParam: 'foo']], + .validate(dispatcher, [header: [headerParam: 'foo']], 'IN', Locale.default) } catch (ServiceValidationException ignored) { Assert.fail('Paramètre complexe non identifié') @@ -163,7 +176,7 @@ class ModelServiceTest { </attribute> </service>''' createModelService(serviceXml) - .validate([header: [headerParam: 'foo']], + .validate(dispatcher, [header: [headerParam: 'foo']], 'IN', Locale.default) } @@ -178,7 +191,7 @@ class ModelServiceTest { </service>''' try { createModelService(serviceXml) - .validate([header: [headerParam: 'foo']], + .validate(dispatcher, [header: [headerParam: 'foo']], 'IN', Locale.default) } catch (ServiceValidationException ignored) { Assert.fail('Missing optional should not throw exception') @@ -196,7 +209,7 @@ class ModelServiceTest { </service>''' try { createModelService(serviceXml) - .validate([header: [headerParam: 'foo', otherParam: 'Good']], + .validate(dispatcher, [header: [headerParam: 'foo', otherParam: 'Good']], 'IN', Locale.default) } catch (ServiceValidationException ignored) { Assert.fail('Complex parameter control error') @@ -213,7 +226,7 @@ class ModelServiceTest { </attribute> </service>''' createModelService(serviceXml) - .validate([header: [headerParam: 'foo', otherParam: 'Good', unexpectedParam: 'Bad']], + .validate(dispatcher, [header: [headerParam: 'foo', otherParam: 'Good', unexpectedParam: 'Bad']], 'IN', Locale.default) } @@ -227,7 +240,7 @@ class ModelServiceTest { </attribute> </service>''' createModelService(serviceXml) - .validate([header: ['headerParam', 'otherParam']], + .validate(dispatcher, [header: ['headerParam', 'otherParam']], 'IN', Locale.default) } @@ -244,8 +257,8 @@ class ModelServiceTest { </service>''' try { createModelService(serviceXml) - .validate([header: [headerParam: [subHeaderParam: 'true'], - otherParam: 'true']], + .validate(dispatcher, [header: [headerParam: [subHeaderParam: 'true'], + otherParam: 'true']], 'IN', Locale.default) } catch (ServiceValidationException ignored) { Assert.fail('Paramètre complexe non identifié') @@ -264,8 +277,8 @@ class ModelServiceTest { </attribute> </service>''' createModelService(serviceXml) - .validate([header: [headerParam: [subHeaderParam: 'true', otherParam: 'false'], - otherParam: 'true']], + .validate(dispatcher, [header: [headerParam: [subHeaderParam: 'true', otherParam: 'false'], + otherParam: 'true']], 'IN', Locale.default) } @@ -277,8 +290,8 @@ class ModelServiceTest { </service>''' try { createModelService(serviceXml) - .validate([header: [headerParam: [subHeaderParam: 'true', otherParam: 'false'], - otherParam: 'true']], + .validate(dispatcher, [header: [headerParam: [subHeaderParam: 'true', otherParam: 'false'], + otherParam: 'true']], 'IN', Locale.default) } catch (ServiceValidationException ignored) { Assert.fail('Map should not have been analyzed') @@ -296,8 +309,8 @@ class ModelServiceTest { </service>''' try { createModelService(serviceXml) - .validate([header: [[headerParam: 'line1', otherParam: 'Good'], - [headerParam: 'line2', otherParam: 'Good']]], + .validate(dispatcher, [header: [[headerParam: 'line1', otherParam: 'Good'], + [headerParam: 'line2', otherParam: 'Good']]], 'IN', Locale.default) } catch (ServiceValidationException ignored) { Assert.fail('Complex List Parameter Error') @@ -314,7 +327,7 @@ class ModelServiceTest { </attribute> </service>''' createModelService(serviceXml) - .validate([header: [[headerParam: 'line1', otherParam: 'Good'], + .validate(dispatcher, [header: [[headerParam: 'line1', otherParam: 'Good'], [headerParam: 'line2', otherParam: 'Good', unwanted: 'Bad']]], 'IN', Locale.default) @@ -343,7 +356,7 @@ class ModelServiceTest { 'testParam': modelService]) try { - modelService.validate([header: [headerParam: 'line1', otherParam: 'Good']], 'IN', Locale.default) + modelService.validate(dispatcher, [header: [headerParam: 'line1', otherParam: 'Good']], 'IN', Locale.default) } catch (ServiceValidationException ignored) { Assert.fail('Complex implement not valid') } @@ -407,4 +420,19 @@ class ModelServiceTest { .createModelService(serviceElement, 'TEST') } + @Test + void callValidatorCreationWithDelegator() { + String serviceXml = '''<service name="testParam" engine="java" + location="org.apache.ofbiz.common.CommonServices" invoke="ping"> + <attribute name="fileNameToTest" type="String" mode="IN"> + <type-validate class="org.apache.ofbiz.security.SecuredUpload" method="isValidFileName"> + <fail-property resource="SecurityUiLabels" property="SupportedFileFormatsIncludingSvg"/> + </type-validate> + </attribute> + </service>''' + ModelService fo = createModelService(serviceXml) + ModelParam.ModelParamValidator validator = fo.getParam('fileNameToTest').getValidators().first() + assert validator + assert ModelService.typeValidate(dispatcher, validator, "myFileName") + } }