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
The following commit(s) were added to refs/heads/release18.12 by this push:
new 668e801 Improved: Secure the uploads (OFBIZ-12080)
668e801 is described below
commit 668e801ada8ae020e6ccc80c0b27252275f829ed
Author: Jacques Le Roux <jacques.le.r...@les7arts.com>
AuthorDate: Sat Feb 19 11:37:43 2022 +0100
Improved: Secure the uploads (OFBIZ-12080)
In security.properties, adds some deniedFileExtensions and improves few
comments
(easier to merge later)
In DataServices::createFileNoPerm filters file upload to verify the
filename
before the content is checked if necessary
Manually merges SecuredUpload.java based on trunk
---
.../apache/ofbiz/content/data/DataServices.java | 32 +++++++++---
framework/security/config/security.properties | 17 ++++---
.../org/apache/ofbiz/security/SecuredUpload.java | 57 +++++++++++++---------
3 files changed, 68 insertions(+), 38 deletions(-)
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 5dd75a7..2bd71c1 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
@@ -49,6 +49,7 @@ import org.apache.ofbiz.entity.Delegator;
import org.apache.ofbiz.entity.GenericEntityException;
import org.apache.ofbiz.entity.GenericValue;
import org.apache.ofbiz.entity.util.EntityQuery;
+import org.apache.ofbiz.security.SecuredUpload;
import org.apache.ofbiz.service.DispatchContext;
import org.apache.ofbiz.service.GenericServiceException;
import org.apache.ofbiz.service.ModelService;
@@ -195,7 +196,15 @@ public class DataServices {
return createFileMethod(dctx, context);
}
- public static Map<String, Object> createFileNoPerm(DispatchContext dctx,
Map<String, ? extends Object> rcontext) {
+ public static Map<String, Object> createFileNoPerm(DispatchContext dctx,
Map<String, ? extends Object> rcontext) throws IOException,
+ ImageReadException {
+ String originalFileName = (String) rcontext.get("dataResourceName");
+ Delegator delegator = dctx.getDelegator();
+ Locale locale = (Locale) rcontext.get("locale");
+ if (!SecuredUpload.isValidFile(originalFileName, "All", delegator)) {
+ String errorMessage =
UtilProperties.getMessage("SecurityUiLabels", "SupportedTextFileFormats",
locale);
+ return ServiceUtil.returnError(errorMessage);
+ }
Map<String, Object> context = UtilMisc.makeMapWritable(rcontext);
context.put("skipPermissionCheck", "true");
return createFileMethod(dctx, context);
@@ -254,7 +263,9 @@ public class DataServices {
if (UtilValidate.isNotEmpty(textData)) {
try (OutputStreamWriter out = new OutputStreamWriter(new
FileOutputStream(file), StandardCharsets.UTF_8);) {
out.write(textData);
- if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(file.getAbsolutePath(),
"Text", delegator)) {
+ // Check if a webshell is not uploaded
+ // TODO I believe the call below to SecuredUpload::isValidFile
is now useless because of the same in createFileNoPerm
+ if (!SecuredUpload.isValidFile(file.getAbsolutePath(), "Text",
delegator)) {
String errorMessage =
UtilProperties.getMessage("SecurityUiLabels", "SupportedTextFileFormats",
locale);
return ServiceUtil.returnError(errorMessage);
}
@@ -265,10 +276,11 @@ public class DataServices {
}
} else if (binData != null) {
try {
- // Check if a webshell is not uploaded
Path tempFile = Files.createTempFile(null, null);
Files.write(tempFile, binData.array(),
StandardOpenOption.APPEND);
- if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(),
"All", delegator)) {
+ // Check if a webshell is not uploaded
+ // TODO I believe the call below to SecuredUpload::isValidFile
is now useless because of the same in createFileNoPerm
+ if (!SecuredUpload.isValidFile(tempFile.toString(), "All",
delegator)) {
String errorMessage =
UtilProperties.getMessage("SecurityUiLabels",
"SupportedFileFormatsIncludingSvg", locale);
return ServiceUtil.returnError(errorMessage);
}
@@ -461,7 +473,8 @@ public class DataServices {
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)) {
+ // TODO I believe the call below to
SecuredUpload::isValidFile is now useless because of the same in
createFileNoPerm
+ if (!SecuredUpload.isValidFile(file.getAbsolutePath(),
"Text", delegator)) {
String errorMessage =
UtilProperties.getMessage("SecurityUiLabels", "SupportedTextFileFormats",
locale);
return ServiceUtil.returnError(errorMessage);
}
@@ -473,9 +486,10 @@ public class DataServices {
} else if (binData != null) {
try {
// Check if a webshell is not uploaded
+ // TODO I believe the call below to
SecuredUpload::isValidFile is now useless because of the same in
createFileNoPerm
Path tempFile = Files.createTempFile(null, null);
Files.write(tempFile, binData.array(),
StandardOpenOption.APPEND);
- if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(),
"Image", delegator)) {
+ if (!SecuredUpload.isValidFile(tempFile.toString(),
"Image", delegator)) {
String errorMessage =
UtilProperties.getMessage("SecurityUiLabels",
"SupportedFileFormatsIncludingSvg", locale);
return ServiceUtil.returnError(errorMessage);
}
@@ -648,7 +662,8 @@ public class DataServices {
try (FileOutputStream out = new FileOutputStream(file);) {
out.write(imageData);
// Check if a webshell is not uploaded
- if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(file.getAbsolutePath(),
"All", delegator)) {
+ // TODO I believe the call below to SecuredUpload::isValidFile
is now useless because of the same in createFileNoPerm
+ if (!SecuredUpload.isValidFile(file.getAbsolutePath(), "All",
delegator)) {
String errorMessage =
UtilProperties.getMessage("SecurityUiLabels",
"SupportedFileFormatsIncludingSvg", locale);
return ServiceUtil.returnError(errorMessage);
}
@@ -707,7 +722,8 @@ public class DataServices {
try (FileOutputStream out = new FileOutputStream(file);) {
out.write(imageData);
// Check if a webshell is not uploaded
- if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(file.getAbsolutePath(),
"All", delegator)) {
+ // TODO I believe the call below to SecuredUpload::isValidFile
is now useless because of the same in createFileNoPerm
+ if (!SecuredUpload.isValidFile(file.getAbsolutePath(), "All",
delegator)) {
String errorMessage =
UtilProperties.getMessage("SecurityUiLabels",
"SupportedFileFormatsIncludingSvg", locale);
return ServiceUtil.returnError(errorMessage);
}
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index eb53037..b11ec6a 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -1,4 +1,4 @@
-###############################################################################
+##############################################################################
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
@@ -121,8 +121,9 @@ security.login.cert.allow=true
# -- pattern for the userlogin id in CN section of certificate
security.login.cert.pattern=^(\\w*\\s?\\w*)\\W*.*$
-# -- use Tomcat SingleSignOn valve
-# -- Tomcat SSO should not be used in a cluster: OFBIZ-10123
+# -- Use Tomcat SingleSignOn valve to allow single sign on (SSO) and single
log out (SLO).
+# -- Remember to set security.login.externalLoginKey.enabled to false when
using Tomcat SSO.
+# -- Note Tomcat SSO is not implemented for cluster as Tomcat
ClusterSingleSignOn is not used: OFBIZ-10123
security.login.tomcat.sso=false
# -- Hours after which EmailAdressVerification should expire
@@ -156,7 +157,11 @@ security.token.key=security.token.key
# -- no spaces after commas,no wildcard, can be extended of course...
host-headers-allowed=localhost,127.0.0.1,demo-trunk.ofbiz.apache.org,demo-stable.ofbiz.apache.org,demo-old.ofbiz.apache.org
-# -- By default the SameSite value in SameSiteFilter is strict. This allows to
change it to lax if needed
+# -- By default the SameSite value in SameSiteFilter is 'strict'.
+# -- This property allows to change to 'lax' if needed.
+# -- If you use 'lax' we recommend that you set
+# -- org.apache.ofbiz.security.CsrfDefenseStrategy
+# -- for csrf.defense.strategy (see below)
SameSiteCookieAttribute=
# -- Freemarker TemplateClassResolver option, see OFBIZ-11709.
@@ -203,8 +208,8 @@ templateClassResolver=
#--
#-- List of denied files suffixes to be uploaded
#-- OFBiz of course also check contents...
-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
+deniedFileExtensions=html,htm,php,php1,php2,hph3,php4,php5,php6,php7,phps,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,pht,phtml,shtm,inc,asp,cdx,asa,cer,py,pl,shtml,hta,ps1,tag,pgif,htaccess,phar,inc,cgi,wss,do,action
#--
#-- 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
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 35df8c5..364c066 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
@@ -101,19 +101,19 @@ public class SecuredUpload {
private static final String MODULE = SecuredUpload.class.getName();
private static final List<String> DENIEDFILEEXTENSIONS =
deniedFileExtensions();
private static final List<String> DENIEDWEBSHELLTOKENS =
deniedWebShellTokens();
- private static final Integer maxLineLength =
UtilProperties.getPropertyAsInteger("security", "maxLineLength", 10000);
+ private static final Integer MAXLINELENGTH =
UtilProperties.getPropertyAsInteger("security", "maxLineLength", 10000);
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 {
+ public static boolean isValidFileName(String fileToCheck, Delegator
delegator) throws IOException {
// Allow all
if
(("true".equalsIgnoreCase(EntityUtilProperties.getPropertyValue("security",
"allowAllUploads", delegator)))) {
return true;
}
- // Prevent double extensions
+ // Prevents double extensions
if (StringUtils.countMatches(fileToCheck, ".") > 1) {
Debug.logError("Double extensions are not allowed for security
reason", MODULE);
return false;
@@ -121,17 +121,30 @@ public class SecuredUpload {
// Check max line length, default 10000
if (!checkMaxLinesLength(fileToCheck)) {
- Debug.logError("For security reason lines over " +
maxLineLength.toString() + " are not allowed", MODULE);
+ Debug.logError("For security reason lines over " +
MAXLINELENGTH.toString() + " are not allowed", MODULE);
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))) {
+ 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.
+ String extension =
FilenameUtils.getExtension(fileToCheck).toLowerCase();
+ // Prevents null byte in filename
+ if (extension.contains("%00")
+ || extension.contains("%0a")
+ || extension.contains("%20")
+ || extension.contains("%0d%0a")
+ || extension.contains("/")
+ || extension.contains("./")
+ || extension.contains(".")) {
+ Debug.logError("Special bytes in extension are not allowed for
security reason", MODULE);
+ return false;
+ }
+ if (DENIEDFILEEXTENSIONS.contains(extension)) {
Debug.logError("This file extension is not allowed for security
reason", MODULE);
deleteBadFile(fileToCheck);
return false;
@@ -139,7 +152,8 @@ public class SecuredUpload {
// 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
+ // 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
@@ -161,6 +175,7 @@ public class SecuredUpload {
wrongFile = false;
}
}
+ }
if (wrongFile) {
Debug.logError("Uploaded file "
@@ -289,8 +304,9 @@ public class SecuredUpload {
File file = new File(fileName);
boolean safeState = false;
boolean fallbackOnApacheCommonsImaging;
- try {
+
if ((file != null) && file.exists() && file.canRead() &&
file.canWrite()) {
+ try (OutputStream fos = Files.newOutputStream(file.toPath(),
StandardOpenOption.WRITE)) {
// Get the image format
String formatName;
ImageInputStream iis = ImageIO.createImageInputStream(file);
@@ -340,7 +356,7 @@ public class SecuredUpload {
Graphics bg = sanitizedImage.getGraphics();
bg.drawImage(initialSizedImage, 0, 0, null);
bg.dispose();
- OutputStream fos = Files.newOutputStream(file.toPath(),
StandardOpenOption.WRITE);
+
if (!fallbackOnApacheCommonsImaging) {
ImageIO.write(sanitizedImage, formatName, fos);
} else {
@@ -348,36 +364,29 @@ public class SecuredUpload {
// Handle only formats for which Apache Commons Imaging
can successfully write (YES in Write column of the reference link)
// the image format. See reference link in the class header
switch (formatName) {
- case "TIFF": {
+ case "TIFF":
imageParser = new TiffImageParser();
break;
- }
- case "GIF": {
+ case "GIF":
imageParser = new GifImageParser();
break;
- }
- case "PNG": {
+ case "PNG":
imageParser = new PngImageParser();
break;
- }
- case "JPEG": {
+ case "JPEG":
imageParser = new JpegImageParser();
break;
- }
- default: {
+ default:
throw new IOException("Format of the original image "
+ fileName + " is not supported for write operation !");
}
- }
imageParser.writeImage(sanitizedImage, fos, new
HashMap<>());
}
// Set state flag
- fos.close(); // This was not correctly handled in the
document-upload-protection example, and I did not spot it :/
safeState = true;
- }
} catch (IOException | ImageReadException | ImageWriteException e) {
- safeState = false;
Debug.logWarning(e, "Error during Image file " + fileName + "
processing !", MODULE);
}
+ }
return safeState;
}
@@ -693,7 +702,7 @@ public class SecuredUpload {
File file = new File(fileToCheck);
List<String> lines = FileUtils.readLines(file,
Charset.defaultCharset());
for (String line : lines) {
- if (line.length() > maxLineLength) {
+ if (line.length() > MAXLINELENGTH) {
return false;
}
}
