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 <jacques.le.r...@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
