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 2022/02/23 11:00:31 UTC
[ofbiz-framework] branch trunk updated (f3a9f05 -> 53dc91a)
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a change to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git.
from f3a9f05 Improved: Remove Whitespaces before checkboxes (OFBIZ-10461)
new 6cb9b12 Fixed: Trivial change in ProgramExport.groovy, uses the SecuredUpload import
new 835934a Fixed: Secure the uploads (OFBIZ-12080)
new 53dc91a Fixed: Prevent post-Auth vulnerability: FreeMarker Bypass (OFBIZ-12582)
The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../ofbiz/content/ContentManagementServices.java | 18 +++++++++++++++++-
framework/security/config/security.properties | 4 ++--
.../org/apache/ofbiz/security/SecurityUtilTest.java | 8 +++++---
.../webtools/groovyScripts/entity/ProgramExport.groovy | 3 ++-
4 files changed, 26 insertions(+), 7 deletions(-)
[ofbiz-framework] 02/03: Fixed: Secure the uploads (OFBIZ-12080)
Posted by jl...@apache.org.
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
commit 835934a9c986f1381d24c8fd85478ed6ab908082
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Wed Feb 23 09:33:17 2022 +0100
Fixed: Secure the uploads (OFBIZ-12080)
Adds some tokens in security.properties::deniedWebShellTokens
Updates SecurityUtilTest::webShellTokensTesting accordingly
---
framework/security/config/security.properties | 4 ++--
.../src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java | 8 +++++---
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties
index 8ad620b..d41def2 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -249,10 +249,10 @@ allowAllUploads=
#-- "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...
#-- If you are sure you are safe for a token you can remove it, etc.
-deniedWebShellTokens=freemarker,<script,javascript,<body,<form,<jsp:,<c:out,taglib,<prefix,<%@ page,<?php,exec(,\
+deniedWebShellTokens=java.,beans,freemarker,<script,javascript,<body,<form,<jsp:,<c:out,taglib,<prefix,<%@ page,<?php,exec(,\
%eval,@eval,eval(,runtime,import,passthru,shell_exec,assert,str_rot13,system,decode,include,page ,\
chmod,mkdir,fopen,fclose,new file,upload,getfilename,download,getoutputstring,readfile,\
- python,perl ,/perl,ruby ,/ruby,process,function,class,InputStream,to_server,wget ,\
+ python,perl ,/perl,ruby ,/ruby,process,function,class,InputStream,to_server,wget ,static,\
ifconfig,route,crontab,netstat,uname ,hostname,iptables,whoami,"cmd",*cmd|,+cmd|,=cmd|,localhost
diff --git a/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java b/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
index 4cf8e37..0afa0f6 100644
--- a/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
+++ b/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
@@ -59,10 +59,10 @@ public class SecurityUtilTest {
@Test
public void webShellTokensTesting() {
// Currently used
- // freemarker,<script,javascript,<body,<form,<jsp:,<c:out,taglib,<prefix,<%@ page,<?php,exec(,\
+ // java.,beans,freemarker,<script,javascript,<body,<form,<jsp:,<c:out,taglib,<prefix,<%@ page,<?php,exec(,\
// %eval,@eval,eval(,runtime,import,passthru,shell_exec,assert,str_rot13,system,decode,include,page ,\
// chmod,mkdir,fopen,fclose,new file,upload,getfilename,download,getoutputstring,readfile,\
- // python,perl ,/perl,ruby ,/ruby,process,function,class,InputStream,to_server,wget ,\
+ // python,perl ,/perl,ruby ,/ruby,process,function,class,InputStream,to_server,wget ,static,\
// ifconfig,route,crontab,netstat,uname,hostname,iptables,whoami,"cmd",*cmd|,+cmd|,=cmd|,localhost
try {
@@ -72,6 +72,8 @@ public class SecurityUtilTest {
allowed = new ArrayList<>();
assertFalse(SecuredUpload.isValidText("hack.getFileName", allowed));
+ assertFalse(SecuredUpload.isValidText("java.", allowed));
+ assertFalse(SecuredUpload.isValidText("beans", allowed));
assertFalse(SecuredUpload.isValidText("freemarker", allowed));
assertFalse(SecuredUpload.isValidText("<script", allowed));
assertFalse(SecuredUpload.isValidText("javascript", allowed));
@@ -117,7 +119,7 @@ public class SecurityUtilTest {
assertFalse(SecuredUpload.isValidText("function", allowed));
assertFalse(SecuredUpload.isValidText("class", allowed));
assertFalse(SecuredUpload.isValidText("wget ", allowed));
-
+ assertFalse(SecuredUpload.isValidText("static", allowed));
assertFalse(SecuredUpload.isValidText("ifconfig", allowed));
assertFalse(SecuredUpload.isValidText("route", allowed));
[ofbiz-framework] 01/03: Fixed: Trivial change in ProgramExport.groovy, uses the SecuredUpload import
Posted by jl...@apache.org.
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
commit 6cb9b12df1fd9d9beb5c182403a3c39307ef188e
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Wed Feb 23 09:31:25 2022 +0100
Fixed: Trivial change in ProgramExport.groovy, uses the SecuredUpload import
No Functional change
---
framework/webtools/groovyScripts/entity/ProgramExport.groovy | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/framework/webtools/groovyScripts/entity/ProgramExport.groovy b/framework/webtools/groovyScripts/entity/ProgramExport.groovy
index 639b82d..47923d4 100644
--- a/framework/webtools/groovyScripts/entity/ProgramExport.groovy
+++ b/framework/webtools/groovyScripts/entity/ProgramExport.groovy
@@ -17,6 +17,7 @@
* under the License.
*/
import org.apache.ofbiz.entity.GenericValue
+import org.apache.ofbiz.security.SecuredUpload
import org.codehaus.groovy.control.CompilerConfiguration
import org.codehaus.groovy.control.MultipleCompilationErrorsException
import org.codehaus.groovy.control.customizers.ImportCustomizer
@@ -78,7 +79,7 @@ def shell = new GroovyShell(loader, binding, configuration)
if (groovyProgram) {
try {
// Check if a webshell is not uploaded but allow "import"
- if (!org.apache.ofbiz.security.SecuredUpload.isValidText(groovyProgram,["import"])) {
+ if (!SecuredUpload.isValidText(groovyProgram, ["import"])) {
logError("================== Not executed for security reason ==================")
request.setAttribute("_ERROR_MESSAGE_", "Not executed for security reason")
return
[ofbiz-framework] 03/03: Fixed: Prevent post-Auth vulnerability: FreeMarker Bypass (OFBIZ-12582)
Posted by jl...@apache.org.
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
commit 53dc91aa0eac034498fa2f52802d4b386f2f89e6
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Wed Feb 23 11:46:18 2022 +0100
Fixed: Prevent post-Auth vulnerability: FreeMarker Bypass (OFBIZ-12582)
By inserting malicious content in the “Text” field from
“/content/control/updateLayoutSubContent” -> “Templates”, an attacker may
perform SSTI (Server-Side Template Injection) attacks, which can leverage
FreeMarker exposed objects to bypass restrictions and obtain RCE (Remote Code
Execution).
This fixes it by calling SecuredUpload::isValidText on the “Text” field content.
I'll check that there are no other attack opportunities...
Thanks: Mal Aware <aw...@gmail.com> for reporting this post-auth vulnerabily
---
.../ofbiz/content/ContentManagementServices.java | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/applications/content/src/main/java/org/apache/ofbiz/content/ContentManagementServices.java b/applications/content/src/main/java/org/apache/ofbiz/content/ContentManagementServices.java
index a9b2133..b273d66 100644
--- a/applications/content/src/main/java/org/apache/ofbiz/content/ContentManagementServices.java
+++ b/applications/content/src/main/java/org/apache/ofbiz/content/ContentManagementServices.java
@@ -23,6 +23,7 @@ import java.io.IOException;
import java.math.BigDecimal;
import java.nio.ByteBuffer;
import java.sql.Timestamp;
+import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
@@ -50,6 +51,7 @@ import org.apache.ofbiz.entity.condition.EntityOperator;
import org.apache.ofbiz.entity.model.ModelUtil;
import org.apache.ofbiz.entity.util.EntityQuery;
import org.apache.ofbiz.entity.util.EntityUtil;
+import org.apache.ofbiz.security.SecuredUpload;
import org.apache.ofbiz.security.Security;
import org.apache.ofbiz.service.DispatchContext;
import org.apache.ofbiz.service.GenericServiceException;
@@ -146,6 +148,20 @@ public class ContentManagementServices {
Map<String, Object> context = UtilMisc.makeMapWritable(rcontext);
Locale locale = (Locale) context.get("locale");
+ // Check if a webshell is not uploaded
+ String textData = (String) context.get("textData");
+ if (UtilValidate.isNotEmpty(textData)) {
+ try {
+ if (!SecuredUpload.isValidText(textData, Collections.emptyList())) {
+ Debug.logError("================== Not saved for security reason ==================", MODULE);
+ return ServiceUtil.returnError("================== Not saved for security reason ==================");
+ }
+ } catch (IOException e) {
+ Debug.logError("================== Not saved for security reason ==================", MODULE);
+ return ServiceUtil.returnError("================== Not saved for security reason ==================");
+ }
+ }
+
// Knowing why a request fails permission check is one of the more difficult
// aspects of content management. Setting "displayFailCond" to true will
// put an html table in result.errorMessage that will show what tests were performed
@@ -176,7 +192,7 @@ public class ContentManagementServices {
if (Debug.infoOn()) {
Debug.logInfo("in persist... contentPurposeList(0):" + contentPurposeList, MODULE);
- Debug.logInfo("in persist... textData(0):" + context.get("textData"), MODULE);
+ Debug.logInfo("in persist... textData(0):" + textData, MODULE);
}
GenericValue content = delegator.makeValue("Content");