Improved: Allow to use GroovyDsl in FlexibleStringExpander (OFBIZ-13133)

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'

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 <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 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 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 {

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
+    }
+}

framework/base/src/test/java/org/apache/ofbiz/base/util/string/FlexibleStringExpanderTests.java

@@ -305,6 +305,15 @@ 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 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);

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.