From e6f26dc9457a086b81da8e563f7febafbd6321bf 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. (cherry picked from commit 11f4e525eae26e965af40de049b8f4328dc872a2) --- .../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 5d21a70698c..2c82f3ef9ab 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. (cherry picked from commit f8a1118459e4022efee8d1ca3b2f038e932b5988) --- .../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 c1a1a20dea6..a698c60a686 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; @@ -698,27 +699,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 ba5d5b00011f23a09ace485c7a6e9fe836d2a60b 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). (cherry picked from commit e1fc5563295e8071083cf5ff275593452d7461d9) --- .../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 a698c60a686..b5110a15607 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 @@ -568,6 +568,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; @@ -738,6 +744,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);