You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by jl...@apache.org on 2021/09/17 16:51:16 UTC
[ofbiz-framework] branch trunk updated: Fixed: Groovy Program
sandbox bypass (OFBIZ-12305)
This is an automated email from the ASF dual-hosted git repository.
jleroux 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 6a7f3cd Fixed: Groovy Program sandbox bypass (OFBIZ-12305)
6a7f3cd is described below
commit 6a7f3cd83e61f97c4ece6283ba04c683ab84b910
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Fri Sep 17 18:45:58 2021 +0200
Fixed: Groovy Program sandbox bypass (OFBIZ-12305)
This much increases the current security by creating SecuredUpload::isValidText
and call it from ProgramExport.groovy
This said I agree with thiscodecc that a better solution would be to use a
Groovy sandbox, and not only in ProgramExport.groovy.
I had a quick glance at a such solution. Unfortunately as Cédric Champeau reports
at https://melix.github.io/blog/2015/03/sandboxing.html and unlike thiscodecc
suggest there are no "mature solutions on the market".
Nevertheless I'll have a deeper look at an OFBiz specific Groovy sandbox
solution.
Somehow related: I'll also soon extract the list of words used in
SecuredUpload::isValidText in a deniedWebShellWords property in security.properties
Thanks: thiscodecc for report
---
.../org/apache/ofbiz/security/SecuredUpload.java | 77 ++++++++++++----------
.../groovyScripts/entity/ProgramExport.groovy | 8 +--
2 files changed, 45 insertions(+), 40 deletions(-)
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 682cab6..7c7f462 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
@@ -34,6 +34,7 @@ import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.StandardOpenOption;
+import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
@@ -609,47 +610,53 @@ public class SecuredUpload {
return false;
}
String content = new String(bytesFromFile);
- return isValidText(content);
+ ArrayList<String> allowed = new ArrayList<>();
+ return isValidText(content, allowed);
}
- public static boolean isValidText(String content) throws IOException {
- return !(content.toLowerCase().contains("freemarker") // Should be OK, should not be used in Freemarker templates, not part of the syntax.
- // Else "template.utility.Execute" is a good replacement but not as much catching, who
- // knows...
- || content.toLowerCase().contains("import=\"java")
- || content.toLowerCase().contains("runtime.getruntime().exec(")
- || content.toLowerCase().contains("<%@ page")
- || content.toLowerCase().contains("<script")
- || content.toLowerCase().contains("<body>")
- || content.toLowerCase().contains("<form")
- || content.toLowerCase().contains("php")
- || content.toLowerCase().contains("javascript")
- || content.toLowerCase().contains("%eval")
- || content.toLowerCase().contains("@eval")
- || content.toLowerCase().contains("import os") // Python
- || content.toLowerCase().contains("passthru")
- || content.toLowerCase().contains("exec")
- || content.toLowerCase().contains("shell_exec")
- || content.toLowerCase().contains("assert")
- || content.toLowerCase().contains("str_rot13")
- || content.toLowerCase().contains("system")
- || content.toLowerCase().contains("phpinfo")
- || content.toLowerCase().contains("base64_decode")
- || content.toLowerCase().contains("chmod")
- || content.toLowerCase().contains("mkdir")
- || content.toLowerCase().contains("fopen")
- || content.toLowerCase().contains("fclose")
- || content.toLowerCase().contains("new file")
- || content.toLowerCase().contains("import")
- || content.toLowerCase().contains("upload")
- || content.toLowerCase().contains("getfilename")
- || content.toLowerCase().contains("download")
- || content.toLowerCase().contains("getoutputstring")
- || content.toLowerCase().contains("readfile"));
+ public static boolean isValidText(String content, List<String> allowed) throws IOException {
+ // "freemarker" should be OK, should not be used in Freemarker templates, not part of the syntax.
+ // Else "template.utility.Execute" is a good replacement but not as much catching, who knows...
+ return isValid(content, "freemarker", allowed)
+ && isValid(content, "freemarker", allowed)
+ && isValid(content, "import=\"java", allowed)
+ && isValid(content, "runtime.getruntime().exec(", allowed)
+ && isValid(content, "<%@ page", allowed)
+ && isValid(content, "<script", allowed)
+ && isValid(content, "<body>", allowed)
+ && isValid(content, "<form", allowed)
+ && isValid(content, "php", allowed)
+ && isValid(content, "javascript", allowed)
+ && isValid(content, "%eval", allowed)
+ && isValid(content, "@eval", allowed)
+ && isValid(content, "import os", allowed) // Python
+ && isValid(content, "passthru", allowed)
+ && isValid(content, "exec", allowed)
+ && isValid(content, "shell_exec", allowed)
+ && isValid(content, "assert", allowed)
+ && isValid(content, "str_rot13", allowed)
+ && isValid(content, "system", allowed)
+ && isValid(content, "phpinfo", allowed)
+ && isValid(content, "base64_decode", allowed)
+ && isValid(content, "chmod", allowed)
+ && isValid(content, "mkdir", allowed)
+ && isValid(content, "fopen", allowed)
+ && isValid(content, "fclose", allowed)
+ && isValid(content, "new file", allowed)
+ && isValid(content, "import", allowed)
+ && isValid(content, "upload", allowed)
+ && isValid(content, "getfilename", allowed)
+ && isValid(content, "download", allowed)
+ && isValid(content, "getoutputstring", allowed)
+ && isValid(content, "readfile", allowed);
// TODO.... to be continued with known webshell contents... a complete allow list is impossible anyway...
// eg: https://www.acunetix.com/blog/articles/detection-prevention-introduction-web-shells-part-5/
}
+ private static boolean isValid(String content, String string, List<String> allowed) {
+ return !content.toLowerCase().contains(string) || allowed.contains(string);
+ }
+
private static void deleteBadFile(String fileToCheck) {
Debug.logError("File :" + fileToCheck + ", can't be uploaded for security reason", MODULE);
File badFile = new File(fileToCheck);
diff --git a/framework/webtools/groovyScripts/entity/ProgramExport.groovy b/framework/webtools/groovyScripts/entity/ProgramExport.groovy
index e06cd96..0d3a5f7 100644
--- a/framework/webtools/groovyScripts/entity/ProgramExport.groovy
+++ b/framework/webtools/groovyScripts/entity/ProgramExport.groovy
@@ -22,6 +22,7 @@ import org.apache.ofbiz.entity.condition.EntityCondition
import org.apache.ofbiz.entity.condition.EntityOperator
import org.apache.ofbiz.entity.model.ModelEntity
import org.apache.ofbiz.base.util.*
+
import org.w3c.dom.Document
import org.codehaus.groovy.control.customizers.ImportCustomizer
@@ -34,7 +35,7 @@ recordValues = []
errMsgList = []
if (!parameters.groovyProgram) {
-
+
groovyProgram = '''
// Use the List variable recordValues to fill it with GenericValue maps.
// full groovy syntax is available
@@ -85,10 +86,7 @@ def shell = new GroovyShell(loader, binding, configuration)
if (groovyProgram) {
try {
- // TODO more can be added...
- if (groovyProgram.contains("new File")
- || groovyProgram.contains(".jsp")
- || groovyProgram.contains("<%=")) {
+ if (!org.apache.ofbiz.security.SecuredUpload.isValidText(groovyProgram,["import"])) {
request.setAttribute("_ERROR_MESSAGE_", "Not executed for security reason")
return
}