Skip to content

Improved: Enhance validation checks for file uploads #1157

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jacopoc merged 3 commits into from
May 4, 2026
Merged

Improved: Enhance validation checks for file uploads #1157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a46e26
Fixed: Path traversal via productId in ImageManagementServices
jacopoc May 4, 2026
6e57e6e
Fixed: Inspect all IDAT chunks and reject trailing bytes in noWebshel…
jacopoc May 4, 2026
076d584
Improved: Explicitly reject JPEG and GIF uploads with trailing bytes
jacopoc May 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 60 additions & 1 deletion ...oduct/src/main/java/org/apache/ofbiz/product/imagemanagement/ImageManagementServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -92,6 +93,14 @@ public static Map<String, Object> 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()) {
Expand Down Expand Up @@ -306,11 +315,20 @@ public static Map<String, Object> 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);
Expand Down Expand Up @@ -369,7 +387,16 @@ private static Map<String, Object> 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));
Expand Down Expand Up @@ -536,6 +563,14 @@ public static Map<String, Object> 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<String, Object> contentThumb = new HashMap<>();
Expand Down Expand Up @@ -729,6 +764,14 @@ public static Map<String, Object> 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;
Expand Down Expand Up @@ -801,6 +844,14 @@ public static Map<String, Object> 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));
Expand Down Expand Up @@ -831,6 +882,14 @@ public static Map<String, Object> renameImage(DispatchContext dctx, Map<String,
String productId = (String) context.get("productId");
String contentId = (String) context.get("contentId");
String filenameToUse = (String) context.get("drDataResourceName");
// 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 rename image, productId: " + productId, MODULE);
return ServiceUtil.returnError(UtilProperties.getMessage(RES_ERROR,
"ProductImageViewUnableWriteFile", UtilMisc.toMap("fileName", resolvedProductDir.toString()), locale));
}
String imageType = filenameToUse.substring(filenameToUse.lastIndexOf('.'));
String imgExtension = filenameToUse.substring(filenameToUse.length() - 3, filenameToUse.length());
String imageUrl = imageServerUrl + "/" + productId + "/" + filenameToUse;
Expand Down
205 changes: 188 additions & 17 deletions framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -581,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;

Expand Down Expand Up @@ -712,33 +719,197 @@ 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;
}
}

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);
Expand Down
Loading