Skip to content

Commit e1fc556

Browse files
jacopocjacopoc
committed
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).
1 parent f8a1118 commit e1fc556

1 file changed

Lines changed: 158 additions & 0 deletions

File tree

‎framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java‎

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -582,6 +582,12 @@ private static boolean imageMadeSafe(String fileName) {
582582
if (!noWebshellInPNG(file)) {
583583
return false;
584584
}
585+
if (!noWebshellInJPEG(file)) {
586+
return false;
587+
}
588+
if (!noWebshellInGIF(file)) {
589+
return false;
590+
}
585591

586592
boolean safeState = false;
587593

@@ -752,6 +758,158 @@ private static boolean noWebshellInPNG(File file) {
752758
}
753759
}
754760

761+
private static boolean noWebshellInJPEG(File file) {
762+
try {
763+
byte[] bytes = Files.readAllBytes(file.toPath());
764+
if (!Imaging.guessFormat(bytes).equals(ImageFormats.JPEG)) {
765+
return true; // Not a JPEG file, it's OK so far
766+
}
767+
// SOI marker check
768+
if (bytes.length < 4 || (bytes[0] & 0xFF) != 0xFF || (bytes[1] & 0xFF) != 0xD8) {
769+
Debug.logError("================== Not saved for security reason, malformed JPEG ==================", MODULE);
770+
return false;
771+
}
772+
int pos = 2;
773+
while (pos < bytes.length) {
774+
if ((bytes[pos] & 0xFF) != 0xFF) {
775+
Debug.logError("================== Not saved for security reason, malformed JPEG marker ==================", MODULE);
776+
return false;
777+
}
778+
// Skip 0xFF fill bytes (valid marker padding per JPEG spec)
779+
while (pos < bytes.length && (bytes[pos] & 0xFF) == 0xFF) {
780+
pos++;
781+
}
782+
if (pos >= bytes.length) {
783+
Debug.logError("================== Not saved for security reason, JPEG missing EOI ==================", MODULE);
784+
return false;
785+
}
786+
int marker = bytes[pos++] & 0xFF;
787+
if (marker == 0xD9) {
788+
// EOI — reject any trailing bytes
789+
if (pos != bytes.length) {
790+
Debug.logError("================ Not saved for security reason, JPEG has trailing bytes after EOI ================", MODULE);
791+
return false;
792+
}
793+
return true;
794+
} else if (marker >= 0xD0 && marker <= 0xD8) {
795+
// SOI (0xD8) and RST0–RST7 (0xD0–0xD7) — no length field
796+
continue;
797+
} else if (marker == 0xDA) {
798+
// SOS: length-prefixed header followed by entropy-coded scan data
799+
if (pos + 2 > bytes.length) return false;
800+
int len = ((bytes[pos] & 0xFF) << 8) | (bytes[pos + 1] & 0xFF);
801+
if (len < 2 || pos + len > bytes.length) return false;
802+
pos += len; // Skip SOS header
803+
// Scan entropy-coded data, respecting byte stuffing (FF 00) and restart markers
804+
while (pos < bytes.length - 1) {
805+
if ((bytes[pos] & 0xFF) == 0xFF) {
806+
int next = bytes[pos + 1] & 0xFF;
807+
if (next == 0x00 || (next >= 0xD0 && next <= 0xD7)) {
808+
pos += 2; // Stuffed 0xFF or RST — part of scan data
809+
} else {
810+
break; // Real marker — stop scanning scan data
811+
}
812+
} else {
813+
pos++;
814+
}
815+
}
816+
} else {
817+
// Regular length-delimited segment
818+
if (pos + 2 > bytes.length) return false;
819+
int len = ((bytes[pos] & 0xFF) << 8) | (bytes[pos + 1] & 0xFF);
820+
if (len < 2 || pos + len > bytes.length) return false;
821+
pos += len;
822+
}
823+
}
824+
Debug.logError("================== Not saved for security reason, JPEG missing EOI ==================", MODULE);
825+
return false;
826+
} catch (IOException error) {
827+
Debug.logError("================== Not saved for security reason ==================" + error, MODULE);
828+
return false;
829+
}
830+
}
831+
832+
private static boolean noWebshellInGIF(File file) {
833+
try {
834+
byte[] bytes = Files.readAllBytes(file.toPath());
835+
if (!Imaging.guessFormat(bytes).equals(ImageFormats.GIF)) {
836+
return true; // Not a GIF file, it's OK so far
837+
}
838+
// Header: "GIF87a" or "GIF89a"
839+
if (bytes.length < 13) return false;
840+
String gifHeader = new String(bytes, 0, 6, StandardCharsets.US_ASCII);
841+
if (!"GIF87a".equals(gifHeader) && !"GIF89a".equals(gifHeader)) {
842+
Debug.logError("================== Not saved for security reason, malformed GIF ==================", MODULE);
843+
return false;
844+
}
845+
int pos = 6;
846+
// Logical Screen Descriptor: packed byte at offset 4 within LSD
847+
int packed = bytes[pos + 4] & 0xFF;
848+
boolean hasGCT = (packed & 0x80) != 0;
849+
int gctSize = packed & 0x07;
850+
pos += 7;
851+
// Skip Global Color Table
852+
if (hasGCT) {
853+
int gctBytes = 3 * (1 << (gctSize + 1));
854+
if (pos + gctBytes > bytes.length) return false;
855+
pos += gctBytes;
856+
}
857+
// Parse blocks until Trailer
858+
while (pos < bytes.length) {
859+
int blockType = bytes[pos++] & 0xFF;
860+
if (blockType == 0x3B) {
861+
// Trailer — reject any trailing bytes
862+
if (pos != bytes.length) {
863+
Debug.logError("=============== Not saved for security reason, GIF has trailing bytes after Trailer ===============", MODULE);
864+
return false;
865+
}
866+
return true;
867+
} else if (blockType == 0x21) {
868+
// Extension: label byte + sub-blocks
869+
if (pos >= bytes.length) return false;
870+
pos++; // Skip extension label
871+
pos = skipGIFSubBlocks(bytes, pos);
872+
if (pos < 0) return false;
873+
} else if (blockType == 0x2C) {
874+
// Image Descriptor: 9 bytes
875+
if (pos + 9 > bytes.length) return false;
876+
int imagePacked = bytes[pos + 8] & 0xFF;
877+
boolean hasLCT = (imagePacked & 0x80) != 0;
878+
int lctSize = imagePacked & 0x07;
879+
pos += 9;
880+
if (hasLCT) {
881+
int lctBytes = 3 * (1 << (lctSize + 1));
882+
if (pos + lctBytes > bytes.length) return false;
883+
pos += lctBytes;
884+
}
885+
pos++; // LZW minimum code size
886+
pos = skipGIFSubBlocks(bytes, pos);
887+
if (pos < 0) return false;
888+
} else {
889+
Debug.logError("================== Not saved for security reason, unknown GIF block type ==================", MODULE);
890+
return false;
891+
}
892+
}
893+
Debug.logError("================== Not saved for security reason, GIF missing Trailer ==================", MODULE);
894+
return false;
895+
} catch (IOException error) {
896+
Debug.logError("================== Not saved for security reason ==================" + error, MODULE);
897+
return false;
898+
}
899+
}
900+
901+
private static int skipGIFSubBlocks(byte[] bytes, int pos) {
902+
while (pos < bytes.length) {
903+
int blockSize = bytes[pos++] & 0xFF;
904+
if (blockSize == 0) {
905+
return pos; // Block Terminator
906+
}
907+
pos += blockSize;
908+
if (pos > bytes.length) return -1;
909+
}
910+
return -1; // Reached EOF without Block Terminator
911+
}
912+
755913
private static boolean isPNG(File file) throws IOException {
756914
Path filePath = Paths.get(file.getPath());
757915
byte[] bytesFromFile = Files.readAllBytes(filePath);

0 commit comments

Comments
 (0)