From 6ecad940ff29c55bf00d110f53a76745d9b752ba Mon Sep 17 00:00:00 2001 From: Nicolas Malin Date: Thu, 10 Oct 2024 14:07:49 +0200 Subject: [PATCH 01/13] Improved: Allow to use GroovyDsl in FlexibleStringExpander (OFBIZ-13133) Second improvement on this functionality with increase the security by analyse each script to control the presence of potential code injection. The regexp to control is a property: security.deniedScriptletsTokens. If a script match the regexp, OFBiz raise in log an alert with the script and the script hash. The script is disabled and can't run. If you have a safe script who is matched by the regexp, you can add the hash given by OFBiz on the property: security.allowedScriptletHashes --- .../apache/ofbiz/base/util/ScriptUtil.java | 67 ++++++++++++++++++- .../string/FlexibleStringExpanderTests.java | 2 + framework/security/config/security.properties | 13 ++++ 3 files changed, 81 insertions(+), 1 deletion(-) diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java index b5cf491c5c..86d9409b49 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java @@ -32,6 +32,7 @@ import java.util.Map; import java.util.Set; +import java.util.regex.Pattern; import javax.script.Bindings; import javax.script.Compilable; import javax.script.CompiledScript; @@ -44,6 +45,7 @@ import javax.script.SimpleBindings; import javax.script.SimpleScriptContext; +import org.apache.ofbiz.base.crypto.HashCrypt; import org.apache.ofbiz.base.location.FlexibleLocation; import org.apache.ofbiz.base.util.cache.UtilCache; import org.apache.ofbiz.common.scripting.ScriptHelperImpl; @@ -69,9 +71,12 @@ public final class ScriptUtil { /** The ScriptHelper key. */ public static final String SCRIPT_HELPER_KEY = "ofbiz"; private static final UtilCache PARSED_SCRIPTS = UtilCache.createUtilCache("script.ParsedScripts", 0, 0, false); + private static final UtilCache> ALLOWED_SCRIPTS = UtilCache.createUtilCache("script.allowed.Scripts", 0, 0, false); private static final Object[] EMPTY_ARGS = {}; /** A set of script names - derived from the JSR-223 scripting engines. */ public static final Set SCRIPT_NAMES; + private static final Pattern DENIEDSCRIPTLETSTOKENS = initScriptletsTokensPattern(); + private static final Boolean USEDENIEDSCRIPTLETSTOKENS = UtilProperties.getPropertyAsBoolean("security", "useDeniedScriptletsTokens", false); static { Set writableScriptNames = new HashSet<>(); @@ -237,6 +242,9 @@ public static Object evaluate(String language, String script, Class scriptCla if (scriptClass != null) { return InvokerHelper.createScript(scriptClass, GroovyUtil.getBinding(context)).run(); } + if (!isSafeScript(language, script)) { + return ""; + } try { CompiledScript compiledScript = compileScriptString(language, script); if (compiledScript != null) { @@ -388,7 +396,9 @@ public static Class parseScript(String language, String script) { Class scriptClass = null; if ("groovy".equals(language)) { try { - scriptClass = GroovyUtil.parseClass(script); + if (isSafeScript(language, script)) { + scriptClass = GroovyUtil.parseClass(script); + } } catch (IOException e) { Debug.logError(e, MODULE); return null; @@ -397,6 +407,61 @@ public static Class parseScript(String language, String script) { return scriptClass; } + /** + * Analyse if we can run the script or need to block it due to potential security issue + * @param language + * @param script + * @return true is we can run the script + * @throws IOException + */ + private static boolean isSafeScript(String language, String script) throws IOException { + HashSet allowedScript = ALLOWED_SCRIPTS.putIfAbsentAndGet(language, initAllowedScriptHashes()); + String scriptHash = HashCrypt.digestHash("SHA", script.getBytes()); + boolean canExecuteScript = allowedScript.contains(scriptHash); + if (!canExecuteScript) { + if (!checkIfScriptIsSafe(script)) { + Debug.logWarning(String.format("Tried to execute unauthorized script \n **** \n%s\n **** " + + "\nif it's safe script you can add the following hash to security.allowedScriptlets: %s", + script, scriptHash), MODULE); + return false; + } + allowedScript.add(scriptHash); + } + return true; + } + + /** + * if USEDENIEDSCRIPTLETSTOKENS is true check if content match the regexp DENIEDSCRIPTLETSTOKENS + * @param content + * @return true if the content doesn't match + * @throws IOException + */ + public static boolean checkIfScriptIsSafe(String content) throws IOException { + if (content == null || !USEDENIEDSCRIPTLETSTOKENS) { + return true; + } + return !DENIEDSCRIPTLETSTOKENS.matcher(content).find(); + } + + /** + * Load the regExp for analyse script security + * @return Pattern init by the regExp security.deniedScriptletsTokens + */ + private static Pattern initScriptletsTokensPattern() { + String deniedScriptletsTokens = UtilProperties.getPropertyValue("security", "deniedScriptletsTokens", ""); + return Pattern.compile(deniedScriptletsTokens); + } + + /** + * Load the list of script exception that we autorise to run despite the security risk + * @return Pattern init by the regExp security.deniedScriptletsTokens + */ + private static HashSet initAllowedScriptHashes() { + List allowedScripts = StringUtil.split(UtilProperties.getPropertyValue("security", + "allowedScriptletHashes", ""), ","); + return allowedScripts != null ? new HashSet<>(allowedScripts) : new HashSet<>(); + } + private ScriptUtil() { } private static final class ProtectedBindings implements Bindings { diff --git a/framework/base/src/test/java/org/apache/ofbiz/base/util/string/FlexibleStringExpanderTests.java b/framework/base/src/test/java/org/apache/ofbiz/base/util/string/FlexibleStringExpanderTests.java index 81c39731ce..856b70ee17 100644 --- a/framework/base/src/test/java/org/apache/ofbiz/base/util/string/FlexibleStringExpanderTests.java +++ b/framework/base/src/test/java/org/apache/ofbiz/base/util/string/FlexibleStringExpanderTests.java @@ -305,6 +305,8 @@ public void testEverything() { fseTest("groovy: null", "${groovy:return null;}!", testMap, "!", false); fseTest("groovy missing property", "${groovy: return noList[0]}", testMap, null, null, "", null, false); fseTest("groovy: throw Exception", "${groovy:return throwException.value;}!", testMap, "!", false); + fseTest("groovy: generate security issue", "${groovy: java.util.Map.of('key', 'value')}!", testMap, "!", false); + fseTest("groovy: another generate security issue", "${groovy: 'ls /'.execute()}!", testMap, "!", false); fseTest("groovy: converter exception", "${groovy:return specialNumber;}!", testMap, "1!", false); fseTest("UEL integration: Map", "Hello ${testMap.var}!", testMap, "Hello World!", false); fseTest("UEL integration: blank", "Hello ${testMap.blank}World!", testMap, "Hello World!", false); diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties index 625d56d016..953fccf7ea 100644 --- a/framework/security/config/security.properties +++ b/framework/security/config/security.properties @@ -290,6 +290,19 @@ deniedWebShellTokens=java.,beans,freemarker, Date: Thu, 10 Oct 2024 16:11:49 +0200 Subject: [PATCH 02/13] Update framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java Co-authored-by: Gil Portenseigne --- .../src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java index 86d9409b49..eb79f2e62d 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java @@ -411,7 +411,7 @@ public static Class parseScript(String language, String script) { * Analyse if we can run the script or need to block it due to potential security issue * @param language * @param script - * @return true is we can run the script + * @return true if we can run the script * @throws IOException */ private static boolean isSafeScript(String language, String script) throws IOException { From 21511d1cea1d5d470a96813eca7a906c52d7381d Mon Sep 17 00:00:00 2001 From: Nicolas Malin Date: Thu, 10 Oct 2024 16:12:11 +0200 Subject: [PATCH 03/13] Update framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java Co-authored-by: Gil Portenseigne --- .../src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java index eb79f2e62d..daf4f3962d 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java @@ -417,7 +417,7 @@ public static Class parseScript(String language, String script) { private static boolean isSafeScript(String language, String script) throws IOException { HashSet allowedScript = ALLOWED_SCRIPTS.putIfAbsentAndGet(language, initAllowedScriptHashes()); String scriptHash = HashCrypt.digestHash("SHA", script.getBytes()); - boolean canExecuteScript = allowedScript.contains(scriptHash); + boolean currentScriptAlreadyAllowed = allowedScript.contains(scriptHash); if (!canExecuteScript) { if (!checkIfScriptIsSafe(script)) { Debug.logWarning(String.format("Tried to execute unauthorized script \n **** \n%s\n **** " From 35fca16b2d8e4e9e4028df35ae537fe60e81a913 Mon Sep 17 00:00:00 2001 From: Nicolas Malin Date: Thu, 10 Oct 2024 16:12:18 +0200 Subject: [PATCH 04/13] Update framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java Co-authored-by: Gil Portenseigne --- .../src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java index daf4f3962d..786bceb8c0 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java @@ -418,7 +418,7 @@ private static boolean isSafeScript(String language, String script) throws IOExc HashSet allowedScript = ALLOWED_SCRIPTS.putIfAbsentAndGet(language, initAllowedScriptHashes()); String scriptHash = HashCrypt.digestHash("SHA", script.getBytes()); boolean currentScriptAlreadyAllowed = allowedScript.contains(scriptHash); - if (!canExecuteScript) { + if (!currentScriptAlreadyAllowed) { if (!checkIfScriptIsSafe(script)) { Debug.logWarning(String.format("Tried to execute unauthorized script \n **** \n%s\n **** " + "\nif it's safe script you can add the following hash to security.allowedScriptlets: %s", From 871a1e58a20a60bf670da5bbaf96bc2396e2f6ff Mon Sep 17 00:00:00 2001 From: Nicolas Malin Date: Thu, 10 Oct 2024 16:12:32 +0200 Subject: [PATCH 05/13] Update framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java Co-authored-by: Gil Portenseigne --- .../src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java index 786bceb8c0..753a8a9811 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java @@ -444,7 +444,7 @@ public static boolean checkIfScriptIsSafe(String content) throws IOException { } /** - * Load the regExp for analyse script security + * Load the regExp for security script analysis * @return Pattern init by the regExp security.deniedScriptletsTokens */ private static Pattern initScriptletsTokensPattern() { From 75e67dc6cce5b57759263b20abd05c7568428ac7 Mon Sep 17 00:00:00 2001 From: Nicolas Malin Date: Thu, 10 Oct 2024 16:12:44 +0200 Subject: [PATCH 06/13] Update framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java Co-authored-by: Gil Portenseigne --- .../src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java index 753a8a9811..864842c4d3 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java @@ -453,7 +453,7 @@ private static Pattern initScriptletsTokensPattern() { } /** - * Load the list of script exception that we autorise to run despite the security risk + * Load the list of script exceptions that are authorized to run despite the security risk * @return Pattern init by the regExp security.deniedScriptletsTokens */ private static HashSet initAllowedScriptHashes() { From 5d8d126c503ee4ea957edd6a7b801db38aba0f13 Mon Sep 17 00:00:00 2001 From: Nicolas Malin Date: Fri, 11 Oct 2024 15:43:44 +0200 Subject: [PATCH 07/13] Improved: Allow to use GroovyDsl in FlexibleStringExpander (OFBIZ-13133) Improve reg exp to support more possible code injection --- framework/security/config/security.properties | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties index 953fccf7ea..6c4649c44c 100644 --- a/framework/security/config/security.properties +++ b/framework/security/config/security.properties @@ -293,7 +293,9 @@ allowedTokens=$SHA$OFBiz$2naHrANKTniFcgLJk4oXr3IRQ48 #-- RegExp to secure groovy script execution. If the regExp match a script, it would be disabled and OFBiz run nothing. #-- In this case, you will have on log the original script with it hash. The hash can be added on allowedScriptletHashes #-- properties to accept it on the next execution. -deniedScriptletsTokens=java\.|import|embed|process|class|require|exec|calc +deniedScriptletsTokens=java\\s*\.|import\\s|embed|process|class|require|\.\\s*.exec|\.\\s*calc\ +|System\\s*\.|\.\\s*codehaus|\.\\s*groovy|\.\\s*runtime\|groovyx\\s*\.\ +|Eval\\s*\.|\\s+File #-- If you want to deactivate the security control on each groovy script set to false. # Warn ensure to be sure on what you do because this can open the door for code injection From 1dac7037fffcc045cf57d35f06e3c09d6ef43104 Mon Sep 17 00:00:00 2001 From: Nicolas Malin Date: Fri, 11 Oct 2024 15:44:09 +0200 Subject: [PATCH 08/13] Improved: Allow to use GroovyDsl in FlexibleStringExpander (OFBIZ-13133) Improve reg exp to support more possible code injection --- .../base/util/string/FlexibleStringExpanderTests.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/framework/base/src/test/java/org/apache/ofbiz/base/util/string/FlexibleStringExpanderTests.java b/framework/base/src/test/java/org/apache/ofbiz/base/util/string/FlexibleStringExpanderTests.java index 856b70ee17..fd78cf7ef7 100644 --- a/framework/base/src/test/java/org/apache/ofbiz/base/util/string/FlexibleStringExpanderTests.java +++ b/framework/base/src/test/java/org/apache/ofbiz/base/util/string/FlexibleStringExpanderTests.java @@ -306,7 +306,13 @@ public void testEverything() { fseTest("groovy missing property", "${groovy: return noList[0]}", testMap, null, null, "", null, false); fseTest("groovy: throw Exception", "${groovy:return throwException.value;}!", testMap, "!", false); fseTest("groovy: generate security issue", "${groovy: java.util.Map.of('key', 'value')}!", testMap, "!", false); - fseTest("groovy: another generate security issue", "${groovy: 'ls /'.execute()}!", testMap, "!", false); + fseTest("groovy: another generate security issue 1", "${groovy: 'ls /'.execute()}!", testMap, "!", false); + fseTest("groovy: another generate security issue 2", "${groovy: new File('/etc/passwd').getText()}!", testMap, "!", false); + fseTest("groovy: another generate security issue 3", "${groovy: (new File '/etc/passwd') .getText()}!", testMap, "!", false); + fseTest("groovy: another generate security issue 4", "${groovy: Eval.me('1')}!", testMap, "!", false); + fseTest("groovy: another generate security issue 5", "${groovy: Eval . me('1')}!", testMap, "!", false); + fseTest("groovy: another generate security issue 6", "${groovy: System.properties['ofbiz.home']}!", testMap, "!", false); + fseTest("groovy: another generate security issue 7", "${groovy: new groovyx.net.http.HTTPBuilder('https://XXXX.XXXX.com:443')}!", testMap, "!", false); fseTest("groovy: converter exception", "${groovy:return specialNumber;}!", testMap, "1!", false); fseTest("UEL integration: Map", "Hello ${testMap.var}!", testMap, "Hello World!", false); fseTest("UEL integration: blank", "Hello ${testMap.blank}World!", testMap, "Hello World!", false); From 6ca355f9e4c1ddb8f2f6103df8a3eb78b8f307fe Mon Sep 17 00:00:00 2001 From: Jacques Le Roux Date: Fri, 11 Oct 2024 18:24:06 +0200 Subject: [PATCH 09/13] Update FlexibleStringExpanderTests.java Fixes https://github.com/apache/ofbiz-framework/actions/runs/11293546444/job/31411921173 Line too long --- .../ofbiz/base/util/string/FlexibleStringExpanderTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/framework/base/src/test/java/org/apache/ofbiz/base/util/string/FlexibleStringExpanderTests.java b/framework/base/src/test/java/org/apache/ofbiz/base/util/string/FlexibleStringExpanderTests.java index fd78cf7ef7..aa50f1b046 100644 --- a/framework/base/src/test/java/org/apache/ofbiz/base/util/string/FlexibleStringExpanderTests.java +++ b/framework/base/src/test/java/org/apache/ofbiz/base/util/string/FlexibleStringExpanderTests.java @@ -312,7 +312,8 @@ public void testEverything() { fseTest("groovy: another generate security issue 4", "${groovy: Eval.me('1')}!", testMap, "!", false); fseTest("groovy: another generate security issue 5", "${groovy: Eval . me('1')}!", testMap, "!", false); fseTest("groovy: another generate security issue 6", "${groovy: System.properties['ofbiz.home']}!", testMap, "!", false); - fseTest("groovy: another generate security issue 7", "${groovy: new groovyx.net.http.HTTPBuilder('https://XXXX.XXXX.com:443')}!", testMap, "!", false); + fseTest("groovy: another generate security issue 7", "${groovy: new groovyx.net.http.HTTPBuilder('https://XXXX.XXXX.com:443')}!", + testMap, "!", false); fseTest("groovy: converter exception", "${groovy:return specialNumber;}!", testMap, "1!", false); fseTest("UEL integration: Map", "Hello ${testMap.var}!", testMap, "Hello World!", false); fseTest("UEL integration: blank", "Hello ${testMap.blank}World!", testMap, "Hello World!", false); From 628fbe596dcc931e65bfe4e015923fe1171af0c9 Mon Sep 17 00:00:00 2001 From: Nicolas Malin Date: Thu, 17 Oct 2024 17:27:42 +0200 Subject: [PATCH 10/13] Update framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java --- .../src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java index 864842c4d3..88d1b07cfa 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/ScriptUtil.java @@ -454,7 +454,7 @@ private static Pattern initScriptletsTokensPattern() { /** * Load the list of script exceptions that are authorized to run despite the security risk - * @return Pattern init by the regExp security.deniedScriptletsTokens + * @return Allowed hashes List init by property security.allowedScriptletHashes */ private static HashSet initAllowedScriptHashes() { List allowedScripts = StringUtil.split(UtilProperties.getPropertyValue("security", From 070d55ddddb02734d0ba2dd3ae7ffdfc69c6a2fa Mon Sep 17 00:00:00 2001 From: Gil Portenseigne Date: Mon, 2 Dec 2024 17:53:39 +0100 Subject: [PATCH 11/13] WIP --- build.gradle | 1 + ...FlexibleStringExpanderBaseCodeTests.groovy | 47 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 framework/base/src/test/groovy/org/apache/ofbiz/base/util/string/FlexibleStringExpanderBaseCodeTests.groovy diff --git a/build.gradle b/build.gradle index 61f3424f86..84e23ed5c8 100644 --- a/build.gradle +++ b/build.gradle @@ -304,6 +304,7 @@ sourceSets { srcDirs = getDirectoryInActiveComponentsIfExists('src/test/groovy') include 'org/apache/ofbiz/service/ModelServiceTest.groovy' include 'org/apache/ofbiz/test/TestServices.groovy' + include 'org/apache/ofbiz/base/util/string/FlexibleStringExpanderBaseCodeTests.groovy' include 'org/apache/ofbiz/base/util/FileUtilTests.groovy' include 'org/apache/ofbiz/service/test/ServicePurgeTest.groovy' include 'org.apache.ofbiz.base.test.SimpleTests.groovy' diff --git a/framework/base/src/test/groovy/org/apache/ofbiz/base/util/string/FlexibleStringExpanderBaseCodeTests.groovy b/framework/base/src/test/groovy/org/apache/ofbiz/base/util/string/FlexibleStringExpanderBaseCodeTests.groovy new file mode 100644 index 0000000000..f37786702d --- /dev/null +++ b/framework/base/src/test/groovy/org/apache/ofbiz/base/util/string/FlexibleStringExpanderBaseCodeTests.groovy @@ -0,0 +1,47 @@ +/******************************************************************************* + * 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.base.util.string + +import groovy.io.FileType +import groovy.xml.XmlSlurper +import org.junit.Test + +class FlexibleStringExpanderBaseCodeTests { + + @Test + void testEveryGroovyScriptletFromXmlFiles() { + def filterWidgetXmlFiles = ~/\.\/(framework|application|plugins).*\/widget\/.*(Screens|Menus|Forms)\.xml$/ + new File(".").traverse(type: FileType.FILES, filter: filterWidgetXmlFiles) {it -> + parseXmlFile(it) + } + assert false + } + + String parseXmlFile(File file) { + List setWithGroovy = new XmlSlurper().parse(file).findAll { node -> + node.name() == 'set' && node.text().contains('groovy:') + }.collect() + + if (setWithGroovy){ + println setWithGroovy.first() + } + return '' + } + +} From df2b501a22970d5b2ac6146f5cea34d047f59e62 Mon Sep 17 00:00:00 2001 From: Nicolas Malin Date: Thu, 10 Oct 2024 14:07:49 +0200 Subject: [PATCH 12/13] Improved: Allow to use GroovyDsl in FlexibleStringExpander (OFBIZ-13133) Second improvement on this functionality with increase the security by analyse each script to control the presence of potential code injection. The regexp to control is a property: security.deniedScriptletsTokens. If a script match the regexp, OFBiz raise in log an alert with the script and the script hash. The script is disabled and can't run. If you have a safe script who is matched by the regexp, you can add the hash given by OFBiz on the property: security.allowedScriptletHashes --- framework/security/config/security.properties | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties index 6c4649c44c..91a1e98fb7 100644 --- a/framework/security/config/security.properties +++ b/framework/security/config/security.properties @@ -305,6 +305,19 @@ useDeniedScriptletsTokens=true #-- like allowedScriptletHashes={SHA}59f8ab616b3878ddf825ea50c13ce603a3a6c5a9,{SHA}59f5ab516b3878ddf825ea50c13ce603a3a6c5a9 allowedScriptletHashes= +#-- RegExp to secure groovy script execution. If the regExp match a script, it would be disabled and OFBiz run nothing. +#-- In this case, you will have on log the original script with it hash. The hash can be added on allowedScriptletHashes +#-- properties to accept it on the next execution. +deniedScriptletsTokens=java\.|import|embed|process|class|require|exec|calc + +#-- If you want to deactivate the security control on each groovy script set to false. +# Warn ensure to be sure on what you do because this can open the door for code injection +useDeniedScriptletsTokens=true + +#-- To accept the execution on some groovy script who match the deniedScriptletsTokens regExp, put their hash here. +#-- like allowedScriptletHashes={SHA}59f8ab616b3878ddf825ea50c13ce603a3a6c5a9,{SHA}59f5ab516b3878ddf825ea50c13ce603a3a6c5a9 +allowedScriptletHashes= + allowStringConcatenationInUploadedFiles=false #-- Max line length for uploaded files, by default 10000. You can use 0 to allow any line length. From a4b3e3559347ed0cbef783d6a8e63a154fba4a0c Mon Sep 17 00:00:00 2001 From: Nicolas Malin Date: Thu, 2 Jan 2025 11:37:51 +0100 Subject: [PATCH 13/13] Updated test to analyse all scriplet and return all unsafe scriptlet found. Test is true if all scriptlet are safe --- ...FlexibleStringExpanderBaseCodeTests.groovy | 40 +++++++++++++------ framework/security/config/security.properties | 19 ++------- 2 files changed, 31 insertions(+), 28 deletions(-) diff --git a/framework/base/src/test/groovy/org/apache/ofbiz/base/util/string/FlexibleStringExpanderBaseCodeTests.groovy b/framework/base/src/test/groovy/org/apache/ofbiz/base/util/string/FlexibleStringExpanderBaseCodeTests.groovy index f37786702d..7dac90818b 100644 --- a/framework/base/src/test/groovy/org/apache/ofbiz/base/util/string/FlexibleStringExpanderBaseCodeTests.groovy +++ b/framework/base/src/test/groovy/org/apache/ofbiz/base/util/string/FlexibleStringExpanderBaseCodeTests.groovy @@ -19,29 +19,45 @@ package org.apache.ofbiz.base.util.string import groovy.io.FileType -import groovy.xml.XmlSlurper +import org.apache.ofbiz.base.util.ScriptUtil import org.junit.Test +import java.util.regex.MatchResult +import java.util.regex.Matcher +import java.util.regex.Pattern + class FlexibleStringExpanderBaseCodeTests { + Pattern pattern = Pattern.compile('\\$\\{groovy:.*}') @Test void testEveryGroovyScriptletFromXmlFiles() { def filterWidgetXmlFiles = ~/\.\/(framework|application|plugins).*\/widget\/.*(Screens|Menus|Forms)\.xml$/ new File(".").traverse(type: FileType.FILES, filter: filterWidgetXmlFiles) {it -> - parseXmlFile(it) + assert parseXmlFile(it).isEmpty() } - assert false } - String parseXmlFile(File file) { - List setWithGroovy = new XmlSlurper().parse(file).findAll { node -> - node.name() == 'set' && node.text().contains('groovy:') - }.collect() - - if (setWithGroovy){ - println setWithGroovy.first() + /** Resolve all scriptlet on file on retrieve all identity as unsafe + * + * @param file + * @return List unsafe scriptlet + */ + List parseXmlFile(File file) { + String text = file.getText() + Matcher matcher = pattern.matcher(text) + List matchedScriptlet = [] + for (MatchResult matchResult : matcher.results().toList()) { + String scriptlet = text.substring(matchResult.start() + 9, matchResult.end() - 1) + if (!ScriptUtil.checkIfScriptIsSafe(scriptlet)) { + matchedScriptlet << scriptlet + } } - return '' + if (matchedScriptlet) { + println "Unsafe scriptlet found on file ${file.getName()} : " + println '*************************************' + println '* ' + matchedScriptlet.join('\n* ') + println '*************************************' + } + return matchedScriptlet } - } diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties index 91a1e98fb7..6fbc9b9ea8 100644 --- a/framework/security/config/security.properties +++ b/framework/security/config/security.properties @@ -293,22 +293,9 @@ allowedTokens=$SHA$OFBiz$2naHrANKTniFcgLJk4oXr3IRQ48 #-- RegExp to secure groovy script execution. If the regExp match a script, it would be disabled and OFBiz run nothing. #-- In this case, you will have on log the original script with it hash. The hash can be added on allowedScriptletHashes #-- properties to accept it on the next execution. -deniedScriptletsTokens=java\\s*\.|import\\s|embed|process|class|require|\.\\s*.exec|\.\\s*calc\ -|System\\s*\.|\.\\s*codehaus|\.\\s*groovy|\.\\s*runtime\|groovyx\\s*\.\ -|Eval\\s*\.|\\s+File - -#-- If you want to deactivate the security control on each groovy script set to false. -# Warn ensure to be sure on what you do because this can open the door for code injection -useDeniedScriptletsTokens=true - -#-- To accept the execution on some groovy script who match the deniedScriptletsTokens regExp, put their hash here. -#-- like allowedScriptletHashes={SHA}59f8ab616b3878ddf825ea50c13ce603a3a6c5a9,{SHA}59f5ab516b3878ddf825ea50c13ce603a3a6c5a9 -allowedScriptletHashes= - -#-- RegExp to secure groovy script execution. If the regExp match a script, it would be disabled and OFBiz run nothing. -#-- In this case, you will have on log the original script with it hash. The hash can be added on allowedScriptletHashes -#-- properties to accept it on the next execution. -deniedScriptletsTokens=java\.|import|embed|process|class|require|exec|calc +deniedScriptletsTokens=java\\s*\.|import\\s|embed[^\\w]|process[^\\w]|class[^\\w]|require[^\\w]\ +|\.\\s*.exec.*[\(|\\s]|\.\\s*calc.*[\(|\\s]|\.\\s*.eval.*[\(|\\s]|Eval\\s*\.|\\s+File\ +|System\\s*\.|\.\\s*codehaus|\.\\s*groovy[^:]|\.\\s*runtime\|groovyx\\s*\. #-- If you want to deactivate the security control on each groovy script set to false. # Warn ensure to be sure on what you do because this can open the door for code injection