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
The following commit(s) were added to refs/heads/trunk by this push:
new 71dbb3c Fixed: CVE-2021-37608 vulnerability bypass (OFBIZ-12307)
71dbb3c is described below
commit 71dbb3c824f0b76f8616e0ab11c5fe1e4a79ae4c
Author: Jacques Le Roux <jacques.le.r...@les7arts.com>
AuthorDate: Fri Sep 17 11:29:43 2021 +0200
Fixed: CVE-2021-37608 vulnerability bypass (OFBIZ-12307)
Fixes a number of issues I spotted while working on OFBIZ-12305 in relation
with
OFBIZ-12055
The last change I made for OFBIZ-12305 was incomplete, the files could not
be
checked by SecuredUpload because they did not exist! This concerns
ImageManagementServices, DataServices, FrameImage and ProductServices
classes.
I used Files::createTempFile to fix that.
Also fixes a bug in SecuredUpload where I reversed the check on
fileToCheck.length in Windows case. I also added a comment, Windows 10 now
allows more length (need an OS parameter change though)
Finally, creates public SecuredUpload::isValidText to be used in OFBIZ-12305
---
.../org/apache/ofbiz/content/data/DataServices.java | 13 +++++++++++--
.../ofbiz/product/imagemanagement/FrameImage.java | 8 +++++++-
.../imagemanagement/ImageManagementServices.java | 21 +++++++++++++++++----
.../ofbiz/product/product/ProductServices.java | 8 +++++++-
.../org/apache/ofbiz/security/SecuredUpload.java | 6 +++++-
5 files changed, 47 insertions(+), 9 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 8589096..bb75397 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
@@ -28,6 +28,9 @@ import java.io.StringWriter;
import java.io.Writer;
import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
import java.sql.Timestamp;
import java.util.Arrays;
import java.util.HashMap;
@@ -263,10 +266,13 @@ public class DataServices {
} else if (binData != null) {
try {
// Check if a webshell is not uploaded
- if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(file.getAbsolutePath(),
"All", delegator)) {
+ Path tempFile = Files.createTempFile(null, null);
+ Files.write(tempFile, binData.array(),
StandardOpenOption.APPEND);
+ if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(),
"All", delegator)) {
String errorMessage =
UtilProperties.getMessage("SecurityUiLabels",
"SupportedFileFormatsIncludingSvg", locale);
return ServiceUtil.returnError(errorMessage);
}
+ Files.delete(tempFile);
RandomAccessFile out = new RandomAccessFile(file, "rw");
out.write(binData.array());
out.close();
@@ -466,10 +472,13 @@ public class DataServices {
} else if (binData != null) {
try {
// Check if a webshell is not uploaded
- if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(file.getAbsolutePath(),
"All", delegator)) {
+ Path tempFile = Files.createTempFile(null, null);
+ Files.write(tempFile, binData.array(),
StandardOpenOption.APPEND);
+ if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(),
"Image", delegator)) {
String errorMessage =
UtilProperties.getMessage("SecurityUiLabels",
"SupportedFileFormatsIncludingSvg", locale);
return ServiceUtil.returnError(errorMessage);
}
+ Files.delete(tempFile);
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/FrameImage.java
b/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java
index 6d9a86b..cd72740 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
@@ -31,6 +31,9 @@ import java.io.File;
import java.io.IOException;
import java.io.RandomAccessFile;
import java.nio.ByteBuffer;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
import java.util.HashMap;
import java.util.Locale;
import java.util.Map;
@@ -315,11 +318,14 @@ public class FrameImage {
request.setAttribute("_ERROR_MESSAGE_", "There is an existing
frame, please select from the existing frame.");
return "error";
}
- if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(file.getAbsolutePath(),
"Image", delegator)) {
+ Path tmpFile = Files.createTempFile(null, null);
+ Files.write(tmpFile, imageData.array(), StandardOpenOption.APPEND);
+ if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(tmpFile.toString(),
"Image", delegator)) {
String errorMessage =
UtilProperties.getMessage("SecurityUiLabels",
"SupportedFileFormatsIncludingSvg", locale);
request.setAttribute("_ERROR_MESSAGE_", errorMessage);
return "error";
}
+ Files.delete(tmpFile);
RandomAccessFile out = new RandomAccessFile(file, "rw");
out.write(imageData.array());
out.close();
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 5a194aa..5468d73 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
@@ -27,6 +27,9 @@ import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.RandomAccessFile;
import java.nio.ByteBuffer;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
@@ -154,12 +157,16 @@ public class ImageManagementServices {
}
if (UtilValidate.isEmpty(imageResize)) {
- // Create image file original to folder product id.
+ // check file content
try {
- if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(file.toString(), "Image",
delegator)) {
+ Path tempFile = Files.createTempFile(null, null);
+ Files.write(tempFile, imageData.array(),
StandardOpenOption.APPEND);
+ if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(),
"Image", delegator)) {
String errorMessage =
UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
}
+ Files.delete(tempFile);
+ // Create image file original to folder product id.
RandomAccessFile out = new RandomAccessFile(file, "rw");
out.write(imageData.array());
out.close();
@@ -180,10 +187,13 @@ public class ImageManagementServices {
fileOriginal = checkExistsImage(fileOriginal);
try {
- if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(fileOriginal.toString(),
"Image", delegator)) {
+ Path tempFile = Files.createTempFile(null, null);
+ Files.write(tempFile, imageData.array(),
StandardOpenOption.APPEND);
+ if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(),
"Image", delegator)) {
String errorMessage =
UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
}
+ Files.delete(tempFile);
RandomAccessFile outFile = new
RandomAccessFile(fileOriginal, "rw");
outFile.write(imageData.array());
outFile.close();
@@ -567,10 +577,13 @@ public class ImageManagementServices {
String fileToCheck = imageServerPath + "/" + productId + "/" +
filenameToUseThumb;
File fileOriginalThumb = new File(fileToCheck);
try {
- if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(fileToCheck, "Image",
delegator)) {
+ Path tempFile = Files.createTempFile(null, null);
+ Files.write(tempFile, imageData.array(),
StandardOpenOption.APPEND);
+ if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(),
"Image", delegator)) {
String errorMessage =
UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
}
+ Files.delete(tempFile);
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 ea2f4ac..122c307 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
@@ -24,6 +24,9 @@ import java.io.IOException;
import java.io.RandomAccessFile;
import java.math.BigDecimal;
import java.nio.ByteBuffer;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
import java.sql.Timestamp;
import java.util.Arrays;
import java.util.HashMap;
@@ -1378,10 +1381,13 @@ public class ProductServices {
File file = new File(fileToCheck);
try {
- if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(fileToCheck, "Image",
delegator)) {
+ Path tempFile = Files.createTempFile(null, null);
+ Files.write(tempFile, imageData.array(),
StandardOpenOption.APPEND);
+ if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(),
"Image", delegator)) {
String errorMessage =
UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
return ServiceUtil.returnError(errorMessage);
}
+ Files.delete(tempFile);
RandomAccessFile out = new RandomAccessFile(file, "rw");
out.write(imageData.array());
out.close();
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 30928b9..682cab6 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
@@ -123,7 +123,7 @@ public class SecuredUpload {
}
if (org.apache.commons.lang3.SystemUtils.IS_OS_WINDOWS) {
- if (fileToCheck.length() < 259) {
+ 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("/",
"\\\\"))) {
if (fileName.matches("[a-zA-Z0-9-_ ()]{1,249}.[a-zA-Z0-9-_
]{1,10}")) { // "(" and ")" for duplicates files
@@ -609,6 +609,10 @@ public class SecuredUpload {
return false;
}
String content = new String(bytesFromFile);
+ return isValidText(content);
+ }
+
+ public static boolean isValidText(String content) throws IOException {
return !(content.toLowerCase().contains("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...
