From 3a46e266ea01575324dd86adcbd9dcbca4319893 Mon Sep 17 00:00:00 2001 From: Jacopo Cappellato Date: Mon, 4 May 2026 10:04:05 +0200 Subject: [PATCH 1/3] Fixed: Path traversal via productId in ImageManagementServices The productId request parameter was concatenated directly into filesystem paths across 7 service methods in ImageManagementServices without any path traversal validation. Added Paths.normalize().startsWith() guards in all affected methods (addMultipleuploadForProduct, removeImageFileForImageManagement, scaleImageMangementInAllSize, createContentThumbnail, createNewImageThumbnail, resizeImageOfProduct, renameImage), following the same pattern already in use in ProductServices.java. --- .../ImageManagementServices.java | 61 ++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-) 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 2a33775197c..25993c5976a 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 @@ -28,6 +28,7 @@ import java.nio.ByteBuffer; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; import java.util.HashMap; @@ -92,6 +93,14 @@ public static Map addMultipleuploadForProduct(DispatchContext dc "image.management.path", delegator), context); String imageServerUrl = FlexibleStringExpander.expandString(EntityUtilProperties.getPropertyValue("catalog", "image.management.url", delegator), context); + // Guard against path traversal via productId + Path imageServerNormalizedPath = Paths.get(imageServerPath).normalize(); + Path resolvedProductDir = Paths.get(imageServerPath, productId).normalize(); + if (!resolvedProductDir.startsWith(imageServerNormalizedPath)) { + Debug.logError("Path traversal attempt detected in image management upload, productId: " + productId, MODULE); + return ServiceUtil.returnError(UtilProperties.getMessage(RES_ERROR, + "ProductImageViewUnableWriteFile", UtilMisc.toMap("fileName", resolvedProductDir.toString()), locale)); + } String rootTargetDirectory = imageServerPath; File rootTargetDir = new File(rootTargetDirectory); if (!rootTargetDir.exists()) { @@ -306,11 +315,20 @@ public static Map removeImageFileForImageManagement(DispatchCont String contentId = (String) context.get("contentId"); String dataResourceName = (String) context.get("dataResourceName"); Delegator delegator = dctx.getDelegator(); + Locale locale = (Locale) context.get("locale"); try { if (UtilValidate.isNotEmpty(contentId)) { String imageServerPath = FlexibleStringExpander.expandString(EntityUtilProperties.getPropertyValue("catalog", "image.management.path", delegator), context); + // Guard against path traversal via productId or dataResourceName + Path imageServerNormalizedPath = Paths.get(imageServerPath).normalize(); + Path resolvedFilePath = Paths.get(imageServerPath, productId, dataResourceName).normalize(); + if (!resolvedFilePath.startsWith(imageServerNormalizedPath)) { + Debug.logError("Path traversal attempt detected in image management remove, productId: " + productId, MODULE); + return ServiceUtil.returnError(UtilProperties.getMessage(RES_ERROR, + "ProductImageViewUnableWriteFile", UtilMisc.toMap("fileName", resolvedFilePath.toString()), locale)); + } File file = new File(imageServerPath + "/" + productId + "/" + dataResourceName); if (!file.delete()) { Debug.logError("File :" + file.getName() + ", couldn't be deleted", MODULE); @@ -369,7 +387,16 @@ private static Map scaleImageMangementInAllSize(DispatchContext "image.management.path", dctx.getDelegator()), context); String imageServerUrl = FlexibleStringExpander.expandString(EntityUtilProperties.getPropertyValue("catalog", "image.management.url", dctx.getDelegator()), context); - + // Guard against path traversal via productId + Path imageServerNormalizedPath = Paths.get(imageServerPath).normalize(); + Path resolvedProductDir = Paths.get(imageServerPath, productId).normalize(); + if (!resolvedProductDir.startsWith(imageServerNormalizedPath)) { + Debug.logError("Path traversal attempt detected in image management scale, productId: " + productId, MODULE); + String errMsg = UtilProperties.getMessage(RES_ERROR, + "ProductImageViewUnableWriteFile", UtilMisc.toMap("fileName", resolvedProductDir.toString()), locale); + result.put(ModelService.ERROR_MESSAGE, errMsg); + return result; + } /* get original BUFFERED IMAGE */ resultBufImgMap.putAll(ImageTransform.getBufferedImage(imageServerPath + "/" + productId + "/" + filenameToUse, locale)); @@ -536,6 +563,14 @@ public static Map createContentThumbnail(DispatchContext dctx, M "image.management.path", delegator), context); String nameOfThumb = FlexibleStringExpander.expandString(EntityUtilProperties.getPropertyValue("catalog", "image.management.nameofthumbnail", delegator), context); + // Guard against path traversal via productId + Path imageServerNormalizedPath = Paths.get(imageServerPath).normalize(); + Path resolvedProductDir = Paths.get(imageServerPath, productId).normalize(); + if (!resolvedProductDir.startsWith(imageServerNormalizedPath)) { + Debug.logError("Path traversal attempt detected in image management thumbnail, productId: " + productId, MODULE); + return ServiceUtil.returnError(UtilProperties.getMessage(RES_ERROR, + "ProductImageViewUnableWriteFile", UtilMisc.toMap("fileName", resolvedProductDir.toString()), locale)); + } // Create content for thumbnail Map contentThumb = new HashMap<>(); @@ -729,6 +764,14 @@ public static Map createNewImageThumbnail(DispatchContext dctx, String contentId = (String) context.get("contentId"); String dataResourceName = (String) context.get("dataResourceName"); String width = (String) context.get("sizeWidth"); + // Guard against path traversal via productId + Path imageServerNormalizedPath = Paths.get(imageServerPath).normalize(); + Path resolvedProductDir = Paths.get(imageServerPath, productId).normalize(); + if (!resolvedProductDir.startsWith(imageServerNormalizedPath)) { + Debug.logError("Path traversal attempt detected in create new image thumbnail, productId: " + productId, MODULE); + return ServiceUtil.returnError(UtilProperties.getMessage(RES_ERROR, + "ProductImageViewUnableWriteFile", UtilMisc.toMap("fileName", resolvedProductDir.toString()), locale)); + } String imageType = ".jpg"; int resizeWidth = Integer.parseInt(width); int resizeHeight = resizeWidth; @@ -801,6 +844,14 @@ public static Map resizeImageOfProduct(DispatchContext dctx, Map String width = (String) context.get("resizeWidth"); int resizeWidth = Integer.parseInt(width); int resizeHeight = resizeWidth; + // Guard against path traversal via productId + Path imageServerNormalizedPath = Paths.get(imageServerPath).normalize(); + Path resolvedProductDir = Paths.get(imageServerPath, productId).normalize(); + if (!resolvedProductDir.startsWith(imageServerNormalizedPath)) { + Debug.logError("Path traversal attempt detected in resize image, productId: " + productId, MODULE); + return ServiceUtil.returnError(UtilProperties.getMessage(RES_ERROR, + "ProductImageViewUnableWriteFile", UtilMisc.toMap("fileName", resolvedProductDir.toString()), locale)); + } try { BufferedImage bufImg = ImageIO.read(new File(imageServerPath + "/" + productId + "/" + dataResourceName)); @@ -831,6 +882,14 @@ public static Map renameImage(DispatchContext dctx, Map Date: Mon, 4 May 2026 10:15:32 +0200 Subject: [PATCH 2/3] Fixed: Inspect all IDAT chunks and reject trailing bytes in noWebshellInPNG The previous implementation returned after the first IDAT chunk, leaving any payload in subsequent IDAT chunks or bytes appended after IEND unreachable. PNG (RFC 2083) explicitly permits multiple IDAT chunks whose data must be concatenated before decompression. The loop now accumulates all IDAT chunks into a single buffer and terminates on IEND. After IEND, any trailing bytes cause the file to be rejected. inflate() is called once on the fully concatenated payload, as the spec requires. --- .../apache/ofbiz/security/SecuredUpload.java | 47 ++++++++++++------- 1 file changed, 30 insertions(+), 17 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 0e043b34abf..ee0e98b1282 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.Image; import java.awt.Transparency; import java.awt.image.BufferedImage; +import java.io.ByteArrayOutputStream; import java.io.DataInputStream; import java.io.File; import java.io.FileInputStream; @@ -712,27 +713,39 @@ private static boolean noWebshellInPNG(File file) { return false; } - try (DataInputStream stream = new DataInputStream(new FileInputStream(file));) { - byte[] data = new byte[8]; - stream.readFully(data); //Read PNG Header + try (DataInputStream stream = new DataInputStream(new FileInputStream(file))) { + byte[] header = new byte[8]; + stream.readFully(header); // Read PNG signature + ByteArrayOutputStream idatBuffer = new ByteArrayOutputStream(); + byte[] nameBuf = new byte[4]; 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 + byte[] lenBuf = new byte[4]; + stream.readFully(lenBuf); // Read chunk length + int length = ((lenBuf[0] & 0xFF) << 24) + | ((lenBuf[1] & 0xFF) << 16) + | ((lenBuf[2] & 0xFF) << 8) + | (lenBuf[3] & 0xFF); + stream.readFully(nameBuf); // Read chunk type + String name = new String(nameBuf); 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. + byte[] chunkData = new byte[length]; + stream.readFully(chunkData); // Read data + idatBuffer.write(chunkData); + stream.readFully(new byte[4]); // Skip CRC + } else if (name.equals("IEND")) { + stream.readFully(new byte[4]); // Skip CRC + break; // IEND marks end of PNG datastream + } else { + stream.readFully(new byte[length + 4]); // Skip data and CRC } } + // Reject any bytes appended after IEND + if (stream.read() != -1) { + Debug.logError("================== Not saved for security reason, PNG has trailing bytes after IEND ==================", MODULE); + return false; + } + // Inflate all concatenated IDAT chunks + return inflate(idatBuffer.toByteArray()); } catch (IOException error) { Debug.logError("================== Not saved for security reason, wrong PNG IDAT (weird) ==================" + error, MODULE); return false; From 076d5842439734af6c28fbc6afe15ebfb747a1e9 Mon Sep 17 00:00:00 2001 From: Jacopo Cappellato Date: Mon, 4 May 2026 10:29:36 +0200 Subject: [PATCH 3/3] Improved: Explicitly reject JPEG and GIF uploads with trailing bytes The imageMadeSafe() re-encoding pipeline already strips trailing bytes for all formats, but JPEG and GIF uploads containing a webshell payload appended after their respective terminators were silently sanitized rather than explicitly rejected. Added noWebshellInJPEG() and noWebshellInGIF(), called from imageMadeSafe() alongside the existing noWebshellInPNG() check. noWebshellInJPEG() walks the JPEG marker structure from SOI, handling entropy-coded scan data with byte stuffing, and rejects the file if any bytes follow the EOI (FF D9) marker. noWebshellInGIF() parses the GIF block structure (extensions, image descriptors, and their sub-block chains) and rejects the file if any bytes follow the Trailer (0x3B). --- .../apache/ofbiz/security/SecuredUpload.java | 158 ++++++++++++++++++ 1 file changed, 158 insertions(+) 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 ee0e98b1282..8a1e72c74ba 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 @@ -582,6 +582,12 @@ private static boolean imageMadeSafe(String fileName) { if (!noWebshellInPNG(file)) { return false; } + if (!noWebshellInJPEG(file)) { + return false; + } + if (!noWebshellInGIF(file)) { + return false; + } boolean safeState = false; @@ -752,6 +758,158 @@ private static boolean noWebshellInPNG(File file) { } } + private static boolean noWebshellInJPEG(File file) { + try { + byte[] bytes = Files.readAllBytes(file.toPath()); + if (!Imaging.guessFormat(bytes).equals(ImageFormats.JPEG)) { + return true; // Not a JPEG file, it's OK so far + } + // SOI marker check + if (bytes.length < 4 || (bytes[0] & 0xFF) != 0xFF || (bytes[1] & 0xFF) != 0xD8) { + Debug.logError("================== Not saved for security reason, malformed JPEG ==================", MODULE); + return false; + } + int pos = 2; + while (pos < bytes.length) { + if ((bytes[pos] & 0xFF) != 0xFF) { + Debug.logError("================== Not saved for security reason, malformed JPEG marker ==================", MODULE); + return false; + } + // Skip 0xFF fill bytes (valid marker padding per JPEG spec) + while (pos < bytes.length && (bytes[pos] & 0xFF) == 0xFF) { + pos++; + } + if (pos >= bytes.length) { + Debug.logError("================== Not saved for security reason, JPEG missing EOI ==================", MODULE); + return false; + } + int marker = bytes[pos++] & 0xFF; + if (marker == 0xD9) { + // EOI — reject any trailing bytes + if (pos != bytes.length) { + Debug.logError("================ Not saved for security reason, JPEG has trailing bytes after EOI ================", MODULE); + return false; + } + return true; + } else if (marker >= 0xD0 && marker <= 0xD8) { + // SOI (0xD8) and RST0–RST7 (0xD0–0xD7) — no length field + continue; + } else if (marker == 0xDA) { + // SOS: length-prefixed header followed by entropy-coded scan data + if (pos + 2 > bytes.length) return false; + int len = ((bytes[pos] & 0xFF) << 8) | (bytes[pos + 1] & 0xFF); + if (len < 2 || pos + len > bytes.length) return false; + pos += len; // Skip SOS header + // Scan entropy-coded data, respecting byte stuffing (FF 00) and restart markers + while (pos < bytes.length - 1) { + if ((bytes[pos] & 0xFF) == 0xFF) { + int next = bytes[pos + 1] & 0xFF; + if (next == 0x00 || (next >= 0xD0 && next <= 0xD7)) { + pos += 2; // Stuffed 0xFF or RST — part of scan data + } else { + break; // Real marker — stop scanning scan data + } + } else { + pos++; + } + } + } else { + // Regular length-delimited segment + if (pos + 2 > bytes.length) return false; + int len = ((bytes[pos] & 0xFF) << 8) | (bytes[pos + 1] & 0xFF); + if (len < 2 || pos + len > bytes.length) return false; + pos += len; + } + } + Debug.logError("================== Not saved for security reason, JPEG missing EOI ==================", MODULE); + return false; + } catch (IOException error) { + Debug.logError("================== Not saved for security reason ==================" + error, MODULE); + return false; + } + } + + private static boolean noWebshellInGIF(File file) { + try { + byte[] bytes = Files.readAllBytes(file.toPath()); + if (!Imaging.guessFormat(bytes).equals(ImageFormats.GIF)) { + return true; // Not a GIF file, it's OK so far + } + // Header: "GIF87a" or "GIF89a" + if (bytes.length < 13) return false; + String gifHeader = new String(bytes, 0, 6, StandardCharsets.US_ASCII); + if (!"GIF87a".equals(gifHeader) && !"GIF89a".equals(gifHeader)) { + Debug.logError("================== Not saved for security reason, malformed GIF ==================", MODULE); + return false; + } + int pos = 6; + // Logical Screen Descriptor: packed byte at offset 4 within LSD + int packed = bytes[pos + 4] & 0xFF; + boolean hasGCT = (packed & 0x80) != 0; + int gctSize = packed & 0x07; + pos += 7; + // Skip Global Color Table + if (hasGCT) { + int gctBytes = 3 * (1 << (gctSize + 1)); + if (pos + gctBytes > bytes.length) return false; + pos += gctBytes; + } + // Parse blocks until Trailer + while (pos < bytes.length) { + int blockType = bytes[pos++] & 0xFF; + if (blockType == 0x3B) { + // Trailer — reject any trailing bytes + if (pos != bytes.length) { + Debug.logError("=============== Not saved for security reason, GIF has trailing bytes after Trailer ===============", MODULE); + return false; + } + return true; + } else if (blockType == 0x21) { + // Extension: label byte + sub-blocks + if (pos >= bytes.length) return false; + pos++; // Skip extension label + pos = skipGIFSubBlocks(bytes, pos); + if (pos < 0) return false; + } else if (blockType == 0x2C) { + // Image Descriptor: 9 bytes + if (pos + 9 > bytes.length) return false; + int imagePacked = bytes[pos + 8] & 0xFF; + boolean hasLCT = (imagePacked & 0x80) != 0; + int lctSize = imagePacked & 0x07; + pos += 9; + if (hasLCT) { + int lctBytes = 3 * (1 << (lctSize + 1)); + if (pos + lctBytes > bytes.length) return false; + pos += lctBytes; + } + pos++; // LZW minimum code size + pos = skipGIFSubBlocks(bytes, pos); + if (pos < 0) return false; + } else { + Debug.logError("================== Not saved for security reason, unknown GIF block type ==================", MODULE); + return false; + } + } + Debug.logError("================== Not saved for security reason, GIF missing Trailer ==================", MODULE); + return false; + } catch (IOException error) { + Debug.logError("================== Not saved for security reason ==================" + error, MODULE); + return false; + } + } + + private static int skipGIFSubBlocks(byte[] bytes, int pos) { + while (pos < bytes.length) { + int blockSize = bytes[pos++] & 0xFF; + if (blockSize == 0) { + return pos; // Block Terminator + } + pos += blockSize; + if (pos > bytes.length) return -1; + } + return -1; // Reached EOF without Block Terminator + } + private static boolean isPNG(File file) throws IOException { Path filePath = Paths.get(file.getPath()); byte[] bytesFromFile = Files.readAllBytes(filePath);