This is an automated email from the ASF dual-hosted git repository.
nmalin pushed a commit to branch release24.09
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 81cb0a78526181095895f297bd8d29ca6d63f1ea
Author: Nicolas Malin <nicolas.ma...@nereide.fr>
AuthorDate: Thu Jan 2 14:18:01 2025 +0100
Improved: Allow to use GroovyDsl in FlexibleStringExpander (OFBIZ-13133)
(#839)
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
We added a new test that scan all xml file to analyse groovy scriplet and
return all unsafe scriptlet found. The test will fail if unsafe scriptlets was
found.
Thanks to Gil Portenseigne and Jacques Le Roux for help and review
---
build.gradle | 1 +
.../org/apache/ofbiz/base/util/ScriptUtil.java | 67 +++++++++++++++++++++-
.../FlexibleStringExpanderBaseCodeTests.groovy | 63 ++++++++++++++++++++
.../util/string/FlexibleStringExpanderTests.java | 9 +++
framework/security/config/security.properties | 15 +++++
5 files changed, 154 insertions(+), 1 deletion(-)
diff --git a/build.gradle b/build.gradle
index d5d20c61aa..31db14b48f 100644
--- a/build.gradle
+++ b/build.gradle
@@ -254,6 +254,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'
}
resources {
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 cb673e0c30..8c0eb07009 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.List;
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.ScriptException;
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 <code>ScriptHelper</code> key. */
public static final String SCRIPT_HELPER_KEY = "ofbiz";
private static final UtilCache<String, CompiledScript> PARSED_SCRIPTS =
UtilCache.createUtilCache("script.ParsedScripts", 0, 0, false);
+ private static final UtilCache<String, HashSet<String>> 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<String> SCRIPT_NAMES;
+ private static final Pattern DENIEDSCRIPTLETSTOKENS =
initScriptletsTokensPattern();
+ private static final Boolean USEDENIEDSCRIPTLETSTOKENS =
UtilProperties.getPropertyAsBoolean("security", "useDeniedScriptletsTokens",
false);
static {
Set<String> writableScriptNames = new HashSet<>();
@@ -237,6 +242,9 @@ public final class ScriptUtil {
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 final class ScriptUtil {
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 final class ScriptUtil {
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 if we can run the script
+ * @throws IOException
+ */
+ private static boolean isSafeScript(String language, String script) throws
IOException {
+ HashSet<String> allowedScript =
ALLOWED_SCRIPTS.putIfAbsentAndGet(language, initAllowedScriptHashes());
+ String scriptHash = HashCrypt.digestHash("SHA", script.getBytes());
+ boolean currentScriptAlreadyAllowed =
allowedScript.contains(scriptHash);
+ 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",
+ 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 security script analysis
+ * @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 exceptions that are authorized to run despite
the security risk
+ * @return Allowed hashes List init by property
security.allowedScriptletHashes
+ */
+ private static HashSet<String> initAllowedScriptHashes() {
+ List<String> 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/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..7dac90818b
--- /dev/null
+++
b/framework/base/src/test/groovy/org/apache/ofbiz/base/util/string/FlexibleStringExpanderBaseCodeTests.groovy
@@ -0,0 +1,63 @@
+/*******************************************************************************
+ * 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 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 ->
+ assert parseXmlFile(it).isEmpty()
+ }
+ }
+
+ /** 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
+ }
+ }
+ if (matchedScriptlet) {
+ println "Unsafe scriptlet found on file ${file.getName()} : "
+ println '*************************************'
+ println '* ' + matchedScriptlet.join('\n* ')
+ println '*************************************'
+ }
+ return matchedScriptlet
+ }
+}
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..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
@@ -305,6 +305,15 @@ public class FlexibleStringExpanderTests {
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 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);
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index 7551462d32..544dc16421 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -290,6 +290,21 @@
deniedWebShellTokens=java.,beans,freemarker,<script,javascript,<body,body ,<form
#-- If you add a token beware that it does not content ",". It's the separator.
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[^\\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
+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.
