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:14 UTC
[ofbiz-framework] 04/04: Fixed: Remote Code Execution (File Upload) Vulnerability (OFBIZ-11948)
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch release18.12
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 0a25c932ac0b9f7e4f498b9c6cd5eb314901b66d
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
Conflicts handled by hand
framework/security/config/security.properties
framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
framework/service/src/main/groovy/org/apache/ofbiz/service/engine/GroovyBaseScript.groovy
framework/webtools/groovyScripts/entity/ProgramExport.groovy (reverted)
---
.../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 | 25 +++++--
.../org/apache/ofbiz/security/SecuredUpload.java | 69 +++++++++++++------
.../ofbiz/service/engine/GroovyBaseScript.groovy | 79 ++++++++++++++++++----
11 files changed, 147 insertions(+), 46 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 39d7b9d..c185266 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
@@ -1638,6 +1638,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 dd263bf..2ff1215 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
@@ -215,6 +215,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);
@@ -383,6 +384,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 c35e93c..4385b5a 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
@@ -282,7 +282,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)) {
@@ -312,6 +312,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 56346f6..e087e94 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
@@ -155,10 +155,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);
@@ -187,6 +187,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);
@@ -567,6 +568,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 3a9aa55..2436100 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
@@ -1057,6 +1057,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);
@@ -1361,6 +1362,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 c359c95..5cb1168 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
@@ -235,6 +235,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 cf49080..e8085fe 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -179,12 +179,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
#--
@@ -193,7 +203,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
@@ -205,9 +216,11 @@ 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.
#-- So users can know of any unauthorised access to their accounts.
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 53cf774..3d45685 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,44 +90,51 @@ 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);
String fileName = p.getFileName().toString(); // The file name is the farthest element from the root in the directory hierarchy.
boolean wrongFile = true;
-
+
+ // Check extensions
if (DENIEDFILEEXTENSIONS.contains(FilenameUtils.getExtension(fileToCheck))) {
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) {
if (fileToCheck.length() > 259) { // More about that: https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
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;
}
@@ -137,6 +145,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;
}
@@ -154,7 +163,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;
@@ -186,7 +211,7 @@ public class SecuredUpload {
break;
case "AllButCompressed":
- if (isValidTextFile(fileToCheck)
+ if (isValidTextFile(fileToCheck, true)
|| isValidImageIncludingSvgFile(fileToCheck)
|| isValidPdfFile(fileToCheck)) {
return true;
@@ -197,7 +222,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;
@@ -214,7 +239,7 @@ public class SecuredUpload {
break;
default: // All
- if (isValidTextFile(fileToCheck)
+ if (isValidTextFile(fileToCheck, true)
|| isValidImageIncludingSvgFile(fileToCheck)
|| isValidCompressedFile(fileToCheck, delegator)
|| isValidAudioFile(fileToCheck)
@@ -243,7 +268,7 @@ public class SecuredUpload {
|| imageFormat.equals(ImageFormats.TIFF)
|| imageFormat.equals(ImageFormats.JPEG))
&& imageMadeSafe(fileName)
- && isValidText(fileName, new ArrayList<>());
+ && isValidTextFile(fileName, false);
}
/**
@@ -375,7 +400,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;
}
@@ -489,9 +514,12 @@ public class SecuredUpload {
*/
private static String getMimeTypeFromFileName(String fileName) throws IOException {
File file = new File(fileName);
+ if (file.exists()) {
Tika tika = new Tika();
return tika.detect(file);
}
+ return null;
+ }
private static boolean isValidDirectoryInCompressedFile(String folderName, Delegator delegator) throws IOException, ImageReadException {
File folder = new File(folderName);
@@ -606,26 +634,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);
+ 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);
}
@@ -633,7 +660,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/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 f1be9b0..41be1a9 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
@@ -21,20 +21,24 @@ package org.apache.ofbiz.service.engine
import java.util.Map
import org.apache.ofbiz.base.util.Debug
+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()
+ String getModule() {
+ return this.class.getName()
+ }
+
Map runService(String serviceName, Map inputMap) throws ExecutionServiceException {
- LocalDispatcher dispatcher = binding.getVariable('dispatcher');
- DispatchContext dctx = dispatcher.getDispatchContext();
+ LocalDispatcher dispatcher = binding.getVariable('dispatcher')
+ DispatchContext dctx = dispatcher.getDispatchContext()
if (!inputMap.userLogin) {
inputMap.userLogin = this.binding.hasVariable('userLogin')
? this.binding.getVariable('userLogin')
@@ -50,6 +54,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)) {
@@ -63,11 +78,11 @@ abstract class GroovyBaseScript extends Script {
}
Map makeValue(String entityName) throws ExecutionServiceException {
- return result = binding.getVariable('delegator').makeValue(entityName)
+ return binding.getVariable('delegator').makeValue(entityName)
}
Map makeValue(String entityName, Map inputMap) throws ExecutionServiceException {
- return result = binding.getVariable('delegator').makeValidValue(entityName, inputMap)
+ return binding.getVariable('delegator').makeValidValue(entityName, inputMap)
}
EntityQuery from(def entity) {
@@ -87,21 +102,35 @@ abstract class GroovyBaseScript extends Script {
return from(entityName).where(fields).cache(useCache).queryOne()
}
+ def success() {
+ return success(null, null)
+ }
def success(String message) {
+ return success(message, null)
+ }
+ def success(Map returnValues) {
+ return success(null, returnValues)
+ }
+ def success(String message, Map returnValues) {
// TODO: implement some clever i18n mechanism based on the userLogin and locale in the binding
if (this.binding.hasVariable('request')) {
// the script is invoked as an "event"
if (message) {
this.binding.getVariable('request').setAttribute("_EVENT_MESSAGE_", message)
}
+ if (returnValues) {
+ returnValues.each {
+ this.binding.getVariable('request').setAttribute(it.getKey(), it.getValue())
+ }
+ }
return 'success'
} else {
// the script is invoked as a "service"
- if (message) {
- return ServiceUtil.returnSuccess(message)
- } else {
- return ServiceUtil.returnSuccess()
+ Map result = message ? ServiceUtil.returnSuccess(message) : ServiceUtil.returnSuccess()
+ if (returnValues) {
+ result.putAll(returnValues)
}
+ return result
}
}
Map failure(String message) {
@@ -128,17 +157,37 @@ abstract class GroovyBaseScript extends Script {
}
}
}
+
def logInfo(String message) {
- Debug.logInfo(message, module)
+ Debug.logInfo(message, getModule())
}
def logWarning(String message) {
- Debug.logWarning(message, module)
+ Debug.logWarning(message, getModule())
}
def logError(String message) {
- Debug.logError(message, module)
+ Debug.logError(message, getModule())
+ }
+ def logError(Throwable t, String message) {
+ Debug.logError(t, message, getModule())
+ }
+ def logError(Throwable t) {
+ Debug.logError(t, null, getModule())
}
-
def logVerbose(String message) {
- Debug.logVerbose(message, module)
+ Debug.logVerbose(message, getModule())
+ }
+
+ def label(String ressource, String message) {
+ return label(ressource, message, null)
+ }
+ def label(String ressource, String message, Map context) {
+ Locale locale = this.binding.getVariable('locale')
+ if (!locale) {
+ locale = Locale.getDefault()
+ }
+ if (context) {
+ return UtilProperties.getMessage(ressource, message, context, locale)
+ }
+ return UtilProperties.getMessage(ressource, message, locale)
}
}