This is an automated email from the ASF dual-hosted git repository. jleroux pushed a commit to branch release24.09 in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit fd6a3b8644985f0dea78572782f3dfa0cdf8813d Author: Jacques Le Roux <[email protected]> AuthorDate: Wed Sep 24 09:07:51 2025 +0200 Improved: Improve ImageManagementServices code (OFBIZ-13292) Better handling of PNG files --- dependencies.gradle | 1 + .../org/apache/ofbiz/security/SecuredUpload.java | 134 ++++++++++++++++++++- 2 files changed, 131 insertions(+), 4 deletions(-) diff --git a/dependencies.gradle b/dependencies.gradle index a7b487b163..b131e3ea57 100644 --- a/dependencies.gradle +++ b/dependencies.gradle @@ -17,6 +17,7 @@ * under the License. */ dependencies { + implementation 'com.drewnoakes:metadata-extractor:2.19.0' implementation 'com.github.ben-manes.caffeine:caffeine:3.1.8' implementation 'com.google.zxing:core:3.5.3' implementation 'com.googlecode.concurrentlinkedhashmap:concurrentlinkedhashmap-lru:1.4.2' 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 c6a9409349..d2ced1fcee 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 @@ -23,6 +23,7 @@ import java.awt.Graphics; import java.awt.Image; import java.awt.Transparency; import java.awt.image.BufferedImage; +import java.io.DataInputStream; import java.io.File; import java.io.FileInputStream; import java.io.IOException; @@ -39,13 +40,18 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.StandardOpenOption; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Base64; import java.util.Collection; +import java.util.Collections; import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Objects; import java.util.Set; import java.util.UUID; +import java.util.zip.DataFormatException; +import java.util.zip.Inflater; import javax.imageio.ImageIO; import javax.imageio.ImageReader; @@ -96,6 +102,10 @@ import org.mustangproject.ZUGFeRD.ZUGFeRDImporter; import org.w3c.dom.Document; import org.xml.sax.SAXException; +import com.drew.imaging.ImageMetadataReader; +import com.drew.imaging.ImageProcessingException; +import com.drew.metadata.Directory; +import com.drew.metadata.Tag; import com.lowagie.text.pdf.PdfReader; public class SecuredUpload { @@ -244,7 +254,7 @@ public class SecuredUpload { * @throws ImageReadException */ public static boolean isValidFile(String fileToCheck, String fileType, Delegator delegator) throws IOException, ImageReadException { - // Allow all + // Allow all uploads w/o check if (("true".equalsIgnoreCase(EntityUtilProperties.getPropertyValue("security", "allowAllUploads", delegator)))) { return true; } @@ -387,9 +397,17 @@ public class SecuredUpload { */ private static boolean imageMadeSafe(String fileName) { File file = new File(fileName); - boolean safeState = false; boolean fallbackOnApacheCommonsImaging; + if (!noWebshellInMetadata(file)) { + return false; + } + if (!noWebshellInPNG(file)) { + return false; + } + + boolean safeState = false; + if ((file != null) && file.exists() && file.canRead() && file.canWrite()) { try (OutputStream fos = Files.newOutputStream(file.toPath(), StandardOpenOption.WRITE)) { // Get the image format @@ -475,6 +493,114 @@ public class SecuredUpload { return safeState; } + private static boolean noWebshellInMetadata(File file) { + com.drew.metadata.Metadata metadata = null; + try { + metadata = ImageMetadataReader.readMetadata(file); + } catch (ImageProcessingException | IOException error) { + Debug.logError("================== Not saved for security reason ==================" + error, MODULE); + } + + for (Directory directory : metadata.getDirectories()) { + for (Tag tag : directory.getTags()) { + try { + if (!isValidText(tag.toString(), Collections.emptyList())) { + Debug.logError("================== Not saved for security reason ==================", MODULE); + return false; + } + } catch (IOException error) { + Debug.logError("================== Not saved for security reason ==================" + error, MODULE); + return false; + } + } + for (String error : directory.getErrors()) { + Debug.logError("================== Not saved for security reason ==================" + error, MODULE); + return false; + } + } + return true; + } + + private static boolean noWebshellInPNG(File file) { + try { + ImageIO.read(file); + if (!isPNG(file)) { + return true; // Not a PNG file, it's OK so far + } + } catch (IOException error) { + Debug.logError("================== Not saved for security reason ==================" + error, MODULE); + return false; + } + + try (DataInputStream stream = new DataInputStream(new FileInputStream(file));) { + byte[] data = new byte[8]; + stream.readFully(data); //Read PNG Header + while (true) { + data = new byte[4]; + stream.readFully(data); //Read Length + int length = ((data[0] & 0xFF) << 24) + | ((data[1] & 0xFF) << 16) + | ((data[2] & 0xFF) << 8) + | (data[3] & 0xFF); //Byte array to int + stream.readFully(data); //Read Name + String name = new String(data); //Byte array to String + if (name.equals("IDAT")) { + data = new byte[length]; + stream.readFully(data); //Read Data + return inflate(data); + } else { //Don't care about other chunks + data = new byte[length + 4]; //Data length + 4 byte CRC + stream.readFully(data); //Skip Data and CRC. + } + } + } catch (IOException error) { + Debug.logError("================== Not saved for security reason, wrong PNG IDAT (weird) ==================" + error, MODULE); + return false; + } + } + + private static boolean isPNG(File file) throws IOException { + Path filePath = Paths.get(file.getPath()); + byte[] bytesFromFile = Files.readAllBytes(filePath); + ImageFormat imageFormat = Imaging.guessFormat(bytesFromFile); + return (imageFormat.equals(ImageFormats.PNG)); + } + + private static boolean inflate(byte[] data) { + Inflater inflater = new Inflater(); + inflater.setInput(data); + byte[] result = new byte[data.length * 5]; // Inflating ratio max is 5/1 + try { + while (!inflater.finished()) { + int count = inflater.inflate(result); + if (count == 0) { + if (!inflater.needsInput()) { // Not everything read + inflater.inflate(result); + } else if (inflater.needsDictionary()) { // Dictionary to be loaded + inflater.setDictionary(result); + inflater.getAdler(); + } + } + } + if (inflater.getRemaining() > 0) { // There is more than image data in IDAT, check it + byte[] remaining = Arrays.copyOfRange(data, (int) inflater.getBytesRead(), (int) inflater.getBytesRead() + inflater.getRemaining()); + String toCheck = new String(remaining, "UTF-8"); + byte[] decoded = Base64.getDecoder().decode(toCheck); + String decodedStr = new String(decoded, StandardCharsets.UTF_8); + if (!isValidText(decodedStr, Collections.emptyList())) { + Debug.logError("================== Not saved for security reason ==================", MODULE); + inflater.end(); + return false; + } + } + } catch (DataFormatException | IOException error) { + Debug.logError("================== Not saved for security reason ==================" + error, MODULE); + inflater.end(); + return false; + } + return true; + } + /** * Is it a supported image format, including SVG? * @param fileName @@ -826,8 +952,8 @@ 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 + * @param encodedContent true id the file content is encoded + * @return true if the text file does not contains a Freemarker SSTI or other issues * @throws IOException */ private static boolean isValidTextFile(String fileName, Boolean encodedContent) throws IOException {
