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/10 13:18:15 UTC
[ofbiz-framework] branch release22.01 updated (b447f4d -> 8691b1a)
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a change to branch release22.01
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git.
from b447f4d Fixed: Remote Code Execution (File Upload) Vulnerability (OFBIZ-11948)
new 9ea39d4 Documented: Possible authenticated attack related to Tomcat CVE-2020-1938 (OFBIZ-12558)
new af6286e Improved: no functional trivial cleaning changes
new 4bb37d7 Improved: Possible authenticated attack related to Tomcat CVE-2020-1938 (OFBIZ-12558)
new 8691b1a Fixed: Remote Code Execution (File Upload) Vulnerability (OFBIZ-11948)
The 4 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:
README.adoc | 3 +-
.../ofbiz/content/ContentManagementServices.java | 3 +
.../apache/ofbiz/content/data/DataServices.java | 1 +
applications/product/config/catalog.properties | 4 +-
.../org/apache/ofbiz/product/image/ScaleImage.java | 2 +
.../ofbiz/product/imagemanagement/FrameImage.java | 3 +-
.../imagemanagement/ImageManagementServices.java | 4 +-
.../ofbiz/product/product/ProductServices.java | 2 +
.../ofbiz/base/util/HttpRequestFileUpload.java | 1 +
framework/catalina/ofbiz-component.xml | 15 ++--
framework/security/config/security.properties | 23 ++++--
.../org/apache/ofbiz/security/SecuredUpload.java | 82 +++++++++++++++-------
.../apache/ofbiz/security/SecurityUtilTest.java | 41 +++++++++--
.../ofbiz/service/engine/GroovyBaseScript.groovy | 15 +++-
.../groovyScripts/entity/ProgramExport.groovy | 12 +---
runtime/patches/README | 1 -
themes/common-theme/ofbiz-component.xml | 1 -
17 files changed, 150 insertions(+), 63 deletions(-)
delete mode 100644 runtime/patches/README
[ofbiz-framework] 01/04: Documented: Possible authenticated attack related to Tomcat CVE-2020-1938 (OFBIZ-12558)
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch release22.01
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 9ea39d4eff258d51b4f7eb25006d748ba66b8e99
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Mon Feb 7 10:40:43 2022 +0100
Documented: Possible authenticated attack related to Tomcat CVE-2020-1938 (OFBIZ-12558)
Explains that the current AJP config works only for localhost
---
framework/catalina/ofbiz-component.xml | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/framework/catalina/ofbiz-component.xml b/framework/catalina/ofbiz-component.xml
index a06eddc..b63100f 100644
--- a/framework/catalina/ofbiz-component.xml
+++ b/framework/catalina/ofbiz-component.xml
@@ -81,7 +81,8 @@ under the License.
This is in relation with
https://tomcat.apache.org/security-9.html#Fixed_in_Apache_Tomcat_9.0.31
and
- https://tomcat.apache.org/tomcat-9.0-doc/security-howto.html#Connectors
+ https://tomcat.apache.org/tomcat-9.0-doc/security-howto.html#Connectors
+ Long story short, with this configuration only locahost works...
-->
<!-- <property name="secretRequired" value="false"/>
<property name="allowedRequestAttributesPattern" value=".*"/> -->
[ofbiz-framework] 02/04: Improved: no functional trivial cleaning changes
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch release22.01
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit af6286e3045bd31b530a3432a64f426be231ce07
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Mon Feb 7 10:47:18 2022 +0100
Improved: no functional trivial cleaning changes
Removes useless menu-name="secondary" for common-theme webapp
Removes now useless runtime/patches directory. It was only used with Ant
---
runtime/patches/README | 1 -
themes/common-theme/ofbiz-component.xml | 1 -
2 files changed, 2 deletions(-)
diff --git a/runtime/patches/README b/runtime/patches/README
deleted file mode 100644
index 6214ebb..0000000
--- a/runtime/patches/README
+++ /dev/null
@@ -1 +0,0 @@
-To put your patches, related with the build-dev ant target
\ No newline at end of file
diff --git a/themes/common-theme/ofbiz-component.xml b/themes/common-theme/ofbiz-component.xml
index 75a72d5..5987040 100644
--- a/themes/common-theme/ofbiz-component.xml
+++ b/themes/common-theme/ofbiz-component.xml
@@ -31,7 +31,6 @@ under the License.
<!-- web applications -->
<webapp name="common-theme"
title="common theme"
- menu-name="secondary"
server="default-server"
location="webapp/common"
mount-point="/common"
[ofbiz-framework] 03/04: Improved: Possible authenticated attack related to Tomcat CVE-2020-1938 (OFBIZ-12558)
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch release22.01
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 4bb37d795013090fa63de2db7385540a404a7278
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Thu Feb 10 07:33:52 2022 +0100
Improved: Possible authenticated attack related to Tomcat CVE-2020-1938 (OFBIZ-12558)
As reported by Michael, secretRequired value must be false because secret value
is empty. Else a notifying message appears in log saying that AJP is not available.
This uncomment out secretRequired, so its value is now false, and document more
notably about that. I'll later add more information in the
https://cwiki.apache.org/confluence/display/OFBIZ/Keeping+OFBiz+secure wiki page
Thanks: Michael for report
---
README.adoc | 3 ++-
framework/catalina/ofbiz-component.xml | 14 +++++++++-----
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/README.adoc b/README.adoc
index 039d1b5..965b7f0 100644
--- a/README.adoc
+++ b/README.adoc
@@ -167,7 +167,8 @@ Once proper mitigations to the security issues are complete the OFBiz team will
disclose this information to the public mailing list.
* If you find a post-auth security issue, please https://s.apache.org/dsj2p[create a bug in our issue tracker (Jira)] .
-
+* If you want to use AJP on a non localhost OFBiz instance, you need to set the value of allowedRequestAttributesPattern
+in framework/catalina/ofbiz-component.xml
You can find more information about security in OFBiz at
https://cwiki.apache.org/confluence/display/OFBIZ/Keeping+OFBiz+secure[Keeping OFBiz secure]
diff --git a/framework/catalina/ofbiz-component.xml b/framework/catalina/ofbiz-component.xml
index b63100f..b736370 100644
--- a/framework/catalina/ofbiz-component.xml
+++ b/framework/catalina/ofbiz-component.xml
@@ -75,17 +75,21 @@ under the License.
<property name="URIEncoding" value="UTF-8"/>
<property name="xpoweredBy" value="false"/>
<!-- AJP/13 connector attributes -->
- <!-- Despite OFBIZ-11407, the 2 values below are commented out because of OFBIZ-12558
- The Tomcat default values will be used as recommended by
+ <!-- Despite OFBIZ-11407, allowedRequestAttributesPattern is commented out because of OFBIZ-12558
+ OOTB the Tomcat default values should be used as recommended by
https://tomcat.apache.org/tomcat-9.0-doc/config/ajp.html#Introduction
This is in relation with
https://tomcat.apache.org/security-9.html#Fixed_in_Apache_Tomcat_9.0.31
and
https://tomcat.apache.org/tomcat-9.0-doc/security-howto.html#Connectors
- Long story short, with this configuration only locahost works...
+ But secretRequired value must be false because secret value is empty
+ Else a notifying message appears in log saying that AJP is not available.
+
+ Long story short, with this configuration only localhost works.
+ So if you use it you need to use value/s
-->
- <!-- <property name="secretRequired" value="false"/>
- <property name="allowedRequestAttributesPattern" value=".*"/> -->
+ <property name="secretRequired" value="false"/>
+ <!-- <property name="allowedRequestAttributesPattern" value=".*"/> -->
<!-- commented out because the values match the Tomcat defaults:
<property name="tomcatAuthentication" value="true"/>
<property name="allowTrace" value="false"/>
[ofbiz-framework] 04/04: Fixed: Remote Code Execution (File Upload) Vulnerability (OFBIZ-11948)
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch release22.01
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 8691b1a7e168e773711f0d9972345179e8d357ee
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Thu Feb 10 13:58:31 2022 +0100
Fixed: Remote Code Execution (File Upload) Vulnerability (OFBIZ-11948)
This fixes and improves file upload.
Refactor a bit SecuredUpload class and fixes:
Prevent double extensions
Check extensions
Improves getMimeTypeFromFileName() by checking is file exists
In isValidImageFile() finally use isValidText() bypassing encoding
Completely refactor deniedWebShellTokens in security.properties by re-adding
removed tokens in last commit and adding several new ones. I have still to check
encoded, encrypted and obfuscated webshells)
Modifies SecurityUtilTest::webShellTokensTesting accordingly
While at it better format commonsImagingSupportedFormats, deniedFileExtensions
and imagejSupportedFormats properties for legibility.
Check if createAnonFile service is used in GroovyBaseScript.groovy and if a
complete file name is used (file exist) check, using SecuredUpload, extensions
and prevent double extensions, actually check all the file and stop there.
Adds some "Check if a webshell is not uploaded" comments in
ContentManagementServices.java
DataServices.java
ScaleImage.java
FrameImage.java
ImageManagementServices.java
ProductServices.java
HttpRequestFileUpload.java
ProgramExport.groovy
Trivial comments fixes in catalog.properties
Also fixes OFBIZ 12571 (I use here a space because of pending INFRA 22843)
by simply adding processbuilder to deniedWebShellTokens
---
.../ofbiz/content/ContentManagementServices.java | 3 +
.../apache/ofbiz/content/data/DataServices.java | 1 +
applications/product/config/catalog.properties | 4 +-
.../org/apache/ofbiz/product/image/ScaleImage.java | 2 +
.../ofbiz/product/imagemanagement/FrameImage.java | 3 +-
.../imagemanagement/ImageManagementServices.java | 4 +-
.../ofbiz/product/product/ProductServices.java | 2 +
.../ofbiz/base/util/HttpRequestFileUpload.java | 1 +
framework/security/config/security.properties | 23 ++++--
.../org/apache/ofbiz/security/SecuredUpload.java | 82 +++++++++++++++-------
.../apache/ofbiz/security/SecurityUtilTest.java | 41 +++++++++--
.../ofbiz/service/engine/GroovyBaseScript.groovy | 15 +++-
.../groovyScripts/entity/ProgramExport.groovy | 12 +---
13 files changed, 138 insertions(+), 55 deletions(-)
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 980ad55..a9b2133 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
@@ -1594,6 +1594,9 @@ public class ContentManagementServices {
File file = new File(objectInfo);
if (file.isFile()) {
try {
+ // Check if a webshell is not uploaded
+ // This should now be useless after the add of SecuredUpload::isValidFile in GroovyBaseScript for createAnonFile service.
+ // But I prefer to keep it anyway, it's hard to test all cases, better safe than sorry
if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(objectInfo, "All", delegator)) {
errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedFileFormatsIncludingSvg", locale);
}
diff --git a/applications/content/src/main/java/org/apache/ofbiz/content/data/DataServices.java b/applications/content/src/main/java/org/apache/ofbiz/content/data/DataServices.java
index bb75397..5dd75a7 100644
--- a/applications/content/src/main/java/org/apache/ofbiz/content/data/DataServices.java
+++ b/applications/content/src/main/java/org/apache/ofbiz/content/data/DataServices.java
@@ -460,6 +460,7 @@ public class DataServices {
if (UtilValidate.isNotEmpty(textData)) {
try (OutputStreamWriter out = new OutputStreamWriter(new FileOutputStream(file), StandardCharsets.UTF_8);) {
out.write(textData);
+ // Check if a webshell is not uploaded
if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(file.getAbsolutePath(), "Text", delegator)) {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedTextFileFormats", locale);
return ServiceUtil.returnError(errorMessage);
diff --git a/applications/product/config/catalog.properties b/applications/product/config/catalog.properties
index 64244a3..f43ddc6 100644
--- a/applications/product/config/catalog.properties
+++ b/applications/product/config/catalog.properties
@@ -21,7 +21,7 @@
# -- Image upload path on the server
-#FIXME the image server path need to be move on runtime
+#FIXME the image server path need to be moved on runtime
image.server.path=${sys:getProperty('ofbiz.home')}/themes/common/images/webapp/images/${tenantId}
# -- The prefix to put on auto-generated image urls (can be relative or absolute, whatever you want)
@@ -37,7 +37,7 @@ all.product.category=CATALOG1
reactivate.product.from.receipt=Y
# Image upload path on the image management
-#FIXME the image management path need to be move on runtime
+#FIXME the image management path need to be moved on runtime
image.management.path=${sys:getProperty('ofbiz.home')}/themes/common/webapp/images/products/management
image.management.url=/images/products/management
image.management.nameofthumbnail=-100
diff --git a/applications/product/src/main/java/org/apache/ofbiz/product/image/ScaleImage.java b/applications/product/src/main/java/org/apache/ofbiz/product/image/ScaleImage.java
index cb8587d..92ac0fa 100644
--- a/applications/product/src/main/java/org/apache/ofbiz/product/image/ScaleImage.java
+++ b/applications/product/src/main/java/org/apache/ofbiz/product/image/ScaleImage.java
@@ -224,6 +224,7 @@ public class ScaleImage {
try {
String fileToCheck = imageServerPath + "/" + newFileLocation + "." + imgExtension;
ImageIO.write(bufNewImg, imgExtension, new File(fileToCheck));
+ // Check if a webshell is not uploaded
if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(fileToCheck, "Image", delegator)) {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
@@ -402,6 +403,7 @@ public class ScaleImage {
try {
String fileToCheck = imageServerPath + "/" + newFileLocation + "." + imgExtension;
ImageIO.write(bufNewImg, imgExtension, new File(fileToCheck));
+ // Check if a webshell is not uploaded
if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(fileToCheck, "Image", delegator)) {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
diff --git a/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java b/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java
index cd72740..d1873d1 100644
--- a/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java
+++ b/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java
@@ -290,7 +290,7 @@ public class FrameImage {
if (UtilValidate.isEmpty(imageName) || UtilValidate.isEmpty(imageData)) {
session.setAttribute("frameContentId", request.getParameter("frameExistContentId"));
session.setAttribute("frameDataResourceId", request.getParameter("frameExistDataResourceId"));
- request.setAttribute("_ERROR_MESSAGE_", "There is no frame image, please select the image type *.PNG uploading.");
+ request.setAttribute("_ERROR_MESSAGE_", "There is no frame image, please select the image type *.PNG to upload.");
return "error";
}
if (!"image/png".equals(mimType)) {
@@ -320,6 +320,7 @@ public class FrameImage {
}
Path tmpFile = Files.createTempFile(null, null);
Files.write(tmpFile, imageData.array(), StandardOpenOption.APPEND);
+ // Check if a webshell is not uploaded
if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(tmpFile.toString(), "Image", delegator)) {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedFileFormatsIncludingSvg", locale);
request.setAttribute("_ERROR_MESSAGE_", errorMessage);
diff --git a/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/ImageManagementServices.java b/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/ImageManagementServices.java
index 5468d73..59168fb 100644
--- a/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/ImageManagementServices.java
+++ b/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/ImageManagementServices.java
@@ -157,10 +157,10 @@ public class ImageManagementServices {
}
if (UtilValidate.isEmpty(imageResize)) {
- // check file content
try {
Path tempFile = Files.createTempFile(null, null);
Files.write(tempFile, imageData.array(), StandardOpenOption.APPEND);
+ // Check if a webshell is not uploaded
if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(), "Image", delegator)) {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
@@ -189,6 +189,7 @@ public class ImageManagementServices {
try {
Path tempFile = Files.createTempFile(null, null);
Files.write(tempFile, imageData.array(), StandardOpenOption.APPEND);
+ // Check if a webshell is not uploaded
if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(), "Image", delegator)) {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
@@ -579,6 +580,7 @@ public class ImageManagementServices {
try {
Path tempFile = Files.createTempFile(null, null);
Files.write(tempFile, imageData.array(), StandardOpenOption.APPEND);
+ // Check if a webshell is not uploaded
if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(), "Image", delegator)) {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
diff --git a/applications/product/src/main/java/org/apache/ofbiz/product/product/ProductServices.java b/applications/product/src/main/java/org/apache/ofbiz/product/product/ProductServices.java
index fe02b45..8a628fd 100644
--- a/applications/product/src/main/java/org/apache/ofbiz/product/product/ProductServices.java
+++ b/applications/product/src/main/java/org/apache/ofbiz/product/product/ProductServices.java
@@ -1079,6 +1079,7 @@ public class ProductServices {
try {
Path tempFile = Files.createTempFile(null, null);
Files.write(tempFile, imageData.array(), StandardOpenOption.APPEND);
+ // Check if a webshell is not uploaded
if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(), "Image", delegator)) {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
@@ -1386,6 +1387,7 @@ public class ProductServices {
try {
Path tempFile = Files.createTempFile(null, null);
Files.write(tempFile, imageData.array(), StandardOpenOption.APPEND);
+ // Check if a webshell is not uploaded
if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(), "Image", delegator)) {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/HttpRequestFileUpload.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/HttpRequestFileUpload.java
index e3734a6..48135bd 100644
--- a/framework/base/src/main/java/org/apache/ofbiz/base/util/HttpRequestFileUpload.java
+++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/HttpRequestFileUpload.java
@@ -270,6 +270,7 @@ public class HttpRequestFileUpload {
}
fos.flush();
+ // Check if a webshell is not uploaded
if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(fileTocheck, fileType, delegator)) {
return false;
}
diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties
index 0b25cff..b0d76c8 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -212,12 +212,22 @@ templateClassResolver=
#-- For supported formats see https://commons.apache.org/proper/commons-imaging/formatsupport.html
#-- Notably https://commons.apache.org/proper/commons-imaging/formatsupport.html#Metadata_Format_Support
#-- OOTB OFBiz only supports PNG, GIF, TIFF and JPEG, it's a breeze to extend using more:
-#-- commonsImagingSupportedFormats=BMP,GIF,JPEG/JFIF,ICNS,ICO/CUR,PCX/DCX,PNM/PGM/PBM/PPM/PAMPortablePixmap,PNG,PSD/Photoshop,RGBE/RadianceHDR,TIFF,WBMP,XBM,XPM
+#-- commonsImagingSupportedFormats=BMP,GIF,JPEG/JFIF,ICNS,ICO/CUR,PCX/DCX,PNM/PGM/PBM/PPM/PAMPortablePixmap,PNG,PSD/Photoshop,RGBE/RadianceHDR,\
+ TIFF,WBMP,XBM,XPM
#-- You should then modify SupportedImageFormats label.
#--
#-- If you want to get more image formats then use imageJ:
#-- For imagejSupportedFormats see https://imagejdocu.tudor.lu/faq/general/which_file_formats_are_supported_by_imagej. NOTE: plugins support is important here
-#-- imagejSupportedFormats=TIFF(.tiff,.tif),JPEG(.jpeg,.jpg),BMP(.bmp),FITS(.fits),PGM(.pgm),PPM(.ppm),PBM(.pbm),GIF(.gif),AnimatedGIF(.gif),PNG(.png),DICOM(.dic,.dcm,.dicom),PICT(.pict,.pic,.pct),PSD(.psd),TGA(.tga),ICO(.ico),CUR(.cur),Sunraster(.sun),XBM(.xbm),XPM(.xpm),PCX(.pcx),ANALYZE,NIfTi,AHF(.ahf),SPE(.spe),PIC(.pic),LeicaTIFF(.tiff,.lei),Quicktime(.pic,.mov),AVI(.avi),PDS(.pds),LSM(.lsm),RAW,ISAC,FluoViewTIFF(.tiff),FluoviewFV1000OIB(.oib),FluoviewFV1000OIF(.oif,.tif,-ro.pty,.lu [...]
+#-- imagejSupportedFormats=TIFF(.tiff,.tif),JPEG(.jpeg,.jpg),BMP(.bmp),FITS(.fits),PGM(.pgm),PPM(.ppm),PBM(.pbm),GIF(.gif),AnimatedGIF(.gif),\
+ PNG(.png),DICOM(.dic,.dcm,.dicom),PICT(.pict,.pic,.pct),PSD(.psd),TGA(.tga),ICO(.ico),CUR(.cur),Sunraster(.sun),\
+ XBM(.xbm),XPM(.xpm),PCX(.pcx),ANALYZE,NIfTi,AHF(.ahf),SPE(.spe),PIC(.pic),LeicaTIFF(.tiff,.lei),Quicktime(.pic,.mov),\
+ AVI(.avi),PDS(.pds),LSM(.lsm),RAW,ISAC,FluoViewTIFF(.tiff),FluoviewFV1000OIB(.oib),\
+ FluoviewFV1000OIF(.oif,.tif,-ro.pty,.lut,.bmp),IPLAB(.ipl),BrukerNMR(.fid,.ser,.2dseq,.2rr,.2ii,.3rrr,.3iii),FDF(.fdf),\
+ VFF(.vff),SIF(.sif),AxioVisionZVI(.zvi),DM3(.dm3),Deltavision(.dv,.r3d),MI,NII,NIII,IMG(.img),UNC,PerkinElmer(.tif,.tim,\
+ .zpo,.csv,.htm,.ano,.rec,.cfg,.2,.3,.4,.5,.6,.7,.8,\u2026),EPS(.eps,.epsi),SEQ(.seq),IPW(.ipw),OpenLabLIFF(.liff),\
+ OpenLabRAW(.raw),Metamorph(.stk),ICS(.ics,.ids),LeicaLif(.lif),Imaris(.ims),OME-XML(.ome),OME-TIFF(.tiff),\
+ ABD-TIFF(.tiff),GEL(.gel),Nikon(.nef,.tiff),Slidebook(.sld),SPCImage(.sdt),AL3D(.al3d),ND2(.nd2),μManager(.tif,.txt),\
+ MRC(.mrc),JPEG2000(.jp2),MNG(.mng),Flex(.flex),NRRD(.nrrd,.nhdr),VIFFbitmapimage(.xv),ROI(.roi),ERS(.ers),RS(.rs),HPGL
#--
#-- PDFBox and PDFReader are used for PDF files
#--
@@ -226,7 +236,8 @@ templateClassResolver=
#--
#-- List of denied files suffixes to be uploaded
#-- OFBiz of course also check contents...
-deniedFileExtensions=html,htm,php,php2,hph3,php4,php5,asp,aspx,ascx,jsp,jspx,cfm,cfc,bat,exe,com,dll,vbs,js,reg,cgi,htaccess,asis,sh,phtm,shtm,inc,asp,cdx,asa,cer,py,pl,shtml,hta,ps1
+deniedFileExtensions=html,htm,php,php1,php2,hph3,php4,php5,asp,aspx,asa,asax,ascx,ashx,asmx,jsp,jspa,jspx,jsw,jsv,jspf,jtml,cfm,cfc,bat,exe,com,dll,\
+ vbs,js,reg,cgi,htaccess,asis,sh,phtm,shtm,inc,asp,cdx,asa,cer,py,pl,shtml,hta,ps1,tag
#--
#-- The upload vulnerability is only a post-auth (needs a credential with suitable permissions),
#-- people may like to allow more than what is allowed OOTB
@@ -238,8 +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=<%,<jsp,<?,#!,freemarker,<script,javascript,eval,<body>,<form,\
- chmod,mkdir,fopen,fclose,new file,import,upload,getfilename,download,getoutputstring,readfile
+deniedWebShellTokens=freemarker,<script,javascript,<body,<form,<jsp:,scriptlet>,declaration>,expression>,<c:out,taglib,<prefix,<%@ page,\
+ %eval,@eval,runtime,import,passthru,shell_exec,assert,str_rot13,system,base64_decode,include,\
+ chmod,mkdir,fopen,fclose,new file,upload,getfilename,download,getoutputstring,readfile,\
+ python,perl ,/perl,ruby ,/ruby,processbuilder
#-- IMPORTANT: when you change things here you need to do accordingly in SecurityUtilTest::webShellTokensTesting and run "gradlew test" --
#-- Popup last-visited time from database after user has logged in.
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 60f99cc..f595f80 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
@@ -62,6 +62,7 @@ import org.apache.commons.imaging.formats.png.PngImageParser;
import org.apache.commons.imaging.formats.tiff.TiffImageParser;
import org.apache.commons.io.FileUtils;
import org.apache.commons.io.FilenameUtils;
+import org.apache.commons.lang.StringUtils;
import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.FileUtil;
import org.apache.ofbiz.base.util.StringUtil;
@@ -89,45 +90,53 @@ import com.lowagie.text.pdf.PdfReader;
public class SecuredUpload {
+ // To check if a webshell is not uploaded
+
// This can be helpful:
// https://en.wikipedia.org/wiki/File_format
// https://en.wikipedia.org/wiki/List_of_file_signatures
// See also information in security.properties:
- // Line #-- UPLOAD: supported file formats are *safe* PNG, GIF, TIFF, JPEG, PDF, Audio and Video and ZIP
+ // Supported file formats are *safe* PNG, GIF, TIFF, JPEG, PDF, Audio, Video, Text, and ZIP
private static final String MODULE = SecuredUpload.class.getName();
private static final List<String> DENIEDFILEEXTENSIONS = deniedFileExtensions();
private static final List<String> DENIEDWEBSHELLTOKENS = deniedWebShellTokens();
- /**
- * @param fileToCheck
- * @param fileType
- * @return true if the file is valid
- * @throws IOException
- * @throws ImageReadException
- */
- public static boolean isValidFile(String fileToCheck, String fileType, Delegator delegator) throws IOException, ImageReadException {
+ public static boolean isValidText(String content, List<String> allowed) throws IOException {
+ return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(content, token, allowed));
+ }
+ private static boolean isValidFileName(String fileToCheck, Delegator delegator) throws IOException {
+ // Allow all
if (("true".equalsIgnoreCase(EntityUtilProperties.getPropertyValue("security", "allowAllUploads", delegator)))) {
return true;
}
+ // Prevent double extensions
+ if (StringUtils.countMatches(fileToCheck, ".") > 1) {
+ return false;
+ }
+
String imageServerUrl = EntityUtilProperties.getPropertyValue("catalog", "image.management.url", delegator);
Path p = Paths.get(fileToCheck);
boolean wrongFile = true;
+
+ // Check extensions
if (p != null && p.getFileName() != null) {
String fileName = p.getFileName().toString(); // The file name is the farthest element from the root in the directory hierarchy.
- if (DENIEDFILEEXTENSIONS.contains(FilenameUtils.getExtension(fileToCheck))) {
+ if (DENIEDFILEEXTENSIONS.contains(FilenameUtils.getExtension(fileToCheck).toLowerCase())) {
Debug.logError("This file extension is not allowed for security reason", MODULE);
deleteBadFile(fileToCheck);
return false;
}
+ // Check the file and path names
if (org.apache.commons.lang3.SystemUtils.IS_OS_WINDOWS) {
// More about that: https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
if (fileToCheck.length() > 259) {
Debug.logError("Uploaded file name too long", MODULE);
} else if (p.toString().contains(imageServerUrl.replaceAll("/", "\\\\"))) {
+ // TODO check this is still useful in at least 1 case
if (fileName.matches("[a-zA-Z0-9-_ ()]{1,249}.[a-zA-Z0-9-_ ]{1,10}")) { // "(" and ")" for duplicates files
wrongFile = false;
}
@@ -138,6 +147,7 @@ public class SecuredUpload {
if (fileToCheck.length() > 4096) {
Debug.logError("Uploaded file name too long", MODULE);
} else if (p.toString().contains(imageServerUrl)) {
+ // TODO check this is still useful in at least 1 case
if (fileName.matches("[a-zA-Z0-9-_ ()]{1,4086}.[a-zA-Z0-9-_ ]{1,10}")) { // "(" and ")" for duplicates files
wrongFile = false;
}
@@ -156,7 +166,23 @@ public class SecuredUpload {
deleteBadFile(fileToCheck);
return false;
}
+ return true;
+ }
+
+ /**
+ * @param fileToCheck
+ * @param fileType
+ * @return true if the file is valid
+ * @throws IOException
+ * @throws ImageReadException
+ */
+ public static boolean isValidFile(String fileToCheck, String fileType, Delegator delegator) throws IOException, ImageReadException {
+
+ if (!isValidFileName(fileToCheck, delegator)) {
+ return false;
+ }
+ // Check the the file content
if (isExecutable(fileToCheck)) {
deleteBadFile(fileToCheck);
return false;
@@ -188,7 +214,7 @@ public class SecuredUpload {
break;
case "AllButCompressed":
- if (isValidTextFile(fileToCheck)
+ if (isValidTextFile(fileToCheck, true)
|| isValidImageIncludingSvgFile(fileToCheck)
|| isValidPdfFile(fileToCheck)) {
return true;
@@ -199,7 +225,7 @@ public class SecuredUpload {
// The philosophy for isValidTextFile() is that
// we can't presume of all possible text contents used for attacks with payloads
// At least there is an easy way to prevent them in isValidTextFile
- if (isValidTextFile(fileToCheck)) {
+ if (isValidTextFile(fileToCheck, true)) {
return true;
}
break;
@@ -216,7 +242,7 @@ public class SecuredUpload {
break;
default: // All
- if (isValidTextFile(fileToCheck)
+ if (isValidTextFile(fileToCheck, true)
|| isValidImageIncludingSvgFile(fileToCheck)
|| isValidCompressedFile(fileToCheck, delegator)
|| isValidAudioFile(fileToCheck)
@@ -245,7 +271,7 @@ public class SecuredUpload {
|| imageFormat.equals(ImageFormats.TIFF)
|| imageFormat.equals(ImageFormats.JPEG))
&& imageMadeSafe(fileName)
- && isValidText(fileName, new ArrayList<>());
+ && isValidTextFile(fileName, false);
}
/**
@@ -371,7 +397,7 @@ public class SecuredUpload {
} catch (IOException e) {
return false;
}
- return isValidTextFile(fileName); // Validate content to prevent webshell
+ return isValidTextFile(fileName, true); // Validate content to prevent webshell
}
return false;
}
@@ -485,8 +511,11 @@ public class SecuredUpload {
*/
private static String getMimeTypeFromFileName(String fileName) throws IOException {
File file = new File(fileName);
- Tika tika = new Tika();
- return tika.detect(file);
+ if (file.exists()) {
+ Tika tika = new Tika();
+ return tika.detect(file);
+ }
+ return null;
}
private static boolean isValidDirectoryInCompressedFile(String folderName, Delegator delegator) throws IOException, ImageReadException {
@@ -602,26 +631,25 @@ public class SecuredUpload {
/**
* Does this text file contains a Freemarker Server-Side Template Injection (SSTI) using freemarker.template.utility.Execute? Etc.
* @param fileName must be an UTF-8 encoded text file
+ * @param encodedContent TODO
* @return true if the text file does not contains a Freemarker SSTI
* @throws IOException
*/
- private static boolean isValidTextFile(String fileName) throws IOException {
+ private static boolean isValidTextFile(String fileName, Boolean encodedContent) throws IOException {
Path filePath = Paths.get(fileName);
byte[] bytesFromFile = Files.readAllBytes(filePath);
- try {
- Charset.availableCharsets().get("UTF-8").newDecoder().decode(ByteBuffer.wrap(bytesFromFile));
- } catch (CharacterCodingException e) {
- return false;
+ if (encodedContent) {
+ try {
+ Charset.availableCharsets().get("UTF-8").newDecoder().decode(ByteBuffer.wrap(bytesFromFile));
+ } catch (CharacterCodingException e) {
+ return false;
+ }
}
String content = new String(bytesFromFile);
ArrayList<String> allowed = new ArrayList<>();
return isValidText(content, allowed);
}
- public static boolean isValidText(String content, List<String> allowed) throws IOException {
- return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(content, token, allowed));
- }
-
private static boolean isValid(String content, String string, List<String> allowed) {
return !content.toLowerCase().contains(string) || allowed.contains(string);
}
@@ -629,7 +657,7 @@ public class SecuredUpload {
private static void deleteBadFile(String fileToCheck) {
Debug.logError("File :" + fileToCheck + ", can't be uploaded for security reason", MODULE);
File badFile = new File(fileToCheck);
- if (!badFile.delete()) {
+ if (badFile.exists() && !badFile.delete()) {
Debug.logError("File :" + fileToCheck + ", couldn't be deleted", MODULE);
}
}
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 fe4207a..e7302bf 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
@@ -58,6 +58,12 @@ public class SecurityUtilTest {
@Test
public void webShellTokensTesting() {
+ // Currently used
+ // freemarker,<script,javascript,<body,<form,<jsp:,scriptlet>,declaration>,expression>,<c:out,taglib,<prefix,<%@ page
+ // %eval,@eval,runtime,import,passthru,shell_exec,assert,str_rot13,system,base64_decode,include
+ // chmod,mkdir,fopen,fclose,new file,upload,getfilename,download,getoutputstring,readfile
+ // python,perl ,/perl,ruby ,/ruby,processbuilder
+
try {
List<String> allowed = new ArrayList<>();
allowed.add("getfilename");
@@ -65,29 +71,50 @@ public class SecurityUtilTest {
allowed = new ArrayList<>();
assertFalse(SecuredUpload.isValidText("hack.getFileName", allowed));
- assertFalse(SecuredUpload.isValidText("<%", allowed));
- assertFalse(SecuredUpload.isValidText("<jsp", allowed));
- assertFalse(SecuredUpload.isValidText("<?", allowed));
- assertFalse(SecuredUpload.isValidText("#!", allowed));
assertFalse(SecuredUpload.isValidText("freemarker", allowed));
assertFalse(SecuredUpload.isValidText("<script", allowed));
assertFalse(SecuredUpload.isValidText("javascript", allowed));
+ assertFalse(SecuredUpload.isValidText("<body", allowed));
+ assertFalse(SecuredUpload.isValidText("<form", allowed));
+ assertFalse(SecuredUpload.isValidText("<jsp:", allowed));
+ assertFalse(SecuredUpload.isValidText("scriptlet>", allowed));
+ assertFalse(SecuredUpload.isValidText("declaration>", allowed));
+ assertFalse(SecuredUpload.isValidText("expression>", allowed));
+ assertFalse(SecuredUpload.isValidText("<c:out", allowed));
+ assertFalse(SecuredUpload.isValidText("taglib", allowed));
+ assertFalse(SecuredUpload.isValidText("<prefix", allowed));
+ assertFalse(SecuredUpload.isValidText("<%@ page", allowed));
+
assertFalse(SecuredUpload.isValidText("%eval", allowed));
assertFalse(SecuredUpload.isValidText("@eval", allowed));
- assertFalse(SecuredUpload.isValidText("<body>", allowed));
- assertFalse(SecuredUpload.isValidText("<form", allowed));
+ assertFalse(SecuredUpload.isValidText("runtime", allowed));
assertFalse(SecuredUpload.isValidText("import", allowed));
+ assertFalse(SecuredUpload.isValidText("passthru", allowed));
+ assertFalse(SecuredUpload.isValidText("shell_exec", allowed));
+ assertFalse(SecuredUpload.isValidText("assert", allowed));
+ assertFalse(SecuredUpload.isValidText("str_rot13", allowed));
+ assertFalse(SecuredUpload.isValidText("system", allowed));
+ assertFalse(SecuredUpload.isValidText("base64_decode", allowed));
+ assertFalse(SecuredUpload.isValidText("include", allowed));
+
assertFalse(SecuredUpload.isValidText("chmod", allowed));
assertFalse(SecuredUpload.isValidText("mkdir", allowed));
assertFalse(SecuredUpload.isValidText("fopen", allowed));
assertFalse(SecuredUpload.isValidText("fclose", allowed));
assertFalse(SecuredUpload.isValidText("new file", allowed));
- assertFalse(SecuredUpload.isValidText("import", allowed));
assertFalse(SecuredUpload.isValidText("upload", allowed));
assertFalse(SecuredUpload.isValidText("getfilename", allowed));
assertFalse(SecuredUpload.isValidText("download", allowed));
assertFalse(SecuredUpload.isValidText("getoutputstring", allowed));
assertFalse(SecuredUpload.isValidText("readfile", allowed));
+
+ assertFalse(SecuredUpload.isValidText("python", allowed));
+ assertFalse(SecuredUpload.isValidText("perl ", allowed));
+ assertFalse(SecuredUpload.isValidText("/perl", allowed));
+ assertFalse(SecuredUpload.isValidText("ruby ", allowed));
+ assertFalse(SecuredUpload.isValidText("/ruby", allowed));
+ assertFalse(SecuredUpload.isValidText("processbuilder", allowed)); // Groovy
+
} catch (IOException e) {
fail(String.format("IOException occured : %s", e.getMessage()));
}
diff --git a/framework/service/src/main/groovy/org/apache/ofbiz/service/engine/GroovyBaseScript.groovy b/framework/service/src/main/groovy/org/apache/ofbiz/service/engine/GroovyBaseScript.groovy
index 50f6b9f..377b05d 100644
--- a/framework/service/src/main/groovy/org/apache/ofbiz/service/engine/GroovyBaseScript.groovy
+++ b/framework/service/src/main/groovy/org/apache/ofbiz/service/engine/GroovyBaseScript.groovy
@@ -20,13 +20,13 @@ package org.apache.ofbiz.service.engine
import org.apache.ofbiz.base.util.Debug
import org.apache.ofbiz.base.util.UtilProperties
+import org.apache.ofbiz.entity.GenericValue
import org.apache.ofbiz.entity.util.EntityQuery
import org.apache.ofbiz.service.DispatchContext
+import org.apache.ofbiz.service.ExecutionServiceException
import org.apache.ofbiz.service.LocalDispatcher
import org.apache.ofbiz.service.ModelService
import org.apache.ofbiz.service.ServiceUtil
-import org.apache.ofbiz.service.ExecutionServiceException
-import org.apache.ofbiz.entity.GenericValue
abstract class GroovyBaseScript extends Script {
public static final String module = GroovyBaseScript.class.getName()
@@ -53,6 +53,17 @@ abstract class GroovyBaseScript extends Script {
? this.binding.getVariable('locale')
: this.binding.getVariable('parameters').locale
}
+ if (serviceName.equals("createAnonFile")) {
+ String dataResourceName = inputMap.get("dataResourceName")
+ File file = new File(dataResourceName)
+ if (file.exists()) {
+ // Check if a webshell is not uploaded
+ if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(dataResourceName, "All", delegator)) {
+ String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedFileFormatsIncludingSvg", inputMap.locale)
+ throw new ExecutionServiceException(errorMessage)
+ }
+ }
+ }
Map serviceContext = dctx.makeValidContext(serviceName, ModelService.IN_PARAM, inputMap)
Map result = dispatcher.runSync(serviceName, serviceContext)
if (ServiceUtil.isError(result)) {
diff --git a/framework/webtools/groovyScripts/entity/ProgramExport.groovy b/framework/webtools/groovyScripts/entity/ProgramExport.groovy
index 0d3a5f7..52a7a03 100644
--- a/framework/webtools/groovyScripts/entity/ProgramExport.groovy
+++ b/framework/webtools/groovyScripts/entity/ProgramExport.groovy
@@ -16,19 +16,10 @@
* specific language governing permissions and limitations
* under the License.
*/
-import org.apache.ofbiz.entity.Delegator
import org.apache.ofbiz.entity.GenericValue
-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
import org.codehaus.groovy.control.CompilerConfiguration
import org.codehaus.groovy.control.MultipleCompilationErrorsException
-import org.codehaus.groovy.control.ErrorCollector
+import org.codehaus.groovy.control.customizers.ImportCustomizer
String groovyProgram = null
recordValues = []
@@ -86,6 +77,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"])) {
request.setAttribute("_ERROR_MESSAGE_", "Not executed for security reason")
return