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 2024/03/12 09:32:13 UTC
(ofbiz-framework) branch trunk updated (891dc348fb -> f62f177533)
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a change to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
from 891dc348fb Fixed: Test next FreeMarker release (OFBIZ-12934)
new d5d5a37940 Fixed: Prevent possible file race condition with SecuredUpload class (OFBIZ-12937)
new f62f177533 Fixed: SecuredUpload::checkMaxLinesLength put in a regression for PDF files (OFBIZ-12936)
The 2 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:
.../apache/ofbiz/content/data/DataServices.java | 6 +++--
.../imagemanagement/ImageManagementServices.java | 9 ++++---
.../ofbiz/product/product/ProductServices.java | 6 +++--
.../org/apache/ofbiz/security/SecuredUpload.java | 30 +++++++++++++++++++---
4 files changed, 40 insertions(+), 11 deletions(-)
(ofbiz-framework) 02/02: Fixed: SecuredUpload::checkMaxLinesLength put in a regression for PDF files (OFBIZ-12936)
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit f62f17753327c9ae249260a363d1b823842a39c1
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Tue Mar 12 10:26:59 2024 +0100
Fixed: SecuredUpload::checkMaxLinesLength put in a regression for PDF files (OFBIZ-12936)
As commented at https://s.apache.org/wpkuj, while working on OFBIZ-12926
I realised that checkMaxLinesLength put in a possible regression for PDF files.
As a summary: we can't use checkMaxLinesLength for PDF files because they may
contain several CharSet encodings. Hence no possibility to use
Files::readAllLines that needs a sole CharSet. So we need to bypass this check.
---
.../org/apache/ofbiz/security/SecuredUpload.java | 30 +++++++++++++++++++---
1 file changed, 26 insertions(+), 4 deletions(-)
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 38a3ce93a3..b61c780960 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
@@ -233,10 +233,14 @@ public class SecuredUpload {
// Check the file content
- // Check max line length, default 10000
- if (!checkMaxLinesLength(fileToCheck)) {
- Debug.logError("For security reason lines over " + MAXLINELENGTH.toString() + " are not allowed", MODULE);
- return false;
+ // Check max line length, default 10000.
+ // PDF files are not concerned because they may contain several CharSet encodings
+ // hence no possibility to use Files::readAllLines that needs a sole CharSet
+ if (!isPdfFile(fileToCheck)) {
+ if (!checkMaxLinesLength(fileToCheck)) {
+ Debug.logError("For security reason lines over " + MAXLINELENGTH.toString() + " are not allowed", MODULE);
+ return false;
+ }
}
if (isExecutable(fileToCheck)) {
@@ -464,6 +468,24 @@ public class SecuredUpload {
return false;
}
+ /**
+ * @param fileName
+ * @return true if it's a PDF file
+ */
+ private static boolean isPdfFile(String fileName) {
+ File file = new File(fileName);
+ try {
+ if (Objects.isNull(file) || !file.exists()) {
+ return false;
+ }
+ // Load stream in PDF parser
+ PdfReader reader = new PdfReader(file.getAbsolutePath());
+ return true;
+ } catch (Exception e) {
+ return false;
+ }
+ }
+
/**
* @param fileName
* @return true if it's a safe PDF file: is PDF and does not contains embedded files
(ofbiz-framework) 01/02: Fixed: Prevent possible file race condition with SecuredUpload class (OFBIZ-12937)
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit d5d5a379404a2ab458bed5b92f3c0c4fa7c50638
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Tue Mar 12 08:06:58 2024 +0100
Fixed: Prevent possible file race condition with SecuredUpload class (OFBIZ-12937)
While trying to upload PDF files I found a possible file race condition in
DataServices::createFileMethod and DataServices::updateFileMethod
The same could exist in several places in ImageManagementServices and
ProductServices classes
As this file is a temporary file, the ideal fix is to replace File::delete by
File::deleteOnExit
---
.../main/java/org/apache/ofbiz/content/data/DataServices.java | 6 ++++--
.../ofbiz/product/imagemanagement/ImageManagementServices.java | 9 ++++++---
.../java/org/apache/ofbiz/product/product/ProductServices.java | 6 ++++--
3 files changed, 14 insertions(+), 7 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 96a4b3f99d..23a7f37669 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
@@ -299,7 +299,8 @@ public class DataServices {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedFileFormatsIncludingSvg", locale);
return ServiceUtil.returnError(errorMessage);
}
- Files.delete(tempFile);
+ File tempFileToDelete = new File(tempFile.toString());
+ tempFileToDelete.deleteOnExit();
RandomAccessFile out = new RandomAccessFile(file, "rw");
out.write(binData.array());
out.close();
@@ -508,7 +509,8 @@ public class DataServices {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedFileFormatsIncludingSvg", locale);
return ServiceUtil.returnError(errorMessage);
}
- Files.delete(tempFile);
+ File tempFileToDelete = new File(tempFile.toString());
+ tempFileToDelete.deleteOnExit();
RandomAccessFile out = new RandomAccessFile(file, "rw");
out.setLength(binData.array().length);
out.write(binData.array());
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 59168fb7be..a3f44bf3ed 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
@@ -165,7 +165,8 @@ public class ImageManagementServices {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
}
- Files.delete(tempFile);
+ File tempFileToDelete = new File(tempFile.toString());
+ tempFileToDelete.deleteOnExit();
// Create image file original to folder product id.
RandomAccessFile out = new RandomAccessFile(file, "rw");
out.write(imageData.array());
@@ -194,7 +195,8 @@ public class ImageManagementServices {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
}
- Files.delete(tempFile);
+ File tempFileToDelete = new File(tempFile.toString());
+ tempFileToDelete.deleteOnExit();
RandomAccessFile outFile = new RandomAccessFile(fileOriginal, "rw");
outFile.write(imageData.array());
outFile.close();
@@ -585,7 +587,8 @@ public class ImageManagementServices {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
}
- Files.delete(tempFile);
+ File tempFileToDelete = new File(tempFile.toString());
+ tempFileToDelete.deleteOnExit();
RandomAccessFile outFileThumb = new RandomAccessFile(fileOriginalThumb, "rw");
outFileThumb.write(imageData.array());
outFileThumb.close();
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 8a628fd792..5999991364 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
@@ -1084,7 +1084,8 @@ public class ProductServices {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
}
- Files.delete(tempFile);
+ File tempFileToDelete = new File(tempFile.toString());
+ tempFileToDelete.deleteOnExit();
RandomAccessFile out = new RandomAccessFile(fileToCheck, "rw");
out.write(imageData.array());
out.close();
@@ -1392,7 +1393,8 @@ public class ProductServices {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
}
- Files.delete(tempFile);
+ File tempFileToDelete = new File(tempFile.toString());
+ tempFileToDelete.deleteOnExit();
RandomAccessFile out = new RandomAccessFile(file, "rw");
out.write(imageData.array());
out.close();